Re: Sparse Question

Re: Sparse Question

[Al Viro]

On Mon, Mar 31, 2008 at 02:39:58PM -0700, Harvey Harrison wrote:
> On Mon, 2008-03-31 at 14:15 -0700, Harvey Harrison wrote:
> > Hi Al,
> > 
> > Further to eliminating some of the trivial sparse noise in a kernel
> > build, I just can't seem to understand what sparse is warning about:
> > 
> 
> I should have mentioned, the other block of warnings comes from
> drivers/media/video/videodev.c....again initializing arrays of IOCTLs

1 ? 0 : x

is not valid in contexts where C requires integer constant expressions.
Index in static array initializer is one of those.

gcc allows it, but its extensions in that area are inconsistent, to say
the least - basically, it goes with "if optimizer can fold that into
constant with this set of options, it will be accepted".  With very weird
boundary between accepted and not accepted (as in "reorder arguments of +,
and what had been recognized as constant is not recognized anymore").

sparse doesn't even try to duplicate that set of bugs.  We _could_ try
to go for a more or less reasonable subset (e.g. ?: with integer constant
expression as the first argument and integer constant expression as
the second or the third resp., depending on the value of the first one,
similar for && and ||), but I'm not all that sure that it's worth doing.

The fact is, use of what we have for _IOC in such contexts is not just
a gccism, it's ill-defined one.  I suspect that the right solution is
to sanitize _that_...

FWIW, why not simply put division by 0 into the branch that shouldn't
be reached instead of using a variable that doesn't exist and would
blow at ld(1) time?  I.e. go with
#define _IOC_TYPECHECK(t) \
        ((sizeof(t) == sizeof(t[1]) && \
          sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
          sizeof(t) : 1/0)
instead.  I'd say that trading a pretty name in linker stderr for
compiler error that shows exact location in the source would be
a good bargain...

Linus, would you object against that in post-2.6.25?

Re: Sparse Question

[Harvey Harrison]

On Mon, 2008-03-31 at 22:58 +0100, Al Viro wrote:
> On Mon, Mar 31, 2008 at 02:39:58PM -0700, Harvey Harrison wrote:
> > On Mon, 2008-03-31 at 14:15 -0700, Harvey Harrison wrote:
> > > Hi Al,
> > > 
> > > Further to eliminating some of the trivial sparse noise in a kernel
> > > build, I just can't seem to understand what sparse is warning about:
> > > 
> > 
> > I should have mentioned, the other block of warnings comes from
> > drivers/media/video/videodev.c....again initializing arrays of IOCTLs
> 
> 1 ? 0 : x
> 
> is not valid in contexts where C requires integer constant expressions.
> Index in static array initializer is one of those.
> 
> gcc allows it, but its extensions in that area are inconsistent, to say
> the least - basically, it goes with "if optimizer can fold that into
> constant with this set of options, it will be accepted".  With very weird
> boundary between accepted and not accepted (as in "reorder arguments of +,
> and what had been recognized as constant is not recognized anymore").
> 
> sparse doesn't even try to duplicate that set of bugs.  We _could_ try
> to go for a more or less reasonable subset (e.g. ?: with integer constant
> expression as the first argument and integer constant expression as
> the second or the third resp., depending on the value of the first one,
> similar for && and ||), but I'm not all that sure that it's worth doing.
> 
> The fact is, use of what we have for _IOC in such contexts is not just
> a gccism, it's ill-defined one.  I suspect that the right solution is
> to sanitize _that_...
> 
> FWIW, why not simply put division by 0 into the branch that shouldn't
> be reached instead of using a variable that doesn't exist and would
> blow at ld(1) time?  I.e. go with
> #define _IOC_TYPECHECK(t) \
>         ((sizeof(t) == sizeof(t[1]) && \
>           sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
>           sizeof(t) : 1/0)
> instead.  I'd say that trading a pretty name in linker stderr for
> compiler error that shows exact location in the source would be
> a good bargain...
> 
> Linus, would you object against that in post-2.6.25?

Sorry, maybe I'm thick, but how does _IOC_TYPECHECK get pulled
into the _IOC_NR use?

Harvey

Re: Sparse Question

[Harvey Harrison]

On Mon, 2008-03-31 at 15:07 -0700, Harvey Harrison wrote:
> Sorry, maybe I'm thick, but how does _IOC_TYPECHECK get pulled
> into the _IOC_NR use?
> 

Nevermind, I see it now, I was just hung up on the IOC_NR part.

Harvey

[PATCH] asm-generic: suppress sparse warning in ioctl.h

[Harvey Harrison]

1 ? 0 : x

is not valid in contexts where C requires integer constant expressions.
Index in static array initializer is one of those.

Instead of using a non-existant extern function, use 1/0 as the guard
expression to avoid using a gcc-ism.  IOC_TYPECHECK gets pulled into
some static array initializations where this is not valid.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
I've confirmed this patch 'fixes' the large blocks of sparse warnings in
static array initializers.

 include/asm-generic/ioctl.h |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/ioctl.h b/include/asm-generic/ioctl.h
index cd02729..f5ae529 100644
--- a/include/asm-generic/ioctl.h
+++ b/include/asm-generic/ioctl.h
@@ -47,12 +47,10 @@
 	 ((nr)   << _IOC_NRSHIFT) | \
 	 ((size) << _IOC_SIZESHIFT))
 
-/* provoke compile error for invalid uses of size argument */
-extern unsigned int __invalid_size_argument_for_IOC;
 #define _IOC_TYPECHECK(t) \
 	((sizeof(t) == sizeof(t[1]) && \
 	  sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
-	  sizeof(t) : __invalid_size_argument_for_IOC)
+	  sizeof(t) : 1/0)
 
 /* used to create numbers */
 #define _IO(type,nr)		_IOC(_IOC_NONE,(type),(nr),0)
-- 
1.5.5.rc1.135.g8527