code review 6281050: cmd/[568]g: correct freeing of allocated Regs.

cmd/[568]g: correct freeing of allocated Regs.

[remyoudompheng, 2012-06-04 20:09:33]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
remy@archlinux.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-06-04 20:47:05]

LGTM

Do the C compilers need this fix too?

[remyoudompheng, 2012-06-04 20:48:53]

It seems so.

[remyoudompheng, 2012-06-04 20:55:43]

Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[rsc, 2012-06-04 20:57:44]

LGTM

[rsc, 2012-06-04 20:58:29]

hang on a minute

[rsc, 2012-06-04 21:01:35]

It looks like maybe the C compilers are okay?
Or is r1 != lastr for some reason I'm missing?

I don't mind changing them, but I'd like to understand that lastr really is the
last instruction, that code was not reordered by one of the passes.

Maybe we should introduce a new loop to find the true end of the list? I think
once you get past the initial creation loop lastr may not be reliable anymore,
which is why the C compiler uses r1.

So maybe the end of each of these should say:

for(r1=firstr; r1 && r1->link; r1=r1->link)
    ;
if(r1 != R) {
     r1->link = freer;
     freer = firstr;
}

?

http://codereview.appspot.com/6281050/diff/9001/src/cmd/6c/reg.c
File src/cmd/6c/reg.c (right):

http://codereview.appspot.com/6281050/diff/9001/src/cmd/6c/reg.c#newcode670
src/cmd/6c/reg.c:670: if(lastr != R) {
In this case the for(r=first...) loop at line 654 has left r1 set to the right
thing, no?

[remyoudompheng, 2012-06-04 21:24:56]

Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[remyoudompheng, 2012-06-04 21:25:55]

Reverts the changes to the C compilers. In the Go compilers i don't see anywhere
where the r->link's could be modified.

http://codereview.appspot.com/6281050/diff/9001/src/cmd/6c/reg.c
File src/cmd/6c/reg.c (right):

http://codereview.appspot.com/6281050/diff/9001/src/cmd/6c/reg.c#newcode658
src/cmd/6c/reg.c:658: r1 = r;
ah, I see the last assignment of r1 is here, so it becomes naturally equal to
lastr.

http://codereview.appspot.com/6281050/diff/9001/src/cmd/6c/reg.c#newcode670
src/cmd/6c/reg.c:670: if(lastr != R) {
Yes, so the C compilers are ok.

http://codereview.appspot.com/6281050/diff/9001/src/cmd/6g/reg.c
File src/cmd/6g/reg.c (right):

http://codereview.appspot.com/6281050/diff/9001/src/cmd/6g/reg.c#newcode643
src/cmd/6g/reg.c:643: r1 = r->link;
here it seems r1 was used for something else so it is always nil in the end.

[rsc, 2012-06-04 23:15:53]

LGTM

you seem to have reverted 6g. please put it back.
once it's back, feel free to submit.

[remyoudompheng, 2012-06-05 04:35:21]

Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[remyoudompheng, 2012-06-05 04:43:51]

*** Submitted as http://code.google.com/p/go/source/detail?r=f1028a23b059 ***

cmd/[568]g: correct freeing of allocated Regs.

R=golang-dev, rsc
CC=golang-dev, remy
http://codereview.appspot.com/6281050