| Submitter | Patrick Georgi |
|---|---|
| Date | 2010-03-15 09:59:27 |
| Message ID | <4B9E04FF.9050308@georgi-clan.de> |
| Download | mbox | patch |
| Permalink | /patch/1055/ |
| State | Accepted |
| Commit | r5212 |
| Headers | show |
Comments
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 <patrick.georgi@coresystems.de> > I can't really verify if this is the correct thing to do, but since it fixes abuild... Acked-by: Stefan Reinauer <stepan@coresystems.de> > 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: coreboot@coreboot.org > http://www.coreboot.org/mailman/listinfo/coreboot
Stefan Reinauer <stepan@coresystems.de> 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 <patrick.georgi@coresystems.de> > > > I can't really verify if this is the correct thing to do, but since it fixes > abuild... > > Acked-by: Stefan Reinauer <stepan@coresystems.de> 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
Stefan Reinauer <stepan@coresystems.de> 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 <patrick.georgi@coresystems.de> > > > I can't really verify if this is the correct thing to do, but since it fixes > abuild... > > Acked-by: Stefan Reinauer <stepan@coresystems.de> 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 <stepan@coresystems.de> 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 <patrick.georgi@coresystems.de> > > > I can't really verify if this is the correct thing to do, but since it fixes > abuild... > > Acked-by: Stefan Reinauer <stepan@coresystems.de> To be clear. change 5210 was just plain bad and needs to be completely not just partially reverted. Eric
On Mon, Mar 15, 2010 at 10:27 AM, Eric W. Biederman <ebiederm@xmission.com> 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
On 3/15/10 8:28 PM, ron minnich wrote: > On Mon, Mar 15, 2010 at 10:27 AM, Eric W. Biederman > <ebiederm@xmission.com> 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
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 <patrick@georgi-clan.de> 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
On 3/16/10 1:38 AM, Eric W. Biederman wrote: > Patrick Georgi <patrick@georgi-clan.de> 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
On 3/16/10 1:59 AM, Stefan Reinauer wrote: > On 3/16/10 1:38 AM, Eric W. Biederman wrote: > >> Patrick Georgi <patrick@georgi-clan.de> 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
Patch
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;