Ouch: romcc "x[0] |= something" patch causes another crash

[Keith Hui]

Hi all,

I regret to report that the romcc patch circulated earlier to fix the
segfault I reported, is now causing another segfault. This also seems to be
triggered by something in the 440BX code, as it didn't segfault when I
compile for any mainboards that isn't 440BX. As of now I don't know what
this new segfault is. I'll report back with more findings.

Thanks
Keith

Ouch: romcc "x[0] |= something" patch causes another crash

[Patrick Georgi]

Am 15.03.2010 03:32, schrieb Keith Hui:
> Hi all,
>
> I regret to report that the romcc patch circulated earlier to fix the
> segfault I reported, is now causing another segfault. This also seems to
> be triggered by something in the 440BX code, as it didn't segfault when
> I compile for any mainboards that isn't 440BX. As of now I don't know
> what this new segfault is. I'll report back with more findings.

It seems the problem was that copy_triple() isn't supposed to be used on
flattened (and simple) nodes.
I built a simple test case that failed:
void main(void) {
        int c = 0;
        c |= 4;
}

With the attached patch, this testcase, your testcase, and a full abuild
run work.

Signed-off-by: Patrick Georgi

Ouch: romcc "x[0] |= something" patch causes another crash

[Stefan Reinauer]

On 3/15/10 10:59 AM, Patrick Georgi wrote:
> Am 15.03.2010 03:32, schrieb Keith Hui:
> > > Hi all,
> > >
> > > I regret to report that the romcc patch circulated earlier to fix the
> > > segfault I reported, is now causing another segfault. This also seems to
> > > be triggered by something in the 440BX code, as it didn't segfault when
> > > I compile for any mainboards that isn't 440BX. As of now I don't know
> > > what this new segfault is. I'll report back with more findings.
>
> It seems the problem was that copy_triple() isn't supposed to be used on
> flattened (and simple) nodes.
> I built a simple test case that failed:
> void main(void) {
>         int c = 0;
>         c |= 4;
> }
>
> With the attached patch, this testcase, your testcase, and a full abuild
> run work.
>
> Signed-off-by: Patrick Georgi

I can't really verify if this is the correct thing to do, but since it
fixes abuild...

Acked-by: Stefan Reinauer
> 20100315-2-romcc
> Index: util/romcc/romcc.c
>
> --- util/romcc/romcc.c	(Revision 5210)
> +++ util/romcc/romcc.c	(Arbeitskopie)
> @@ -11557,7 +11557,7 @@
>
> static struct triple *assignment_expr(struct compile_state *state)
>  {
> struct triple *def, *left, *right;
> struct triple *def, *left, *left2, *right;
> int tok, op, sign;
> /* The C grammer in K&R shows assignment expressions
> only taking unary expressions as input on their
>
> @@ -11578,6 +11578,9 @@
>     */
>    def = conditional_expr(state);
>    left = def;
> left2 = left;
> if (!(left2->id & TRIPLE_FLAG_FLATTENED))
> left2 = copy_triple(state, left2);
> switch((tok = peek(state))) {
> case TOK_EQ:
> lvalue(state, left);
>
> @@ -11603,19 +11606,19 @@
>    	}
>    	def = write_expr(state, left,
>    		triple(state, op, left->type,
> 		read_expr(state, copy_triple(state, left)), right));
> 		read_expr(state, left2), right));
> break;
> case TOK_PLUSEQ:
> lvalue(state, left);
> eat(state, TOK_PLUSEQ);
> def = write_expr(state, left,
> 	mk_add_expr(state, copy_triple(state, left), assignment_expr(state)));
> 	mk_add_expr(state, left2, assignment_expr(state)));
> break;
> case TOK_MINUSEQ:
> lvalue(state, left);
> eat(state, TOK_MINUSEQ);
> def = write_expr(state, left,
> 	mk_sub_expr(state, copy_triple(state, left), assignment_expr(state)));
> 	mk_sub_expr(state, left2, assignment_expr(state)));
> break;
> case TOK_SLEQ:
> case TOK_SREQ:
>
> @@ -11639,7 +11642,7 @@
>    	}
>    	def = write_expr(state, left,
>    		triple(state, op, left->type,
> 		read_expr(state, copy_triple(state,left)), right));
> 		read_expr(state, left2), right));
> break;
> }
> return def;
>
> -- coreboot mailing list: [e-mail]
> http://www.coreboot.org/mailman/listinfo/coreboot

Stefan Reinauer  writes:
> On 3/15/10 10:59 AM, Patrick Georgi wrote:
> Am 15.03.2010 03:32, schrieb Keith Hui:
>
>
>     > Hi all,
>     >
>     > I regret to report that the romcc patch circulated earlier to fix the
>     > segfault I reported, is now causing another segfault. This also seems to
>     > be triggered by something in the 440BX code, as it didn't segfault when
>     > I compile for any mainboards that isn't 440BX. As of now I don't know
>     > what this new segfault is. I'll report back with more findings.
>
>
> It seems the problem was that copy_triple() isn't supposed to be used on
> flattened (and simple) nodes.
> I built a simple test case that failed:
> void main(void) {
>         int c = 0;
>         c |= 4;
> }
>
> With the attached patch, this testcase, your testcase, and a full abuild
> run work.
>
> Signed-off-by: Patrick Georgi <[e-mail]>
>
>
> I can't really verify if this is the correct thing to do, but since it fixes
> abuild...
>
> Acked-by: Stefan Reinauer

Looking at the rest fragment that has been passed around I think the
actual bug is that romcc allows non-static non-const arrays to be
declared.  I can not find any indication that I ever added support for
this when I wrote romcc.

My practical concern is that there is no support for the general case where
you do:
char array[5];
for (i = 0; i < 5; i++) {
    array[i] = 7;
}

I certainly don't see a test case for arrays declared in variables.

Until we have solid support for array indexing of variables stored in
registers there is no real point in implementing any other support for
arrays stored in registers.  It will lead to false expectations and
strange debugging sessions.

Furthermore there is no point in using arrays stored in registers if
you have to refer to the elements without indirect addressing.

This feels like feature addition through the back door of bug reports,
which is really the wrong way to go about it.

Eric

[ron minnich]

On Mon, Mar 15, 2010 at 10:27 AM, Eric W. Biederman
 wrote:
> My practical concern is that there is no support for the general case where
> you do:
> char array[5];
> for (i = 0; i < 5; i++) {
>    array[i] = 7;
> }

The bigger problem is that people are trying to take this compiler
beyond what makes sense (to me). I'm not sure we're going to cease to
exist if we don't have arrays.

If romcc can't do something, then work around it; we can warn people
about no arrays. But given that nobody has the time to really support
romcc (as, e.g., gcc or llvm are supported) we're taking some real
risks just plugging changes in.

ron

Ouch: romcc "x[0] |= something" patch causes another crash

[Stefan Reinauer]

On 3/15/10 8:28 PM, ron minnich wrote:
> On Mon, Mar 15, 2010 at 10:27 AM, Eric W. Biederman
>  wrote:
> > My practical concern is that there is no support for the general case where
> > you do:
> > char array[5];
> > for (i = 0; i < 5; i++) {
> >    array[i] = 7;
> > }
>
> The bigger problem is that people are trying to take this compiler
> beyond what makes sense (to me). I'm not sure we're going to cease to
> exist if we don't have arrays.

I agree we don't necessarily need to have such arrays. I think we just
naturally assumed it should work,
so no attempt to sneak anything in.

What's implemented and what is not is hidden in Eric's brain and in a
single file of 25k lines of code.

If we don't want non-static non-const arrays, can we easily detect them
in the code and give the user an error message that is better than a
segfault?
"You're a fool because you used non-const non-static arrays in romcc"
would be fine. Just dropping dead without error is certainly less helpful.
> If romcc can't do something, then work around it; we can warn people
> about no arrays. But given that nobody has the time to really support
> romcc (as, e.g., gcc or llvm are supported) we're taking some real
> risks just plugging changes in.

The changes were merely trying to fix the segfaults, not implement or
change anything big. I think we do want fixes for segfaults. Always.

Stefan

Ouch: romcc "x[0] |= something" patch causes another crash

[Patrick Georgi]

Am 15.03.2010 20:48, schrieb Stefan Reinauer:
> The changes were merely trying to fix the segfaults, not implement or
> change anything big. I think we do want fixes for segfaults. Always.

To be fair, the original issue wasn't a segfault, but an internal
compiler error - not exactly any more helpful, but still...

The segfault is what I introduced while trying to fix a feature that
doesn't even exist.

Patrick

Patrick Georgi  writes:
> Am 15.03.2010 20:48, schrieb Stefan Reinauer:
> > The changes were merely trying to fix the segfaults, not implement or
> > change anything big. I think we do want fixes for segfaults. Always.
>
> To be fair, the original issue wasn't a segfault, but an internal
> compiler error - not exactly any more helpful, but still...
>
> The segfault is what I introduced while trying to fix a feature that
> doesn't even exist.

Could you work to ensure that the remnants of that bad change
get out of the tree..

Thanks,
Eric

Ouch: romcc "x[0] |= something" patch causes another crash

[Stefan Reinauer]

On 3/16/10 1:38 AM, Eric W. Biederman wrote:
> Patrick Georgi  writes:
> > Am 15.03.2010 20:48, schrieb Stefan Reinauer:
> > > The changes were merely trying to fix the segfaults, not implement or
> > > change anything big. I think we do want fixes for segfaults. Always.
> >
> > To be fair, the original issue wasn't a segfault, but an internal
> > compiler error - not exactly any more helpful, but still...
> >
> > The segfault is what I introduced while trying to fix a feature that
> > doesn't even exist.
>
> Could you work to ensure that the remnants of that bad change
> get out of the tree..

That would be r5214.

Thanks,

Stefan

Ouch: romcc "x[0] |= something" patch causes another crash

[Stefan Reinauer]

On 3/16/10 1:59 AM, Stefan Reinauer wrote:
> On 3/16/10 1:38 AM, Eric W. Biederman wrote:
> > Patrick Georgi  writes:
> > > Am 15.03.2010 20:48, schrieb Stefan Reinauer:
> > > > The changes were merely trying to fix the segfaults, not implement or
> > > > change anything big. I think we do want fixes for segfaults. Always.
> > >
> > > To be fair, the original issue wasn't a segfault, but an internal
> > > compiler error - not exactly any more helpful, but still...
> > >
> > > The segfault is what I introduced while trying to fix a feature that
> > > doesn't even exist.
> >
> > Could you work to ensure that the remnants of that bad change
> > get out of the tree..
>
> That would be r5214.

Wanna go ahead and check your fix in?

Stefan

Stefan Reinauer  writes:
> On 3/15/10 8:28 PM, ron minnich wrote:
> > On Mon, Mar 15, 2010 at 10:27 AM, Eric W. Biederman
> >  wrote:
> > > My practical concern is that there is no support for the general case where
> > > you do:
> > > char array[5];
> > > for (i = 0; i < 5; i++) {
> > >    array[i] = 7;
> > > }
> >
> > The bigger problem is that people are trying to take this compiler
> > beyond what makes sense (to me). I'm not sure we're going to cease to
> > exist if we don't have arrays.
>
> I agree we don't necessarily need to have such arrays. I think we just
> naturally assumed it should work,
> so no attempt to sneak anything in.
>
> What's implemented and what is not is hidden in Eric's brain and in a
> single file of 25k lines of code.
>
> If we don't want non-static non-const arrays, can we easily detect them
> in the code and give the user an error message that is better than a
> segfault?
> "You're a fool because you used non-const non-static arrays in romcc"
> would be fine. Just dropping dead without error is certainly less helpful.

We should be able to. I thought such a warning existed.  So I have been
scratching my head a bit to understand why it doesn't.
> > If romcc can't do something, then work around it; we can warn people
> > about no arrays. But given that nobody has the time to really support
> > romcc (as, e.g., gcc or llvm are supported) we're taking some real
> > risks just plugging changes in.
>
> The changes were merely trying to fix the segfaults, not implement or
> change anything big. I think we do want fixes for segfaults. Always.

Fixing segfaults and then getting code that doesn't work correctly
is worse.  Always.  I did my best with romcc to ensure the compile
fails if we are not going to generate the correct code.

At the same time I would much rather it be an assert than a random
segfault.

I have to run but I think this patch adds the missing check to
catch non-static arrays.

Index: romcc.c
===================================================================
--- romcc.c	(revision 4892)
+++ romcc.c	(working copy)
@@ -13458,6 +13458,10 @@
    if ((type->type & TYPE_MASK) == TYPE_FUNCTION) {
    	error(state, 0, "Function prototypes not supported");
    }
+	if (ident &&
+		((type->type & TYPE_MASK) == TYPE_ARRAY) &&
+		((type->type & STOR_MASK) != STOR_STATIC))
+		error(state, 0, "non static arrays not supported");
    if (ident && 
    	((type->type & STOR_MASK) == STOR_STATIC) &&
    	((type->type & QUAL_CONST) == 0)) {

Eric

Ouch: romcc "x[0] |= something" patch causes another crash

[Stefan Reinauer]

On 3/15/10 9:18 PM, Eric W. Biederman wrote:
> > > If romcc can't do something, then work around it; we can warn people
> > > about no arrays. But given that nobody has the time to really support
> > > romcc (as, e.g., gcc or llvm are supported) we're taking some real
> > > risks just plugging changes in.
> >
> > The changes were merely trying to fix the segfaults, not implement or
> > change anything big. I think we do want fixes for segfaults. Always.
>
> Fixing segfaults and then getting code that doesn't work correctly
> is worse.  Always.

Absolutely true. We're just only learning that we were this stupid :-)
> I did my best with romcc to ensure the compile
> fails if we are not going to generate the correct code.
>
> At the same time I would much rather it be an assert than a random
> segfault.
>
> I have to run but I think this patch adds the missing check to
> catch non-static arrays.

Seems to do the job... Please send a Signed-off-by: for the books:
http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure

and this will make it into the tree.

You can of course check it in yourself if you wish, in any case this is
Acked-by: Stefan Reinauer
> Index: romcc.c
>
> --- romcc.c	(revision 4892)
> +++ romcc.c	(working copy)
> @@ -13458,6 +13458,10 @@
>    if ((type->type & TYPE_MASK) == TYPE_FUNCTION) {
>    	error(state, 0, "Function prototypes not supported");
>    }
> if (ident &&
> ((type->type & TYPE_MASK) == TYPE_ARRAY) &&
> ((type->type & STOR_MASK) != STOR_STATIC))
> error(state, 0, "non static arrays not supported");
> if (ident && 
> ((type->type & STOR_MASK) == STOR_STATIC) &&
> ((type->type & QUAL_CONST) == 0)) {
>
> Eric

Stefan Reinauer  writes:
> Seems to do the job... Please send a Signed-off-by: for the books:
> http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure

Signed-off-by: "Eric W. Biederman"
> and this will make it into the tree.
>
> You can of course check it in yourself if you wish, in any case this is
> Acked-by: Stefan Reinauer

Just my bad habit of not signing off on patches I haven't even tested.
> > Index: romcc.c
> >
> > --- romcc.c	(revision 4892)
> > +++ romcc.c	(working copy)
> > @@ -13458,6 +13458,10 @@
> >    if ((type->type & TYPE_MASK) == TYPE_FUNCTION) {
> >    	error(state, 0, "Function prototypes not supported");
> >    }
> > if (ident &&
> > ((type->type & TYPE_MASK) == TYPE_ARRAY) &&
> > ((type->type & STOR_MASK) != STOR_STATIC))
> > error(state, 0, "non static arrays not supported");
> > if (ident && 
> > ((type->type & STOR_MASK) == STOR_STATIC) &&
> > ((type->type & QUAL_CONST) == 0)) {

Eric

Ouch: romcc "x[0] |= something" patch causes another crash

[Stefan Reinauer]

On 3/16/10 1:31 AM, Eric W. Biederman wrote:
> Stefan Reinauer  writes:
> > Seems to do the job... Please send a Signed-off-by: for the books:
> > http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure
>
> Signed-off-by: "Eric W. Biederman"

Thanks a lot.. r5230
> > and this will make it into the tree.
> >
> > You can of course check it in yourself if you wish, in any case this is
> > Acked-by: Stefan Reinauer
>
> Just my bad habit of not signing off on patches I haven't even tested.

We'll to that for you if you keep them coming,... at least trying ;-)

Stefan Reinauer  writes:
> On 3/15/10 10:59 AM, Patrick Georgi wrote:
> Am 15.03.2010 03:32, schrieb Keith Hui:
>
>
>     > Hi all,
>     >
>     > I regret to report that the romcc patch circulated earlier to fix the
>     > segfault I reported, is now causing another segfault. This also seems to
>     > be triggered by something in the 440BX code, as it didn't segfault when
>     > I compile for any mainboards that isn't 440BX. As of now I don't know
>     > what this new segfault is. I'll report back with more findings.
>
>
> It seems the problem was that copy_triple() isn't supposed to be used on
> flattened (and simple) nodes.
> I built a simple test case that failed:
> void main(void) {
>         int c = 0;
>         c |= 4;
> }
>
> With the attached patch, this testcase, your testcase, and a full abuild
> run work.
>
> Signed-off-by: Patrick Georgi <[e-mail]>
>
>
> I can't really verify if this is the correct thing to do, but since it fixes
> abuild...
>
> Acked-by: Stefan Reinauer

Looking at the rest fragment that has been passed around I think the
actual bug is that romcc allows non-static non-const arrays to be
declared.  I can not find any indication that I ever added support for
this when I wrote romcc.

My practical concern is that there is no support for the general case where
you do:
char array[5];
for (i = 0; i < 5; i++) {
    array[i] = 7;
}

I certainly don't see a test case for arrays declared in variables.

Until we have solid support for array indexing of variables stored in
registers there is no real point in implementing any other support for
arrays stored in registers.  It will lead to false expectations and
strange debugging sessions.

Furthermore there is no point in using arrays stored in registers if
you have to refer to the elements without indirect addressing.

This feels like feature addition through the back door of bug reports,
which is really the wrong way to go about it.

....

Looking at the patch a little more it is definitely wrong.  I have
array indexing abstracted into read_expr and write_expr and those are
the functions that need to be modified to add support for arrays in
registers.

Eric

Stefan Reinauer  writes:
> On 3/15/10 10:59 AM, Patrick Georgi wrote:
> Am 15.03.2010 03:32, schrieb Keith Hui:
>
>
>     > Hi all,
>     >
>     > I regret to report that the romcc patch circulated earlier to fix the
>     > segfault I reported, is now causing another segfault. This also seems to
>     > be triggered by something in the 440BX code, as it didn't segfault when
>     > I compile for any mainboards that isn't 440BX. As of now I don't know
>     > what this new segfault is. I'll report back with more findings.
>
>
> It seems the problem was that copy_triple() isn't supposed to be used on
> flattened (and simple) nodes.
> I built a simple test case that failed:
> void main(void) {
>         int c = 0;
>         c |= 4;
> }
>
> With the attached patch, this testcase, your testcase, and a full abuild
> run work.
>
> Signed-off-by: Patrick Georgi <[e-mail]>
>
>
> I can't really verify if this is the correct thing to do, but since it fixes
> abuild...
>
> Acked-by: Stefan Reinauer

To be clear. change 5210 was just plain bad and needs to be completely
not just partially reverted.

Eric