Skip to contentSkip to conversation

Sparse Question

PATCH New Al Viro 2 participants 4 messages Mar 31, 2008 2 messages not in the archive Generic include/asm header files

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 1206998108.6543.74.camel@brick

A message that is not in the archive (sent off-list or not yet received): 1206999598.6543.76.camel@brick

Al Viro

author
#2 lore raw
Quoted 9 lines · Harvey Harrison
Quoted 5 lines · Harvey Harrison

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?

#3 lore raw
Quoted 39 lines · Al Viro
Quoted 9 lines · Harvey Harrison
Quoted 5 lines · Harvey Harrison

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)
Quoted 5 lines

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

#4 lore raw
Quoted 3 lines · Harvey Harrison

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

#5 lore raw

PATCH 1/1 asm-generic: suppress sparse warning in ioctl.h

In reply to Al Viro

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 <[email protected]>

---
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(-)
include/asm-generic/ioctl.h +1 -3
index cd02729..f5ae529 100644
@@ -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)
Signature · 1 line
1.5.5.rc1.135.g8527