Re: [PATCHv2 0/3] catch non-sign-extended '~' brainos

[Phil Carmody <phil@dovecot.fi>]

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@dovecot.fi> 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