sparse segfaults

sparse segfaults

[Russell King]

Linus,

Sparse appears to segfault when trying to check kernel/timer.c:

  CC      kernel/ptrace.o
  CHECK   /home/rmk/bk/linux-2.6-rmk/kernel/timer.c
make[2]: *** [kernel/timer.o] Error 139
make[1]: *** [kernel] Error 2
make: *** [_all] Error 2

It doesn't seem to matter which ARM machine I have my kernel configured
for, the result is always the same.

#0  0x08059222 in expand_conditional (expr=0xf6a88c4c) at expand.c:478
478                     *expr = *true;
(gdb) where
#0  0x08059222 in expand_conditional (expr=0xf6a88c4c) at expand.c:478
#1  0x08059956 in expand_expression (expr=0xf6a88c4c) at expand.c:859
#2  0x08059a32 in expand_symbol (sym=0x5) at expand.c:917
#3  0x08048cc0 in clean_up_symbols (list=0x9464fb0) at check.c:100
#4  0x08048eb5 in main (argc=40, argv=0xfef19dc4) at check.c:192
(gdb) print expr
$1 = (struct expression *) 0xf6a88c4c
(gdb) print true
$2 = (struct expression *) 0x0
(gdb) print *expr
$3 = {type = EXPR_CONDITIONAL, op = 63, pos = {type = 6, stream = 1,
    newline = 0, whitespace = 1, pos = 22, line = 566, noexpand = 0},
  ctype = 0x80764a0, {value = 4138241036, fvalue = <invalid float value>,
    string = 0xf6a88c0c, unop = 0xf6a88c0c, statement = 0xf6a88c0c,
    expr_list = 0xf6a88c0c}}

Unfortunately, gdb won't show me the contents of expr->cond_true nor
expr->cond_false without the following:

(gdb) print *(&expr->string)
$4 = (struct string *) 0xf6a88c0c
(gdb) print *(&expr->string+1)
$5 = (struct string *) 0x0
(gdb) print *(&expr->string+2)
$6 = (struct string *) 0xf6a88c6c

Looks like expr->cond_true is NULL.  Line 566 of kernel/timer.c is:

int tickadj = 500/HZ ? : 1;             /* microsecs */

which makes it look like sparse doesn't understand such constructions.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: sparse segfaults

[Linus Torvalds]

On Sat, 20 Nov 2004, Russell King wrote:
> 
> Looks like expr->cond_true is NULL.  Line 566 of kernel/timer.c is:
> 
> int tickadj = 500/HZ ? : 1;             /* microsecs */
> 
> which makes it look like sparse doesn't understand such constructions.

Sparse does, but there were some changes in how it handles them, and they
were obviously buggy for the trivial compile-time-evaluation case. I
didn't see them because on all _my_ archiectures, 500/HZ evaluates to
false, so it doesn't look at the true case (which is a special case).

Trivial fix checked in and pushed out. Thanks,

		Linus

Re: sparse segfaults

[Russell King]

On Sat, Nov 20, 2004 at 08:58:28AM -0800, Linus Torvalds wrote:
> Trivial fix checked in and pushed out. Thanks,

Confirmed fixed, thanks.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: sparse segfaults

[linux-os]

On Sat, 20 Nov 2004, Russell King wrote:

> Linus,
>
> Sparse appears to segfault when trying to check kernel/timer.c:
>
>  CC      kernel/ptrace.o
>  CHECK   /home/rmk/bk/linux-2.6-rmk/kernel/timer.c
> make[2]: *** [kernel/timer.o] Error 139
> make[1]: *** [kernel] Error 2
> make: *** [_all] Error 2
>
> It doesn't seem to matter which ARM machine I have my kernel configured
> for, the result is always the same.
>
> #0  0x08059222 in expand_conditional (expr=0xf6a88c4c) at expand.c:478
> 478                     *expr = *true;
> (gdb) where
> #0  0x08059222 in expand_conditional (expr=0xf6a88c4c) at expand.c:478
> #1  0x08059956 in expand_expression (expr=0xf6a88c4c) at expand.c:859
> #2  0x08059a32 in expand_symbol (sym=0x5) at expand.c:917
> #3  0x08048cc0 in clean_up_symbols (list=0x9464fb0) at check.c:100
> #4  0x08048eb5 in main (argc=40, argv=0xfef19dc4) at check.c:192
> (gdb) print expr
> $1 = (struct expression *) 0xf6a88c4c
> (gdb) print true
> $2 = (struct expression *) 0x0
> (gdb) print *expr
> $3 = {type = EXPR_CONDITIONAL, op = 63, pos = {type = 6, stream = 1,
>    newline = 0, whitespace = 1, pos = 22, line = 566, noexpand = 0},
>  ctype = 0x80764a0, {value = 4138241036, fvalue = <invalid float value>,
>    string = 0xf6a88c0c, unop = 0xf6a88c0c, statement = 0xf6a88c0c,
>    expr_list = 0xf6a88c0c}}
>
> Unfortunately, gdb won't show me the contents of expr->cond_true nor
> expr->cond_false without the following:
>
> (gdb) print *(&expr->string)
> $4 = (struct string *) 0xf6a88c0c
> (gdb) print *(&expr->string+1)
> $5 = (struct string *) 0x0
> (gdb) print *(&expr->string+2)
> $6 = (struct string *) 0xf6a88c6c
>
> Looks like expr->cond_true is NULL.  Line 566 of kernel/timer.c is:
>
> int tickadj = 500/HZ ? : 1;             /* microsecs */
>
> which makes it look like sparse doesn't understand such constructions.

I don't think any 'C' compiler should understand such constructions
either.
 	There is no result for the TRUE condition, and the standard
does not provide for a default. The compiler should have written
a diagnostic.


>
> -- 
> Russell King
> Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
> maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
>                 2.6 Serial core


Cheers,
Dick Johnson
Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by John Ashcroft.
                  98.36% of all statistics are fiction.

Re: sparse segfaults

[Nikita Danilov]

linux-os <linux-os@chaos.analogic.com> writes:

> On Sat, 20 Nov 2004, Russell King wrote:
>

[...]

>>
>> int tickadj = 500/HZ ? : 1;             /* microsecs */
>>
>> which makes it look like sparse doesn't understand such constructions.
>
> I don't think any 'C' compiler should understand such constructions
> either.
>  	There is no result for the TRUE condition, and the standard
> does not provide for a default. The compiler should have written
> a diagnostic.

This is GCC extension.

    foo ? : bar

is equivalent to

    foo ? foo : bar

except that foo is evaluated only once.

Nikita.

Re: sparse segfaults

[Linus Torvalds]

On Sun, 21 Nov 2004, linux-os wrote:
> >
> > int tickadj = 500/HZ ? : 1;             /* microsecs */
> >
> > which makes it look like sparse doesn't understand such constructions.
> 
> I don't think any 'C' compiler should understand such constructions
> either.
>  	There is no result for the TRUE condition, and the standard
> does not provide for a default. The compiler should have written
> a diagnostic.

Actually, this is documented gcc behaviour, where a missing true condition 
is substituted with the condition value.

So what the above does is set "tickadj" to 500/HZ _except_ if that 
underflows to zero, in which case tickadj gets the value 1.

IOW, it's the same as

	int tickadj = 500/HZ ? 500/HZ : 1;

except that the short syntax is not only shorter, but it's extremely
convenient in macros etc, because it only evaluates the value once, ie you
can do

	int tickadj = *ptr++ ? : 1;

and it's well-behaved in that it increments the pointer only once.

		Linus

Re: sparse segfaults

[Jan Engelhardt]

>Actually, this is documented gcc behaviour,[...]
>you can do
>	int tickadj = *ptr++ ? : 1;
>and it's well-behaved in that it increments the pointer only once.

And it's specific to GCC. This kinda ruins some tries to get ICC working on the
kernel tree :)



Jan Engelhardt
-- 

Re: sparse segfaults

[Mitchell Blank Jr]

Jan Engelhardt wrote:
> >Actually, this is documented gcc behaviour,[...]
> >you can do
> >	int tickadj = *ptr++ ? : 1;
> >and it's well-behaved in that it increments the pointer only once.
> 
> And it's specific to GCC. This kinda ruins some tries to get ICC working on the
> kernel tree :)

By ICC do you mean the Intel compiler?  It's supported the GCC Extension
"Conditionals with Omitted Operands" since at least version 5.0.1.  See:
  http://www.intel.com/software/products/compilers/c50/linux/comp501.pdf

-Mitch

Re: sparse segfaults

[Linus Torvalds]

On Sun, 21 Nov 2004, Jan Engelhardt wrote:
> 
> And it's specific to GCC. This kinda ruins some tries to get ICC working on the
> kernel tree :)

Ahh, but Intel compiler developers are cunning, and can make (and afaik, 
_did_ make) icc compatible with the good gcc extensions.

And as we all know, the definition of "good gcc extension" really is "the
kernel uses it". (Some of the good ones turned up in C99 and are thus no
longer extensions - obviously gcc wasn't the only compiler that
implemented them).

Good gcc extensions:
 - the inline asm syntax (which could be better, but hey, the gcc syntax
   is certainly not the worst around either)
 - statement expressions (but dammit, it should have used "return" to 
   return the value)
 - short conditionals
 - symbol attributes (sections, "used", asm naming, etc)
 - local labels and computed goto
 - case ranges
 - typeof and alignof.
 - void * arithmetic

BAD gcc extensions:
 - nested functions (gaah)
 - extended lvalues (casts, conditionals and comma-operators as lvalues. 
   They are just too confusing)
 - transparent unions (dammit, you could have done it with overloading
   instead).

		Linus

Re: sparse segfaults

[Jan Engelhardt]

>BAD gcc extensions:

You don't have to use them...


Jan Engelhardt
-- 
Gesellschaft für Wissenschaftliche Datenverarbeitung
Am Fassberg, 37077 Göttingen, www.gwdg.de

Re: sparse segfaults

[Linus Torvalds]

On Mon, 22 Nov 2004, Jan Engelhardt wrote:
>
> >BAD gcc extensions:
> 
> You don't have to use them...

We don't, generally. But they are bad even if you DON'T use them, because 
they sometimes make obvious syntax errors etc much harder to debug.

For example, the "nested function" thing makes something as simple as a 
missing end brace cause the error reporting to be totally off, when gcc 
decides that "hey, that's ok, those other function declarations are just 
nested functions in the middle of that other function". So you get 
something like

	file.c: lastline: parse error at end of input

even though the _real_ parse error could have been pinpointed exactly if 
gcc did NOT do it's totally braindamaged nested functions. IOW, the 
extension causes problems even when you don't use it.

Same goes for the "extended lvalues". They are not only insane, but they 
mean that code like

	(0,i) = 1;

actually compiles. Why is that a problem? Because some people (ie me) have 
used constructs like this in macros to make sure that the macro is 
"read-only", ie you have a situation where you don't want people to 
mis-use the macro on some architecture. So having

	int max_of_something;
	#define MAX_SOMETHING (0,max_of_something)

is actually a nice way to make sure nobody does anything like

	MAX_SOMETHING = new;

but the gcc extension means that this doesn't actually work.

(Yes, I've been bitten by this. And no, I don't see the _point_ of the
extension - does anybody actually admit to ever _using_ comma- expressions
for assignments?)

		Linus

Re: sparse segfaults

[Jan Engelhardt]

>For example, the "nested function" thing makes something as simple as a
>missing end brace cause the error reporting to be totally off, when gcc

Oh yeah I've experienced that myself (and ever since, fear nested functions).
The compiler generates like "nested_function.0" for a nested function entitled
"nested_function". So far so good, but GDB does not now "nested_function.0",
even if it is a valid symbol according to `nm`. Sigh.

>Same goes for the "extended lvalues". They are not only insane, but they
>mean that code like
>	(0,i) = 1;
>extension - does anybody actually admit to ever _using_ comma- expressions
>for assignments?)

Not in C, at least. And neither in C++. (Only Perl if you ask, but that has
vastly different semantics for "($thing,$anoterthing)".)

I totally agree that some extensions are like superfluous. Some may say "nice
to have", but go make a survey and ask how many users use them. Bet zero on
the average? ;)



Jan Engelhardt
-- 
Gesellschaft für Wissenschaftliche Datenverarbeitung
Am Fassberg, 37077 Göttingen, www.gwdg.de

Re: sparse segfaults

[Mitchell Blank Jr]

Linus Torvalds wrote:
> So having
> 
> 	int max_of_something;
> 	#define MAX_SOMETHING (0,max_of_something)
> 
> is actually a nice way to make sure nobody does anything like
> 
> 	MAX_SOMETHING = new;

When I want to do that I just use:

	#define MAX_SOMETHING (max_of_something + 0)

When gcc accepts an arbitrary algebraic expression as an lvalue I'll be
impressed :-)

-Mitch

Re: sparse segfaults

[Linus Torvalds]

On Mon, 22 Nov 2004, Mitchell Blank Jr wrote:
> 
> When I want to do that I just use:
> 
> 	#define MAX_SOMETHING (max_of_something + 0)

Yes, I think I've done that too, and that works. My point is that the 
silly comma-expression should _also_ work, and I don't see any _valid_ use 
of the comma-expression as an lvalue.

I suspect (but don't have any real argument to back that up) is that the
gcc "extended lvalues" fell out as a side effect from how gcc ended up
doing some random semantic tree parsing, and it wasn't really "designed"
per se, as much as just a random implementation detail. Then somebody 
noticed it, and said "cool" and documented it.

That's actually in my opinion a really good way to occasionally find a
more generic form of something - the act of noticing that an algorithm
just happens to give unintentional side effects that can actually be
mis-used. So I don't think that it's a bad way per se to add features,
especially as they then are often free (or even "negative cost", since
_not_ adding the feature would entail having to add a check against it).

And for all I know, many of the _good_ gcc features ended up being done 
that way too. 

But I think the (unintentional) downsides of these features are bigger 
than the advantages.

		Linus

Re: sparse segfaults

[Duncan Sands]

> I suspect (but don't have any real argument to back that up) is that the
> gcc "extended lvalues" fell out as a side effect from how gcc ended up
> doing some random semantic tree parsing, and it wasn't really "designed"
> per se, as much as just a random implementation detail. Then somebody 
> noticed it, and said "cool" and documented it.

Generalized lvalues have been removed.  Check out
http://gcc.gnu.org/ml/gcc/2004-11/msg00604.html

All the best,

Duncan.

Re: sparse segfaults

[Nix]

On 22 Nov 2004, Duncan Sands mused:
> Generalized lvalues have been removed.  Check out
> http://gcc.gnu.org/ml/gcc/2004-11/msg00604.html

There is talk of putting a subset of them back again, because a *lot* of
code does things like

((foo_t *)foo)++;

and the generalized lvalues extension makes that work as expected. Yes,
all such code is technically broken, but a large number of non-GCC
compilers also implement the extension enough for the construct above to
be valid.


Where it's really bad is in C++, where it can change the semantics of
some otherwise-valid code (due to the way it interacts with function
overloading). The whole generalized lvalues extension is definitely not
coming back, because fixing that C++ bug was a major reason why it was
removed in the first place.

-- 
`The sword we forged has turned upon us
 Only now, at the end of all things do we see
 The lamp-bearer dies; only the lamp burns on.'

Re: sparse segfaults

[Linus Torvalds]

On Mon, 22 Nov 2004, Mitchell Blank Jr wrote:
> 
> When gcc accepts an arbitrary algebraic expression as an lvalue I'll be
> impressed :-)

Btw, the "+0" thing is something that actually might be dropped pretty 
early, and as such a compiler _might_ get it wrong just because it ended 
up optimizing the expression away. So you don't need to be all that 
impressed, certain trivial expressions might just disappear under some 
circumstances.

Side note: the _biggest_ reason why "+0" is hard to optimize away early is
actually type handling, not the expression itself. The C type rules means
that "+0" isn't actually a no-op: it implies type expansion for small
integer types etc.

So I agree that it's unlikely to be a problem in practice, but I literally 
think that the reason gcc ends up considering a comma-operator to be an 
lvalue, but not a +-operator really _is_ the type-casting issues. A comma 
doesn't do implicit type expansion.

What I find really strange is the ternary operator lvalue thing, though. A 
ternary operator _does_ do type expansion, so that extended lvalue thing 
is really quite complex for ternary ops. Try something like this:

	int test(int arg)
	{
		char c;
		int i;

		return (arg ? c : i) = 1023;
	}

and think about what a total disaster that is. Yes, gcc gets it right, but
dammit, what a total crock. The people who thought of this feature should
just be shot.

(Yes, it looks cool. Oh, well. The compiler can always simplify the 
expression "(a ? b : c) = d" into "tmp = d ; a ? b = tmp : c = tmp", but 
hey, so can the user, so what's the point? Looking at the output from 
gcc, it really looks like gcc actually handles it as a special case, 
rather than as the generic simplification. Scary. Scary. Scary.)

		Linus

Re: sparse segfaults

[Jan Engelhardt]

Hi,


on Nov 22 2004, Linus Torvalds wrote 
( http://groups-beta.google.com/group/linux.kernel/brow
se_frm/thread/32c75ae8dd925ce7/5ca16f581a293fab?tvc=1#5ca16f581a293fab ):

>Same goes for the "extended lvalues". They are not only insane, but they
>mean that code like [...]


Well, here [http://gcc.gnu.org/gcc-3.4/changes.html] are some things (section 
"C/Objective C/C++") that will be deprecated, including ?: as an lvalue and
the behated (a,b)-as-lvalue.  ;-)


Cheers,
Jan Engelhardt
-- 
No TOFU for me, please.