[patchset] rewrite of initializer handling

[patchset] rewrite of initializer handling

[Al Viro]

	Current tree handling of initializers is rather incomplete
and in many cases broken.  Patchset rewrites that stuff pretty much
from scratch; AFAICS it works.

Fixed:
	* proper walking into subobjects, with warnings about missing
braces.  Current tree doesn't handle that and in the best case we
get a warning when initializer hits the wrong subobject and gets a type
mismatch; in the worst case we get nothing.
	* excess of initializer list elements is reported
	* unnamed bitfields are skipped as the should be; the current tree
doesn't do that
	* fixed handling of string literals; current tree is inconsistent
in that area, especially when they come from inlined functions
(evaluate_expression() changes expr->type, so it's not recognized as string
literal the next time aroung).
	* fixed handling of struct/union assignment-style initializers.
	* fixed handling of gccisms (unnamed union as a member, empty struct
as a member) to match gcc behaviour; gcc extension allowing ("a") to be
treated as "a" is handled, with a warning conditional on -Wparen-string
(default is off).  BTW, several places in the kernel have that sort of
idiocy.

Areas still missing:
	* wide string literals.  We don't support wide char anyway, so...
	* expand doesn't care for order of initializers.  For overlapping
ones that becomes a problem, especially when mixed with gcc range
designators (e.g. [20 ... 100] = {1, 2}, [0 ... 30].a = 15, [49] = {15, 20})
	* expand still leaves EXPR_POS -> EXPR_POS -> EXPR_VALUE for
[0 ... 1][0] = 1, which makes compile-i386.c rather unhappy (segfaults).
While we are at it, compile-i386.c apparently is unaware of range designators
at all - it ignores the count.  Probably best fixed in compile-i386.c...
	* calculation of array size by initializer is still broken; at least
now we get warnings about missing braces in the cases that trigger that
crap; struct {int a, b;} a[] = {1,2,3,4,5}; should give a 3-element array,
not 5-element one.  New code warns about missing braces and puts the values
in right places, but it still doesn't fix the array size calculation - it's
done too early.  Since evaluate_initializer() can work with array of
unknown size, perhaps the best solution would be to call it from the
count_array_initializer() and look for maximal index in the results if
we have EXPR_INITIALIZER / look for string size otherwise (no other
variants are possible for arrays).  Objections?

Have fun...  The tree is in branch 'initializers' of
git://git.kernel.org//pub/scm/linux/kernel/git/viro/sparse/

Al Viro (4):
      make copying of EXPR_INDEX non-lazy
      tie the fields of struct in simple list
      rewrite of initializer handling
      fix handling of typeof on structs

 evaluate.c   |  569 ++++++++++++++++++++++++++++++++++++++++------------------
 expression.h |    1 +
 inline.c     |    2 -
 lib.c        |    2 +
 lib.h        |    1 +
 parse.c      |   20 ++-
 symbol.c     |    7 +-
 symbol.h     |    6 +-
 8 files changed, 428 insertions(+), 180 deletions(-)

Re: [patchset] rewrite of initializer handling

[Al Viro]

On Mon, Jun 18, 2007 at 11:19:29AM +0100, Al Viro wrote:
> Have fun...  The tree is in branch 'initializers' of
> git://git.kernel.org//pub/scm/linux/kernel/git/viro/sparse/

Grr...  That would be

git://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse/

BTW, I wonder why does git care about double slash in that place (or
later in the path, for that matter).  Path normalization and healthy
paranoia about people doing something fishy if it doesn't match the
original literally?

Re: [patchset] rewrite of initializer handling

[Linus Torvalds]

On Mon, 18 Jun 2007, Al Viro wrote:
>
> 	Current tree handling of initializers is rather incomplete
> and in many cases broken.  Patchset rewrites that stuff pretty much
> from scratch; AFAICS it works.

Josh - you may already be aware of it, but I thought I'd point it out 
anyway: when Al Viro sends me a patch (or set of patches), I personally 
just realize that a superior intellect has contacted me, told me I'm 
wrong, and given me the keys to fix it.

IOW, Al's patches tend to be a good point to just say "Oh, ok, I was a 
moron for ever writing anything else, thank you for not pointing it out 
any more impolitely and not chewing my head off", and then just applying 
or pulling them.

Which is not to say that I've never gotten a patch without a bug in it 
from Al, but I've never seen Al do something that wasn't fundamentally the 
"Right Thing(tm)" to do.

IOW, Al is one of those few people where I can just close my eyes and say 
"yup, I'll apply it" without even bothering to look at the patch.

			Linus

Re: [patchset] rewrite of initializer handling

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

Linus Torvalds wrote:
> On Mon, 18 Jun 2007, Al Viro wrote:
>> 	Current tree handling of initializers is rather incomplete
>> and in many cases broken.  Patchset rewrites that stuff pretty much
>> from scratch; AFAICS it works.
> 
> Josh - you may already be aware of it, but I thought I'd point it out 
> anyway: when Al Viro sends me a patch (or set of patches), I personally 
> just realize that a superior intellect has contacted me, told me I'm 
> wrong, and given me the keys to fix it.
> 
> IOW, Al's patches tend to be a good point to just say "Oh, ok, I was a 
> moron for ever writing anything else, thank you for not pointing it out 
> any more impolitely and not chewing my head off", and then just applying 
> or pulling them.
> 
> Which is not to say that I've never gotten a patch without a bug in it 
> from Al, but I've never seen Al do something that wasn't fundamentally the 
> "Right Thing(tm)" to do.
> 
> IOW, Al is one of those few people where I can just close my eyes and say 
> "yup, I'll apply it" without even bothering to look at the patch.

Yeah, I've observed the same thing from activity in both the kernel community
and the Sparse community.  I still look at the patches, but primarily so I
can understand them and learn something from them, not because I expect to
find anything wrong. :)

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [patchset] rewrite of initializer handling

[Al Viro]

On Mon, Jun 18, 2007 at 11:02:55AM -0700, Josh Triplett wrote:
 
> Yeah, I've observed the same thing from activity in both the kernel community
> and the Sparse community.  I still look at the patches, but primarily so I
> can understand them and learn something from them, not because I expect to
> find anything wrong. :)

That's never a good idea, no matter where the patches are coming from.
Odds are that you have a different set of blind spots, so having "why
isn't that broken here?" as background attitude while reading any code
tends to find exactly the crap that happens to be hard to find for author
of the code in question...

Re: [patchset] rewrite of initializer handling

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 2770 bytes --]

Al Viro wrote:
> 	Current tree handling of initializers is rather incomplete
> and in many cases broken.  Patchset rewrites that stuff pretty much
> from scratch; AFAICS it works.

Patchset applied; thanks!

The validation output changed slightly:

-validation/initializer-entry-defined-twice.c:10:12: error: Initializer entry defined twice
-validation/initializer-entry-defined-twice.c:11:12:   also defined here
-validation/initializer-entry-defined-twice.c:25:7: error: Initializer entry defined twice
-validation/initializer-entry-defined-twice.c:27:8:   also defined here
+validation/initializer-entry-defined-twice.c:10:3: error: Initializer entry defined twice
+validation/initializer-entry-defined-twice.c:11:3:   also defined here
+validation/initializer-entry-defined-twice.c:26:4: error: Initializer entry defined twice
+validation/initializer-entry-defined-twice.c:27:4:   also defined here

Given that this just affected the column numbers, and sparse still reports the
correct line numbers, I didn't worry about it.

> 	* fixed handling of gccisms (unnamed union as a member, empty struct
> as a member) to match gcc behaviour; gcc extension allowing ("a") to be
> treated as "a" is handled, with a warning conditional on -Wparen-string
> (default is off).  BTW, several places in the kernel have that sort of
> idiocy.

How much spew does -Wparen-string cause?  If you feel that it always
represents an error, or at least sloppy code, and that it won't drown people
in warnings, I have no problem with turning it on by default.  Your call.

> Areas still missing:
[...]
> 	* calculation of array size by initializer is still broken; at least
> now we get warnings about missing braces in the cases that trigger that
> crap; struct {int a, b;} a[] = {1,2,3,4,5}; should give a 3-element array,
> not 5-element one.  New code warns about missing braces and puts the values
> in right places, but it still doesn't fix the array size calculation - it's
> done too early.  Since evaluate_initializer() can work with array of
> unknown size, perhaps the best solution would be to call it from the
> count_array_initializer() and look for maximal index in the results if
> we have EXPR_INITIALIZER / look for string size otherwise (no other
> variants are possible for arrays).  Objections?

That seems like a great approach to me; logically, an initializer should
expand an array of unspecified size to hold elements up to its maximum index.

Hopefully this would also fix the problem reported by Michael Stefaniuc in
<466489FD.7010405@redhat.com>:
> Running sparse on
>         int i = sizeof((const char []) {'a','a','a',0});
> gives
>         zzz.c:1:9: error: cannot size expression

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [patchset] rewrite of initializer handling

[Al Viro]

On Mon, Jun 18, 2007 at 11:19:08AM -0700, Josh Triplett wrote:
> How much spew does -Wparen-string cause?  If you feel that it always
> represents an error, or at least sloppy code, and that it won't drown people
> in warnings, I have no problem with turning it on by default.  Your call.

Depends on a project.  In case of kernel it's mostly a spew in s2io.c
around
static char ethtool_driver_stats_keys[][ETH_GSTRING_LEN] = {
        {"\n DRIVER STATISTICS"},
        {"single_bit_ecc_errs"},
        {"double_bit_ecc_errs"},
        {"parity_err_cnt"},
        {"serious_err_cnt"},
        {"soft_reset_cnt"},
        {"fifo_full_cnt"},
        {"ring_full_cnt"},
        ("alarm_transceiver_temp_high"),
        ("alarm_transceiver_temp_low"),
        ("alarm_laser_bias_current_high"),
        ("alarm_laser_bias_current_low"),
        ("alarm_laser_output_power_high"),
....

Mind you, both braces and parentheses are redundant here, but the latter
happen to be invalid C as well (and we want at least consistency anyway).

Other projects may differ.  IOW, I'd probably keep it optional.
 
> > Areas still missing:
> [...]
> > 	* calculation of array size by initializer is still broken; at least
> > now we get warnings about missing braces in the cases that trigger that
> > crap; struct {int a, b;} a[] = {1,2,3,4,5}; should give a 3-element array,
> > not 5-element one.  New code warns about missing braces and puts the values
> > in right places, but it still doesn't fix the array size calculation - it's
> > done too early.  Since evaluate_initializer() can work with array of
> > unknown size, perhaps the best solution would be to call it from the
> > count_array_initializer() and look for maximal index in the results if
> > we have EXPR_INITIALIZER / look for string size otherwise (no other
> > variants are possible for arrays).  Objections?
> 
> That seems like a great approach to me; logically, an initializer should
> expand an array of unspecified size to hold elements up to its maximum index.

That's what count_array_initializer() is trying to do, but the trouble is
that it doesn't notice that some list elements may end up initializing
parts of the same array member.  IOW, it tries to do the right thing,
but to do it correctly you need to look at more than just "this has
explicit designator, this doesn't".

I'm not sure that I understand the details of type examination in symbol.c
enough to say whether we'd break anything by doing it that way, though.
The question is how much of the laziness would be destroyed by that.
Linus?

> Hopefully this would also fix the problem reported by Michael Stefaniuc in
> <466489FD.7010405@redhat.com>:
> > Running sparse on
> >         int i = sizeof((const char []) {'a','a','a',0});
> > gives
> >         zzz.c:1:9: error: cannot size expression

Umm...  I don't think that it's related.  count_array_initializer() would
work just fine for that one, since the straightforward list element counting
would work as-is.

Aha... I see what's going on - in evaluate_cast() we examine the type before
associating it with initializer, so when we get around to evaluate_symbol()
a bit later in the same function, it's too late - ->examined is already
set.  I wonder if moving that examine_symbol_type() downstream (and killing
it in EXPR_INITIALIZER branch) would be enough...  Looks like it should
work, but I might be missing something here.

How does something like diff below look to you, folks?  It gets the
testcase to produce expected result (and puts the right value into
i); I'm running it on the kernel cross-builds right now, but that
doesn't guarantee correctness, of course.

diff --git a/evaluate.c b/evaluate.c
index c564ad9..6da8f3e 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2416,7 +2416,7 @@ out:
 static struct symbol *evaluate_cast(struct expression *expr)
 {
 	struct expression *target = expr->cast_expression;
-	struct symbol *ctype = examine_symbol_type(expr->cast_type);
+	struct symbol *ctype;
 	struct symbol *t1, *t2;
 	int class1, class2;
 	int as1, as2;
@@ -2424,9 +2424,6 @@ static struct symbol *evaluate_cast(struct expression *expr)
 	if (!target)
 		return NULL;
 
-	expr->ctype = ctype;
-	expr->cast_type = ctype;
-
 	/*
 	 * Special case: a cast can be followed by an
 	 * initializer, in which case we need to pass
@@ -2441,7 +2438,7 @@ static struct symbol *evaluate_cast(struct expression *expr)
 		struct symbol *sym = expr->cast_type;
 		struct expression *addr = alloc_expression(expr->pos, EXPR_SYMBOL);
 
-		sym->initializer = expr->cast_expression;
+		sym->initializer = target;
 		evaluate_symbol(sym);
 
 		addr->ctype = &lazy_ptr_ctype;	/* Lazy eval */
@@ -2455,6 +2452,10 @@ static struct symbol *evaluate_cast(struct expression *expr)
 		return sym;
 	}
 
+	ctype = examine_symbol_type(expr->cast_type);
+	expr->ctype = ctype;
+	expr->cast_type = ctype;
+
 	evaluate_expression(target);
 	degenerate(target);
 

Re: [patchset] rewrite of initializer handling

[Linus Torvalds]

On Mon, 18 Jun 2007, Al Viro wrote:
> > 
> > That seems like a great approach to me; logically, an initializer should
> > expand an array of unspecified size to hold elements up to its maximum index.
> 
> That's what count_array_initializer() is trying to do, but the trouble is
> that it doesn't notice that some list elements may end up initializing
> parts of the same array member.  IOW, it tries to do the right thing,
> but to do it correctly you need to look at more than just "this has
> explicit designator, this doesn't".
> 
> I'm not sure that I understand the details of type examination in symbol.c
> enough to say whether we'd break anything by doing it that way, though.
> The question is how much of the laziness would be destroyed by that.
> Linus?

I really forget - but I do remember that arrays were a bitch from the type 
handling perspective. We need to examine their types properly early, and 
then we wanted to set their size as part of that, and there were various 
subtle ordering requirements there.

It is entirely possible that your patch fixes these things, but that whole 
type evaluation is quite easy to break by mistake..

			Linus

Re: [patchset] rewrite of initializer handling

[Michael Stefaniuc]

Al Viro wrote:
> On Mon, Jun 18, 2007 at 11:19:08AM -0700, Josh Triplett wrote:
>> Hopefully this would also fix the problem reported by Michael Stefaniuc in
>> <466489FD.7010405@redhat.com>:
>>> Running sparse on
>>>         int i = sizeof((const char []) {'a','a','a',0});
>>> gives
>>>         zzz.c:1:9: error: cannot size expression
> 
> Umm...  I don't think that it's related.  count_array_initializer() would
> work just fine for that one, since the straightforward list element counting
> would work as-is.
Right, it doesn't fixes this error. Actually the output of
make clean; CHECK='sparse -Wno-transparent-union -Wno-old-initializer
-Wno-non-pointer-null' make CC=cgcc > build.out 2>&1
is identical bit by bit with or without Al's patch series.

> Aha... I see what's going on - in evaluate_cast() we examine the type before
> associating it with initializer, so when we get around to evaluate_symbol()
> a bit later in the same function, it's too late - ->examined is already
> set.  I wonder if moving that examine_symbol_type() downstream (and killing
> it in EXPR_INITIALIZER branch) would be enough...  Looks like it should
> work, but I might be missing something here.
> 
> How does something like diff below look to you, folks?  It gets the
> testcase to produce expected result (and puts the right value into
> i); I'm running it on the kernel cross-builds right now, but that
> doesn't guarantee correctness, of course.
Works for Wine and removes most of the "error: cannot size expression"
errors. The 7 remaining errors are preceded by "error: bad constant
expression" in the same line. Have to condense those into a test case too.

> diff --git a/evaluate.c b/evaluate.c
> index c564ad9..6da8f3e 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -2416,7 +2416,7 @@ out:
>  static struct symbol *evaluate_cast(struct expression *expr)
>  {
>  	struct expression *target = expr->cast_expression;
> -	struct symbol *ctype = examine_symbol_type(expr->cast_type);
> +	struct symbol *ctype;
>  	struct symbol *t1, *t2;
>  	int class1, class2;
>  	int as1, as2;
> @@ -2424,9 +2424,6 @@ static struct symbol *evaluate_cast(struct expression *expr)
>  	if (!target)
>  		return NULL;
>  
> -	expr->ctype = ctype;
> -	expr->cast_type = ctype;
> -
>  	/*
>  	 * Special case: a cast can be followed by an
>  	 * initializer, in which case we need to pass
> @@ -2441,7 +2438,7 @@ static struct symbol *evaluate_cast(struct expression *expr)
>  		struct symbol *sym = expr->cast_type;
>  		struct expression *addr = alloc_expression(expr->pos, EXPR_SYMBOL);
>  
> -		sym->initializer = expr->cast_expression;
> +		sym->initializer = target;
>  		evaluate_symbol(sym);
>  
>  		addr->ctype = &lazy_ptr_ctype;	/* Lazy eval */
> @@ -2455,6 +2452,10 @@ static struct symbol *evaluate_cast(struct expression *expr)
>  		return sym;
>  	}
>  
> +	ctype = examine_symbol_type(expr->cast_type);
> +	expr->ctype = ctype;
> +	expr->cast_type = ctype;
> +
>  	evaluate_expression(target);
>  	degenerate(target);
>  


bye
	michael

-- 
Michael Stefaniuc               Tel.: +49-711-96437-199
Sr. Network Engineer            Fax.: +49-711-96437-111
Red Hat GmbH                    Email: mstefani@redhat.com
Hauptstaetterstr. 58            http://www.redhat.de/
D-70178 Stuttgart

Re: [patchset] rewrite of initializer handling

[Al Viro]

On Mon, Jun 18, 2007 at 11:46:13PM +0200, Michael Stefaniuc wrote:

> > Umm...  I don't think that it's related.  count_array_initializer() would
> > work just fine for that one, since the straightforward list element counting
> > would work as-is.
> Right, it doesn't fixes this error. Actually the output of
> make clean; CHECK='sparse -Wno-transparent-union -Wno-old-initializer
> -Wno-non-pointer-null' make CC=cgcc > build.out 2>&1
> is identical bit by bit with or without Al's patch series.

Eh...  count_array_initializer() is not even touched in that series anyway.
The question had been how to fix it; Josh asked if fixing it would take
care of your report as well and I said "no".  Patch that followed was
unrelated to count_array_initializer() problems or problems fixed by
patch series that went into the tree.

> Works for Wine and removes most of the "error: cannot size expression"
> errors.

Any regressions?

> The 7 remaining errors are preceded by "error: bad constant
> expression" in the same line. Have to condense those into a test case too.

Probably genuine VLAs...

Re: [patchset] rewrite of initializer handling

[Michael Stefaniuc]

Al Viro wrote:
> On Mon, Jun 18, 2007 at 11:46:13PM +0200, Michael Stefaniuc wrote:
> 
>>> Umm...  I don't think that it's related.  count_array_initializer() would
>>> work just fine for that one, since the straightforward list element counting
>>> would work as-is.
>> Right, it doesn't fixes this error. Actually the output of
>> make clean; CHECK='sparse -Wno-transparent-union -Wno-old-initializer
>> -Wno-non-pointer-null' make CC=cgcc > build.out 2>&1
>> is identical bit by bit with or without Al's patch series.
> 
> Eh...  count_array_initializer() is not even touched in that series anyway.
> The question had been how to fix it; Josh asked if fixing it would take
> care of your report as well and I said "no".  Patch that followed was
> unrelated to count_array_initializer() problems or problems fixed by
> patch series that went into the tree.
> 
>> Works for Wine and removes most of the "error: cannot size expression"
>> errors.
> 
> Any regressions?
Nope, just the errors removed and no additional warnings introduced.

>> The 7 remaining errors are preceded by "error: bad constant
>> expression" in the same line. Have to condense those into a test case too.
> 
> Probably genuine VLAs...
Right.

bye
	michael
-- 
Michael Stefaniuc               Tel.: +49-711-96437-199
Sr. Network Engineer            Fax.: +49-711-96437-111
Red Hat GmbH                    Email: mstefani@redhat.com
Hauptstaetterstr. 58            http://www.redhat.de/
D-70178 Stuttgart

Re: [patchset] rewrite of initializer handling

[Michael Stefaniuc]

Al Viro wrote:
> On Mon, Jun 18, 2007 at 11:19:08AM -0700, Josh Triplett wrote:
>> How much spew does -Wparen-string cause?  If you feel that it always
>> represents an error, or at least sloppy code, and that it won't drown people
>> in warnings, I have no problem with turning it on by default.  Your call.
> 
> Depends on a project.  In case of kernel it's mostly a spew in s2io.c
> around
> static char ethtool_driver_stats_keys[][ETH_GSTRING_LEN] = {
>         {"\n DRIVER STATISTICS"},
>         {"single_bit_ecc_errs"},
>         {"double_bit_ecc_errs"},
>         {"parity_err_cnt"},
>         {"serious_err_cnt"},
>         {"soft_reset_cnt"},
>         {"fifo_full_cnt"},
>         {"ring_full_cnt"},
>         ("alarm_transceiver_temp_high"),
>         ("alarm_transceiver_temp_low"),
>         ("alarm_laser_bias_current_high"),
>         ("alarm_laser_bias_current_low"),
>         ("alarm_laser_output_power_high"),
> ....
> 
> Mind you, both braces and parentheses are redundant here, but the latter
> happen to be invalid C as well (and we want at least consistency anyway).
> 
> Other projects may differ.  IOW, I'd probably keep it optional.
Not sure if this is a poll but -Wparen-string doesn't add any new
warnings to the Wine run. And Wine has a "strange" way of specifying
wide char strings.

bye
	michael
-- 
Michael Stefaniuc               Tel.: +49-711-96437-199
Sr. Network Engineer            Fax.: +49-711-96437-111
Red Hat GmbH                    Email: mstefani@redhat.com
Hauptstaetterstr. 58            http://www.redhat.de/
D-70178 Stuttgart

Re: [patchset] rewrite of initializer handling

[Al Viro]

On Tue, Jun 19, 2007 at 10:15:49PM +0200, Michael Stefaniuc wrote:
> Not sure if this is a poll but -Wparen-string doesn't add any new
> warnings to the Wine run. And Wine has a "strange" way of specifying
> wide char strings.

Oh?  FWIW, C99 way is L"......" for wide string literals and L'...' for
wide character ones.  sparse doesn't handle either and I'm not sure if
we really want to open that can of worms...

Re: [patchset] rewrite of initializer handling

[Michael Stefaniuc]

Al Viro wrote:
> On Tue, Jun 19, 2007 at 10:15:49PM +0200, Michael Stefaniuc wrote:
>> Not sure if this is a poll but -Wparen-string doesn't add any new
>> warnings to the Wine run. And Wine has a "strange" way of specifying
>> wide char strings.
> 
> Oh?  FWIW, C99 way is L"......" for wide string literals and L'...' for
I know but that is not portable. A wchar is 16bit on Windows and 32bit 
in the normal world. gcc has the -fshort-wchar switch to make a wchar 
16bit but that still produces problems; afair when bridging between the 
hosts native 32bit wchar and Windows 16bit wchar. To avoid that Wine uses:
WCHAR s[] = {'H','e','l','l','o',' ','W','o','r','l','d',0};

> wide character ones.  sparse doesn't handle either and I'm not sure if
> we really want to open that can of worms...
There are only 6 .c files (out of around 2000) that do not parse in Wine 
due to missing support wide char character constant / string literal.
I had a look at sparse on how to fix it but it seems to require more 
than some trivial changes. I mean to implement it correctly and not just 
to stop sparse from mis parsing the rest of the file too.

bye
	michael
-- 
Michael Stefaniuc               Tel.: +49-711-96437-199
Sr. Network Engineer            Fax.: +49-711-96437-111
Red Hat GmbH                    Email: mstefani@redhat.com
Hauptstaetterstr. 58            http://www.redhat.de/
D-70178 Stuttgart

Re: [patchset] rewrite of initializer handling

[Michael Stefaniuc]

Michael Stefaniuc wrote:
> Al Viro wrote:
>> On Tue, Jun 19, 2007 at 10:15:49PM +0200, Michael Stefaniuc wrote:
>>> Not sure if this is a poll but -Wparen-string doesn't add any new
>>> warnings to the Wine run. And Wine has a "strange" way of specifying
>>> wide char strings.
>>
>> Oh?  FWIW, C99 way is L"......" for wide string literals and L'...' for
> I know but that is not portable. A wchar is 16bit on Windows and 32bit
> in the normal world. gcc has the -fshort-wchar switch to make a wchar
> 16bit but that still produces problems; afair when bridging between the
> hosts native 32bit wchar and Windows 16bit wchar. To avoid that Wine uses:
> WCHAR s[] = {'H','e','l','l','o',' ','W','o','r','l','d',0};
> 
>> wide character ones.  sparse doesn't handle either and I'm not sure if
>> we really want to open that can of worms...
> There are only 6 .c files (out of around 2000) that do not parse in Wine
> due to missing support wide char character constant / string literal.
> I had a look at sparse on how to fix it but it seems to require more
> than some trivial changes. I mean to implement it correctly and not just
> to stop sparse from mis parsing the rest of the file too.
Duh ... my bad; i should have looked better. The files i thought sparse
 mis parses after it encounters the L'...' have a lot of wide character
constants in them thus giving the impression that sparse would not be
recovering from the error.

bye
	michael
-- 
Michael Stefaniuc               Tel.: +49-711-96437-199
Sr. Network Engineer            Fax.: +49-711-96437-111
Red Hat GmbH                    Email: mstefani@redhat.com
Hauptstaetterstr. 58            http://www.redhat.de/
D-70178 Stuttgart

Re: [patchset] rewrite of initializer handling

[Alexey Dobriyan]

sparse can segfault in while loop in is_string_literal():

static int is_string_literal(struct expression **v)
{
	struct expression *e = *v;
	while (e->type == EXPR_PREOP && e->op == '(')
===>		e = e->unop;	<===

Here expression is NULL.

Steps to reproduce:

	$ echo 'char a[][] = {(};' | sparse -
	-:1:16: error: Expected ) in expression
	-:1:16: error: got }
	Segmentation fault

Re: [patchset] rewrite of initializer handling

[Al Viro]

On Tue, Jun 19, 2007 at 09:12:36PM +0400, Alexey Dobriyan wrote:
> sparse can segfault in while loop in is_string_literal():
> 
> static int is_string_literal(struct expression **v)
> {
> 	struct expression *e = *v;
> 	while (e->type == EXPR_PREOP && e->op == '(')
> ===>		e = e->unop;	<===
> 
> Here expression is NULL.
> 
> Steps to reproduce:
> 
> 	$ echo 'char a[][] = {(};' | sparse -
> 	-:1:16: error: Expected ) in expression
> 	-:1:16: error: got }
> 	Segmentation fault

Gaack...  The fix is obvious (add e && into that condition and into
e->type == EXPR_STRING a couple of lines below), but...  I wonder
if adding EXPR_BAD and using it to deal with such crap in parser
would be better.  Comments?

Re: [patchset] rewrite of initializer handling

[Al Viro]

On Tue, Jun 19, 2007 at 06:21:43PM +0100, Al Viro wrote:
> Gaack...  The fix is obvious (add e && into that condition and into
> e->type == EXPR_STRING a couple of lines below), but...  I wonder
> if adding EXPR_BAD and using it to deal with such crap in parser
> would be better.  Comments?

Anyway, brute-force patch follows.  I still suspect that long-term
we will be better off with explicit EXPR_BAD nodes and guaranteed
things like "->unop of EXPR_PREOP is never NULL", but that can be
done separately - a lot of checks for NULL will be possible to remove.

diff --git a/evaluate.c b/evaluate.c
--- a/evaluate.c
+++ b/evaluate.c
@@ -2029,6 +2029,10 @@ static struct expression *check_designators(struct expression *e,
 			e->ctype = ctype = type;
 			ctype = type;
 			last = e;
+			if (!e->idx_expression) {
+				err = "invalid";
+				break;
+			}
 			e = e->idx_expression;
 		} else if (e->type == EXPR_IDENTIFIER) {
 			if (ctype->type != SYM_STRUCT && ctype->type != SYM_UNION) {
@@ -2042,6 +2046,10 @@ static struct expression *check_designators(struct expression *e,
 			}
 			e->field = e->ctype = ctype;
 			last = e;
+			if (!e->ident_expression) {
+				err = "invalid";
+				break;
+			}
 			e = e->ident_expression;
 		} else if (e->type == EXPR_POS) {
 			err = "internal front-end error: EXPR_POS in";
@@ -2203,9 +2211,9 @@ found:
 static int is_string_literal(struct expression **v)
 {
 	struct expression *e = *v;
-	while (e->type == EXPR_PREOP && e->op == '(')
+	while (e && e->type == EXPR_PREOP && e->op == '(')
 		e = e->unop;
-	if (e->type != EXPR_STRING)
+	if (!e || e->type != EXPR_STRING)
 		return 0;
 	if (e != *v && Wparen_string)
 		warning(e->pos,
@@ -2274,6 +2282,9 @@ static int handle_simple_initializer(struct expression **ep, int nested,
 	struct expression *e = *ep, *p;
 	struct symbol *type;
 
+	if (!e)
+		return 0;
+
 	/* scalar */
 	if (!(class & TYPE_COMPOUND)) {
 		e = handle_scalar(e, nested);