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

Previous thread: [PATCH 6/7] audit: fix sparse shadowed variable warnings by Harvey Harrison on Wednesday, April 2, 2008 - 5:33 pm. (1 message)

Next thread: [PATCH 1/1] cgroups: introduce cft->read_seq() by Serge E. Hallyn on Wednesday, April 2, 2008 - 6:01 pm. (6 messages)
From: Harvey Harrison
Date: Wednesday, April 2, 2008 - 5:33 pm

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

--

From: Linus Torvalds
Date: Wednesday, April 2, 2008 - 5:57 pm

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
--

From: Al Viro
Date: Wednesday, April 2, 2008 - 6:34 pm

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.
--

From: Linus Torvalds
Date: Wednesday, April 2, 2008 - 6:50 pm

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, 

Yeah, we do seem to have lost a maintainer.

		Linus
--

From: Al Viro
Date: Wednesday, April 2, 2008 - 6:59 pm

Um...  But that's _not_ a link-time constant - simply an int variable not
--

From: Linus Torvalds
Date: Wednesday, April 2, 2008 - 7:51 pm

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
--

From: Al Viro
Date: Wednesday, April 2, 2008 - 7:55 pm

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...
--

From: Al Viro
Date: Friday, April 4, 2008 - 2:19 am

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 ...
From: Al Viro
Date: Wednesday, April 2, 2008 - 7:41 pm

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...
--

From: Josh Triplett
Date: Thursday, April 3, 2008 - 1:31 pm

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
--

From: Al Viro
Date: Thursday, April 3, 2008 - 6:19 pm

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, ...
From: Al Viro
Date: Thursday, April 3, 2008 - 7:00 pm

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...
--

From: Derek M Jones
Date: Friday, April 4, 2008 - 7:08 am

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

Any sane compiler that performs the analysis needed to deduce that
the expression inside the parenthesis always evaluated to zero

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

--

From: Al Viro
Date: Friday, April 4, 2008 - 11:42 am

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

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

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

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.
--

Previous thread: [PATCH 6/7] audit: fix sparse shadowed variable warnings by Harvey Harrison on Wednesday, April 2, 2008 - 5:33 pm. (1 message)

Next thread: [PATCH 1/1] cgroups: introduce cft->read_seq() by Serge E. Hallyn on Wednesday, April 2, 2008 - 6:01 pm. (6 messages)