On Mon, Jun 30, 2014 at 09:44:35AM -0700, Christopher Li wrote:
> On Mon, Jun 30, 2014 at 1:56 AM, Phil Carmody <phil@xxxxxxxxxx> wrote:
> >> Also, if you only care about warning against up cast "~" of
> >> unsigned type, you can do the checking at cast_to().
> >> It will be simpler. I have a sort patch like this, given that I
> >> did not check for the "unsigned int" type, nor do I check for
> >> binary op. You can still get the idea.
> >
> > It needs to check for unsigned-ness in order to trap zero-extension, though.
>
> That is right, it is not a complete patch. Just to show you some ideas.
With unsigned, it becomes a lot quieter, but still much noiser than my
original. (I have a v3 which excludes an idiom using '/'.)
> > Will report back in the next day or so what the kernel builds say.
>
> I did some test against your patch.
> BTW, I just submit a patch for the kernel to save sparse warnings.
> Subject line starts with: [PATCH] sparse: Add CLOG ....
>
> With that patch in kernel. Here is what I did in the testing:
>
> $ make allmodconfig
> $ make -j8 C=2 CLOG=std
>
> # apply your patch and make sparse
>
> $ make -j8 C=2 CLOG=std-exp
> $ find -name ".*.std.sparse" | while read -r file; do diff -du $file
> ${file/std.sparse/std-exp.sparse} ; done > /tmp/signed-diff
>
> $ grep "dubious zero" /tmp/signed-diff | wc -l
>
> That give me 565 new warning. I attach the file here.
>
> I sample some, all the one I saw are false positive.
> e.g.
>
> static inline int nlmsg_msg_size(int payload)
> {
> return NLMSG_HDRLEN + payload; <--------warn here
> }
>
> /**
> * nlmsg_total_size - length of netlink message including padding
> * @payload: length of message payload
> */
> static inline int nlmsg_total_size(int payload)
> {
> return NLMSG_ALIGN(nlmsg_msg_size(payload));
> }
>
> #define NLMSG_ALIGNTO 4U
I consider that, pedantically, to be worth fixing. alignments are defined
by the C standard to be of type size_t. It should either be ((size_t)4)
if you care about the precise type, or 4 if you don't, but not 4U. The
'U' adds nothing apart from a way to create an incorrect mask.
Fixing that line removes ~110 out of the ~120 warnings in my powerpc build.
> #define NLMSG_ALIGN(len) ( ((len)+NLMSG_ALIGNTO-1) & ~(NLMSG_ALIGNTO-1) )
> #define NLMSG_HDRLEN ((int) NLMSG_ALIGN(sizeof(struct nlmsghdr)))
> <------------ sizeof cast up the type
> #define NLMSG_LENGTH(len) ((len) + NLMSG_HDRLEN)
However, that's a good example of where the casting back down should shut
the warning up.
Thanks for the time you've put into this, Chris. I'll do a bit more
head-scratching. I get the feeling that the zero-extend should taint the
expression, and that down-casts should clear that taint. The problem with
that is that you need to trap absolutely everywhere whate taint might
escape.
Phil
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
