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

[PATCH 7/7] 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>
---
 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

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

[Linus Torvalds]

On Wed, 2 Apr 2008, Harvey Harrison wrote:
>
> 1 ? 0 : x
> 
> is not valid in contexts where C requires integer constant expressions.
> Index in static array initializer is one of those.

So I don't much like this one, because (a) we could just make sparse 
accept it and (b) gcc _does_ accept it and gives us nicer error messages. 

Well, maybe "nicer" is wrong (because it's a link-time one), but at least 
not a _totally_ misleading one.

What does "division by zero" mean as an error message? Also, if I recall 
correctly, last time we tried something like this (admittedly long ago), 
some compilers would actually make it a run-time error, not a compile-time 
one - it would simply refuse optimize the 1/0 into a value at all, and 
just generate a run-time divide.

So I'm not even sure that all versions of gcc will even complain at all 
(although it might have been icc).

			Linus

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

[Al Viro]

On Wed, Apr 02, 2008 at 05:57:54PM -0700, Linus Torvalds wrote:
> 
> 
> On Wed, 2 Apr 2008, Harvey Harrison wrote:
> >
> > 1 ? 0 : x
> > 
> > is not valid in contexts where C requires integer constant expressions.
> > Index in static array initializer is one of those.
> 
> So I don't much like this one, because (a) we could just make sparse 
> accept it and (b) gcc _does_ accept it and gives us nicer error messages. 

Umm...  How far do you want sparse to go?  You _really_ don't want
bug-for-bug compatibility with gcc - it's far too weird (and that's
even before going into the effects of optimization flags).

BTW, what happened with sparse.git?  The last changeset in there (in
/pub/scm/devel/sparse/sparse.git/ on g.k.o) is
commit a02aeb329d5a8f9047c0b75b7e7f64ee2db3ffcf
Author: Josh Triplett <josh@freedesktop.org>
Date:   Tue Nov 13 04:15:13 2007 -0800

    Makefile: VERSION=0.4.1

and I definitely had seen patches on sparse maillist since then (hell,
sent several myself - including fixes for show_typename(), etc.)

I don't mind doing more liberal ICE handling, *if* we agree on a well-defined
extensions to what C99 says.  But I'd rather have some idea of what's pending
the inclusion into the tree...

As for the extensions...  Amend 6.6p6 in a way similar to 6.6p3 (i.e. allow
any junk in unevaluated subexpressions)?  Making that option-controlled,
probably...

BTW, gcc is very definitely buggy - int a[1 + 0 * x]; is accepted and
that breaks even 6.6p3, let alone 6.6p6.  With -pedantic -std=c99 -Wall,
at that.

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

[Linus Torvalds]

On Thu, 3 Apr 2008, Al Viro wrote:
> 
> Umm...  How far do you want sparse to go?  You _really_ don't want
> bug-for-bug compatibility with gcc - it's far too weird (and that's
> even before going into the effects of optimization flags).

I would suggest just accepting link-time constants in constant 
expressions. But yeah, maybe limit even that just to unevaluated 
subexpressions.

That said, for sparse, the right thing to do would be to use 
__builtin_warning() or something, but gcc doesn't have that kind of thing, 
so..

> BTW, what happened with sparse.git?

Yeah, we do seem to have lost a maintainer.

		Linus

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

[Al Viro]

On Wed, Apr 02, 2008 at 06:50:20PM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 3 Apr 2008, Al Viro wrote:
> > 
> > Umm...  How far do you want sparse to go?  You _really_ don't want
> > bug-for-bug compatibility with gcc - it's far too weird (and that's
> > even before going into the effects of optimization flags).
> 
> I would suggest just accepting link-time constants in constant 
> expressions. But yeah, maybe limit even that just to unevaluated 
> subexpressions.

Um...  But that's _not_ a link-time constant - simply an int variable not
defined in any object file...
 
> That said, for sparse, the right thing to do would be to use 
> __builtin_warning() or something, but gcc doesn't have that kind of thing, 
> so..
> 
> > BTW, what happened with sparse.git?
> 
> Yeah, we do seem to have lost a maintainer.

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

[Al Viro]

On Wed, Apr 02, 2008 at 06:50:20PM -0700, Linus Torvalds wrote:

> > BTW, what happened with sparse.git?
> 
> Yeah, we do seem to have lost a maintainer.

Have we?  I've seen Josh on sparse list quite recently, IIRC...
Yes - http://marc.info/?l=linux-sparse&m=120716477203477 just
yesterday...

Josh, do you have any pending patches?  I can just start a branch in
my tree, of course, but I'd rather deal with merge headache earlier
than later...

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

[Linus Torvalds]

On Thu, 3 Apr 2008, Al Viro wrote:
> 
> Um...  But that's _not_ a link-time constant - simply an int variable not
> defined in any object file...

Ahh, you're right, my bad. We could change it to take the address of it, 
though. I assume gcc is happy with that too?

			Linus

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

[Al Viro]

On Wed, Apr 02, 2008 at 07:51:08PM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 3 Apr 2008, Al Viro wrote:
> > 
> > Um...  But that's _not_ a link-time constant - simply an int variable not
> > defined in any object file...
> 
> Ahh, you're right, my bad. We could change it to take the address of it, 
> though. I assume gcc is happy with that too?

gcc might be, but what are you going to do with it?  Expression is an
integer one, after all - in cases that had triggered that thread it
was used (after & 0xff) as array index.

Besides, logically it's _not_ a constant at all - not even a base + constant,
after using it in such expression...

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

[Josh Triplett]

Al Viro wrote:
> On Wed, Apr 02, 2008 at 06:50:20PM -0700, Linus Torvalds wrote:
>>> BTW, what happened with sparse.git?
>> Yeah, we do seem to have lost a maintainer.
> 
> Have we?  I've seen Josh on sparse list quite recently, IIRC...
> Yes - http://marc.info/?l=linux-sparse&m=120716477203477 just
> yesterday...
> 
> Josh, do you have any pending patches?  I can just start a branch in
> my tree, of course, but I'd rather deal with merge headache earlier
> than later...

Starting around the beginning of this year, I got buried under
research temporarily in the Winter term, along with a couple of other
things.  I got caught up, and then proceeded to get sick for a week,
and that and recovery time put me far enough behind on both email and
research that I didn't catch up again until recently.

I mostly kept up with Sparse email, hence the recent mails, but built
up a backlog of patches to deal with.  (Sparse has a fairly low patch
rate, so that backlog constitutes less than 10 patches, counting the
ones that still need further work or discussion before they can
merge.)  Also, in switching my email to an IMAP server, I managed to
lose the read/unread status on some of my list mail, including
linux-sparse, which slowed me down a fair bit when trying to process
those mails.

I have a small handful of pending patches to work through on
linux-sparse, with yours at the top of that list; I also have some
local patches that I need to test and push.  Apologies for the
processing delay.

I should have thought to send a "still alive" note with an explanation
to linux-sparse.  I'll make sure to do so in the future.  (Some time
in the next couple of years I have a thesis to write. :) )

I'll catch the backlog up and then send out an "all patches merged"
mail, so that anyone with additional patches or anyone who thinks
their patch got lost can resend.

- Josh Triplett

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

[Al Viro]

On Thu, Apr 03, 2008 at 01:31:00PM -0700, Josh Triplett wrote:
> I mostly kept up with Sparse email, hence the recent mails, but built
> up a backlog of patches to deal with.  (Sparse has a fairly low patch
> rate, so that backlog constitutes less than 10 patches, counting the
> ones that still need further work or discussion before they can
> merge.)  Also, in switching my email to an IMAP server, I managed to
> lose the read/unread status on some of my list mail, including
> linux-sparse, which slowed me down a fair bit when trying to process
> those mails.
> 
> I have a small handful of pending patches to work through on
> linux-sparse, with yours at the top of that list; I also have some
> local patches that I need to test and push.  Apologies for the
> processing delay.
> 
> I should have thought to send a "still alive" note with an explanation
> to linux-sparse.  I'll make sure to do so in the future.  (Some time
> in the next couple of years I have a thesis to write. :) )
> 
> I'll catch the backlog up and then send out an "all patches merged"
> mail, so that anyone with additional patches or anyone who thinks
> their patch got lost can resend.

OK...  FWIW, I'll probably do relaxed integer constant expressions on
top of whatever shows up in your tree + patches I've sent to the list
at some point in the next couple of weeks.  For now I'd say that we
can keep ignoring the errors from _IO...() uses in context that expect
an i-c-e (mostly - indices in array initializers); it's not widespread
and we'd lived with that for quite a while.

As far as annoyances go, lack of VLA support is responsible for far more
lost warnings.  I don't have anything in that direction beyond rather
vague plans (*and* a monstrous backlog of my own, both kernel-side and
sparse-side, so it's unlikely to drift up the list in the next couple
of months).  Does anybody have any pending work in that direction?  It's
not _that_ hard to do, but for pity sake, go with C99 standard when doing
it; gcc is choke-full of broken and barely documented extensions in that
area, so reverse-engineering them is going to result in utter mess.

C99 VLAs are fairly dumb and straightforward - the underlying idea is
"for each variable of type that involves a VLA, compiler should be able
to slap a shadow variable containing the array size in scope nearby".
Thus
	* no variably-modified members in structs or unions (not only
no VLA, but no pointers to VLA, etc.); we would need shadow variables
for each struct instance and that obviously wouldn't work in that model).
	* no functions returning variably-modified type.
	* no variably-modified objects in global scope.
	* no VLA static in function.
	* typedef *can* be variably-modified, but only in block scope.
Warning: this can get sticky for us - all sizes are evaluated when
typedef is reached.  IOW,
	typedef int a[n];
	a x;
	if (n++ == 5) {
		a y;
		int z[n];
	}
will have size of y equal to that of x, but *not* equal to that of z.
	* do *NOT* jump inside the scope of anything variably modified;
not with goto, not with switch (again, you'll miss initialization of
shadows).

Passing a VLA (let alone pointer to such, etc.) to a function is done
exactly as we would for normal arrays; $DEITY help you if the function's
idea of sizes doesn't match that of caller - shadow stuff is _NOT_ passed
at all.  No match => undefined behaviour.

Mixing VLA with normal arrays: compatible if the elements are compatible,
but if you have actual sizes that do not match, you are in nasal daemon
country.  So composite type of two VLA ones is *either*, and if actual
sizes differ, well, too bad for you.

Basically, everything works as if you had SYM_VLA(element, symbol) that
behaved almost as SYM_ARRAY.  With initializers for symbols created at
the point where we declare the object in question (or type, in case of
typedef).  Passing such sucker to function converts to SYM_VLA with *new*
symbol - one in function scope and initialized there.  Note that this
is where the things get extremely ugly for gcc - there you can pass
an object out of its scope and _that_ is what leads to lovely internal
compiler errors for things like
	sizeof *({int n = f(); int a[n]; &a;})
since we get shadow symbol dead and gone with the scope while the type
(SYM_PTR(SYM_VLA(int, n)) outlives the scope, leaving a landmine for
sizeof.

Another thing to keep in mind is that sizeof(VLA) is not a constant
expression *and* that sizeof argument is evaluated.  IOW, not only
	sizeof(int [n = f()])
has side effects, but in
	int (*p)[n];
	...
	sizeof(*(g(), p))
will have g() evaluated.  Note that if p had been declared as
	int (*p)[4];
the same sizeof() would *NOT* have called anything.  This, BTW, is where
the rules for what an integer constant expression is are getting bloody
important: 

int n = 1;
int f(void)
{
	int (*p)[1 + (0 && n)];
	return sizeof(*(g(), p));
}

_must_ call g() according to C99, while the same with
enum {n = 1;} must not, even though n won't be even looked at in either
case *and* any sane compiler will find return value of f() at compile
time, turning it to
inf f(void)
{
	g();
	return sizeof(int);
}
in the first case and
int f(void)
{
	return sizeof(int);
}
in the second.

One more thing: use of sizeof(variably-modified type) may or may not
evaluate size expressions in there if they do not affect the result.
IOW, it's unspecified whether
	sizeof(int (*)[n++])
increments n; different compilers are broken in different ways and it
might be worth generating a warning on such.  Note that
	sizeof(int [n++])
*is* guaranteed to increment n - the unspecified part is for size expressions
in such sizeof(type) buried behind a pointer.

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

[Al Viro]

On Fri, Apr 04, 2008 at 02:19:51AM +0100, Al Viro wrote:
> 	* no functions returning variably-modified type.

Note: *pointer* to function returning a variably-modified type is possible,
is variably-modified itself and as such can appear only in function and
the same "compiler will consider VLA compatible with any array that has
as compatible element, but if the size doesn't match it's on your head"
applies.  IOW,

int a[2][2] = {{1, 2}, {3, 4}};

int (*f(void))[2] /* return a pointer to two-element array of int */
{
        return &a[0];
}

int h(int n)
{
	/* pointer to function that returns a pointer to n-element VLA of int */
        int (*(*p)(void))[n];
	/* OK if n is 2, undefined otherwise */
        p = f;
        return p()[1][1];
}

is fine and h(2) will give you 4, but if you ever do e.g. h(1), you are in
nasal daemon country.  In reality h(1) will _probably_ give a[1][0], but
compiler has every right to silently produce a binary that'll wipe your disk
or do the same itself...

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

[Al Viro]

On Thu, Apr 03, 2008 at 03:55:46AM +0100, Al Viro wrote:
> On Wed, Apr 02, 2008 at 07:51:08PM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Thu, 3 Apr 2008, Al Viro wrote:
> > > 
> > > Um...  But that's _not_ a link-time constant - simply an int variable not
> > > defined in any object file...
> > 
> > Ahh, you're right, my bad. We could change it to take the address of it, 
> > though. I assume gcc is happy with that too?
> 
> gcc might be, but what are you going to do with it?  Expression is an
> integer one, after all - in cases that had triggered that thread it
> was used (after & 0xff) as array index.
> 
> Besides, logically it's _not_ a constant at all - not even a base + constant,
> after using it in such expression...

OK, I think I see how to do it.
	* new bit in ->flags - Weak_Int_Const
	* parser sets it by almost the same rules as Int_Const_Expr, except
that ?:, || and && simply inherit it from the first argument.
	* evaluate strips it in the same cases as Int_Const_Expr, except that
recalculation for ?:, || and && ignores everything except the first argument.
	* when expand sees VALUE && VALUE or VALUE || VALUE and the second
argument is not to be ignored, Weak_Int_Const on node is removed unless the
second argument also had it.
	* when expand sees VALUE ? : and overwrites it with appropriate
branch, have Weak_Int_Const on result iff both the original node and
overwriting argument used to have it.
	* bad_integer_constant_expression() checks either Int_Const_Expr or
Weak_Int_Const, depending on flag set by -W<something>

Should work, AFAICS...  If expression has no Weak_Int_Const out of parser,
it's definitely not an integer constant expression, even with lax rules.
If expression loses Weak_Int_Const on evaluate, we must have a cast to
something other than integer type in it => not an integer constant expression.
If expression loses Weak_Int_Const on expand, we must have found a
subexpression that _might_ be ignored, had not been an integer constant
expression by weak rules and actually was not ignored => not an integer
constant expression.

In no case we could gain Weak_Int_Const and if expand ended up with
EXPR_VALUE and no commas in evaluated subexpressions, having Weak_Int_Const
is equivalent to what we want.

Handling of expr->flags might get a bit clumsy, but other than that it seems
to be easy enough...  Comments?

One thing: behaviour of -Wall is getting more and more unpleasant.  On
kernel builds sparse gets hit with it due to CFLAGS and that kills all
defaults for sparse options.  That -Wall is there for gcc; having it make
sparse to lose all defaults is wrong, especially since gcc does *not*
turn each warning on on that.

For this flag it gets particulary nasty - we have two behaviours (strict
C99 and relaxed one) and the former warns on the things that are OK by
the latter.  So the natural thing to do would be to have -W<something>
to trigger the former, right?  Now think what we'll get on the kernel
build and recall what had started this thread...

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

[Derek M Jones]

Al,

> 	* typedef *can* be variably-modified, but only in block scope.
> Warning: this can get sticky for us - all sizes are evaluated when
> typedef is reached.  IOW,
> 	typedef int a[n];
> 	a x;
> 	if (n++ == 5) {
> 		a y;
> 		int z[n];
> 	}
> will have size of y equal to that of x, but *not* equal to that of z.

An opportunity for Sparse to issue a warning :-)

> Another thing to keep in mind is that sizeof(VLA) is not a constant
> expression *and* that sizeof argument is evaluated.  IOW, not only
> 	sizeof(int [n = f()])
> has side effects, but in
> 	int (*p)[n];
> 	...
> 	sizeof(*(g(), p))
> will have g() evaluated.  Note that if p had been declared as
> 	int (*p)[4];
> the same sizeof() would *NOT* have called anything.  This, BTW, is where
> the rules for what an integer constant expression is are getting bloody
> important:

Any side effect appearing in a sizeof operand ought to be flagged.
There are people out there who think that the side effects occur (even
in C90).

Sentence 1122: http://c0x.coding-guidelines.com/6.5.3.4.html
"If the type of the operand is a variable length array type, the operand 
is evaluated;"

But watch out for sentence 1584: 
http://c0x.coding-guidelines.com/6.7.5.2.html
"Where a size expression is part of the operand of a sizeof operator and
changing the value of the size expression would not affect the result of
the operator, it is unspecified whether or not the size expression is
evaluated."

> int n = 1;
> int f(void)
> {
> 	int (*p)[1 + (0 && n)];
> 	return sizeof(*(g(), p));
> }
> 
> _must_ call g() according to C99, while the same with
> enum {n = 1;} must not, even though n won't be even looked at in either
> case *and* any sane compiler will find return value of f() at compile
> time, turning it to

Any sane compiler that performs the analysis needed to deduce that
the expression inside the parenthesis always evaluated to zero
will turn it into....

> One more thing: use of sizeof(variably-modified type) may or may not
> evaluate size expressions in there if they do not affect the result.
> IOW, it's unspecified whether
> 	sizeof(int (*)[n++])
> increments n; different compilers are broken in different ways and it

This language feature came about because at least one vendor on the
WG14 committee had a compiler that optimized away subexpressions
within a sizeof that did not contribute to the result of the
evaluation.  My attempt to stop the behavior being unspecified
did not succeed :-(

-- 
Derek M. Jones                              tel: +44 (0) 1252 520 667
Knowledge Software Ltd                      mailto:derek@knosof.co.uk
Applications Standards Conformance Testing    http://www.knosof.co.uk

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

[Al Viro]

On Fri, Apr 04, 2008 at 03:08:01PM +0100, Derek M Jones wrote:
> >	* typedef *can* be variably-modified, but only in block scope.
> >Warning: this can get sticky for us - all sizes are evaluated when
> >typedef is reached.  IOW,
> >	typedef int a[n];
> >	a x;
> >	if (n++ == 5) {
> >		a y;
> >		int z[n];
> >	}
> >will have size of y equal to that of x, but *not* equal to that of z.
> 
> An opportunity for Sparse to issue a warning :-)

Not without data flow analysis, and sparse really doesn't do that class
of checks...

> Any side effect appearing in a sizeof operand ought to be flagged.
> There are people out there who think that the side effects occur (even
> in C90).

Not by default it shouldn't; it's about the only way to do polymorphic
typechecking a-la "this argument of macro is a function pointer and
that argument could be legitimately passed to it", etc.
 
> Sentence 1122: http://c0x.coding-guidelines.com/6.5.3.4.html
> "If the type of the operand is a variable length array type, the operand 
> is evaluated;"
> 
> But watch out for sentence 1584: 
> http://c0x.coding-guidelines.com/6.7.5.2.html
> "Where a size expression is part of the operand of a sizeof operator and
> changing the value of the size expression would not affect the result of
> the operator, it is unspecified whether or not the size expression is
> evaluated."

Dealt with below, but note that the wording in 6.7.5.2 is lousy: as stated
it covers not only intended sizeof(VM) with side effects in size expressions,
but sizeof(sizeof(int [n++])) as well, which certainly should *not* be
unspecified - the n++ is a part of operand of outer sizeof and it does not
affect its value (it's sizeof(size_t)), but it certainly should _not_ be
evaluated at all since the entire argument of the outer sizeof should not
be evaluated.

AFAICS, intended rules are:
	sizeof(expression of non-VLA type): constant, argument not evaluated
	sizeof(expression of VLA type): constant, argument evaluated
	sizeof(non-VM type): constant
	sizeof(VM type): unspecified whether all size expressions are evaluated

> This language feature came about because at least one vendor on the
> WG14 committee had a compiler that optimized away subexpressions
> within a sizeof that did not contribute to the result of the
> evaluation.  My attempt to stop the behavior being unspecified
> did not succeed :-(

The really interesting question is what the hell did gcc folks intend for
their ({ ... }) and typeof extensions...  Well, aside of "some cases
when ({ ... }) would result in VM type are clearly undefined behaviour" ;-/

BTW, I wish somebody would have come up with a sane set of type-surgery
primitives...  Part of that can be emulated with typeof, but you don't get
"turn qualified-type into type" and "give the type of Nth argument of
function type" that way.