code review 204064: stop the 5c optimizer from clobbering extern static reg...

stop the 5c optimizer from clobbering extern static registers.

[kaib, 2010-02-07 05:57:55]

Hello ken (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[kaib, 2010-02-07 06:23:03]

-ken,+ken2

[ken3, 2010-02-07 23:55:31]

all of the other compilers restrict register allocation
in BtoR rather than RtoB. there is already a & of the
legal registers in BtoR. isnt that enough?

On 2010/02/07 06:23:03, kaib wrote:
> -ken,+ken2

[kaib, 2010-02-08 04:28:28]

On Sun, Feb 7, 2010 at 3:55 PM,  <ken@google.com> wrote:
> all of the other compilers restrict register allocation
> in BtoR rather than RtoB. there is already a & of the
> legal registers in BtoR. isnt that enough?

it seems like neither limit in BtoR or RtoB honor the allocation of
extern static registers. in 5c the & in BtoR statically restricts the
registers to R2-R10. however, if the compiler encounters an extern
static reg declaration (like in runtime.h for m an g) it calls exreg()
and allocates R10 and R9. given the current static check on register
feasibility in BtoR and RtoB the optimizer might decide to use R9 or
R10 and overwrite the value for m or g. the symptom i'm seeing is a
crash later in the program (typically in a morestack preamble) where
the code tries to access R10 as g and receives a SIGSEGV for accessing
a garbage pointer.

now that i look at the code i think it's broken in all three c
compilers. it might have gone unnoticed because the trigger is extern
static reg combined with a complex enough code passage to require the
use of a large number of registers. when i first noticed the bug last
year, and turned 5c optimization off, it only occurred when compiling
one or two c files in src/pkg.

to keep with the existing code i propose adding the check to both BtoR
and RtoB. if we have agreement i'll go ahead and modify the cl.

 kai

[kaib, 2010-02-08 04:49:20]

uploaded proposed changed to 5c. i'm unclear on what to do with 6c/8c.
the filter in BtoR seems to filter for 32 registers but the comments
say that register R15 and down are handled by the compiler..

kai

On Sun, Feb 7, 2010 at 8:28 PM, Kai Backman <kaib@golang.org> wrote:
> On Sun, Feb 7, 2010 at 3:55 PM,  <ken@google.com> wrote:
>> all of the other compilers restrict register allocation
>> in BtoR rather than RtoB. there is already a & of the
>> legal registers in BtoR. isnt that enough?
>
> it seems like neither limit in BtoR or RtoB honor the allocation of
> extern static registers. in 5c the & in BtoR statically restricts the
> registers to R2-R10. however, if the compiler encounters an extern
> static reg declaration (like in runtime.h for m an g) it calls exreg()
> and allocates R10 and R9. given the current static check on register
> feasibility in BtoR and RtoB the optimizer might decide to use R9 or
> R10 and overwrite the value for m or g. the symptom i'm seeing is a
> crash later in the program (typically in a morestack preamble) where
> the code tries to access R10 as g and receives a SIGSEGV for accessing
> a garbage pointer.
>
> now that i look at the code i think it's broken in all three c
> compilers. it might have gone unnoticed because the trigger is extern
> static reg combined with a complex enough code passage to require the
> use of a large number of registers. when i first noticed the bug last
> year, and turned 5c optimization off, it only occurred when compiling
> one or two c files in src/pkg.
>
> to keep with the existing code i propose adding the check to both BtoR
> and RtoB. if we have agreement i'll go ahead and modify the cl.
>
>  kai
>

[ken3, 2010-02-08 06:51:10]

i think 6c/6g and 8c/8g are fine.
you are probably confused by leftover plan9 code that
had a general extreg mechanism. it never worked unless
all code loaded by the loader used the same declarations.

in go, it is not a general mechanism. there are 2 external
registers. the first 2 extreg declarations will get them.
any internal allocations will not get them. (in 8c, these
are not even registers.)

so mentally pick the 2 dedicated registers. and make it so
regalloc will not allocate them, but extreg will. then put them
in the BtoR mask so that reg/peep will not allocate them.

On Sun, Feb 7, 2010 at 8:49 PM, Kai Backman <kaib@golang.org> wrote:
> uploaded proposed changed to 5c. i'm unclear on what to do with 6c/8c.
> the filter in BtoR seems to filter for 32 registers but the comments
> say that register R15 and down are handled by the compiler..
>
> kai
>
> On Sun, Feb 7, 2010 at 8:28 PM, Kai Backman <kaib@golang.org> wrote:
>> On Sun, Feb 7, 2010 at 3:55 PM,  <ken@google.com> wrote:
>>> all of the other compilers restrict register allocation
>>> in BtoR rather than RtoB. there is already a & of the
>>> legal registers in BtoR. isnt that enough?
>>
>> it seems like neither limit in BtoR or RtoB honor the allocation of
>> extern static registers. in 5c the & in BtoR statically restricts the
>> registers to R2-R10. however, if the compiler encounters an extern
>> static reg declaration (like in runtime.h for m an g) it calls exreg()
>> and allocates R10 and R9. given the current static check on register
>> feasibility in BtoR and RtoB the optimizer might decide to use R9 or
>> R10 and overwrite the value for m or g. the symptom i'm seeing is a
>> crash later in the program (typically in a morestack preamble) where
>> the code tries to access R10 as g and receives a SIGSEGV for accessing
>> a garbage pointer.
>>
>> now that i look at the code i think it's broken in all three c
>> compilers. it might have gone unnoticed because the trigger is extern
>> static reg combined with a complex enough code passage to require the
>> use of a large number of registers. when i first noticed the bug last
>> year, and turned 5c optimization off, it only occurred when compiling
>> one or two c files in src/pkg.
>>
>> to keep with the existing code i propose adding the check to both BtoR
>> and RtoB. if we have agreement i'll go ahead and modify the cl.
>>
>>  kai
>>
>

[kaib, 2010-02-08 07:08:26]

ptal

On Sun, Feb 7, 2010 at 10:51 PM, Ken Thompson <ken@google.com> wrote:
> i think 6c/6g and 8c/8g are fine.
> you are probably confused by leftover plan9 code that
> had a general extreg mechanism. it never worked unless
> all code loaded by the loader used the same declarations.

yeah, i was trying to preserve the function of the generic extreg
mechanism. changed CL to simpler spot fix for go.

kai

[kaib, 2010-02-08 07:10:55]

oh, i also disabled the optimizer completely until i can figure out
the issue when we run out of registers.

 kai

On Sun, Feb 7, 2010 at 11:08 PM, Kai Backman <kaib@golang.org> wrote:
> ptal
>
> On Sun, Feb 7, 2010 at 10:51 PM, Ken Thompson <ken@google.com> wrote:
>> i think 6c/6g and 8c/8g are fine.
>> you are probably confused by leftover plan9 code that
>> had a general extreg mechanism. it never worked unless
>> all code loaded by the loader used the same declarations.
>
> yeah, i was trying to preserve the function of the generic extreg
> mechanism. changed CL to simpler spot fix for go.
>
> kai
>

[rsc, 2010-02-11 01:26:32]

LGTM

http://codereview.appspot.com/204064/diff/1006/17
File src/cmd/5c/reg.c (right):

http://codereview.appspot.com/204064/diff/1006/17#newcode82
src/cmd/5c/reg.c:82: // TODO(kaib): enable optimizer once we figure out why it
generates bad
more conventional to comment out the call to regopt.

http://codereview.appspot.com/204064/diff/1006/17#newcode1154
src/cmd/5c/reg.c:1154: if(r < 2 || r >= REGTMP-2)
is the -2 needed?

http://codereview.appspot.com/204064/diff/1006/17#newcode1162
src/cmd/5c/reg.c:1162: b &= 0x01fcL;
add comment  // exclude R9 and R10 for m and g

[kaib, 2010-02-11 06:42:11]

http://codereview.appspot.com/204064/diff/1006/17
File src/cmd/5c/reg.c (right):

http://codereview.appspot.com/204064/diff/1006/17#newcode82
src/cmd/5c/reg.c:82: // TODO(kaib): enable optimizer once we figure out why it
generates bad
On 2010/02/11 01:26:32, rsc wrote:
> more conventional to comment out the call to regopt.
> 

fixed comment. more conventional location is in cc/ and harder to comment out

http://codereview.appspot.com/204064/diff/1006/17#newcode1154
src/cmd/5c/reg.c:1154: if(r < 2 || r >= REGTMP-2)
On 2010/02/11 01:26:32, rsc wrote:
> is the -2 needed?
> 

yep, that's the magic. comment added.

http://codereview.appspot.com/204064/diff/1006/17#newcode1162
src/cmd/5c/reg.c:1162: b &= 0x01fcL;
On 2010/02/11 01:26:32, rsc wrote:
> add comment  // exclude R9 and R10 for m and g
> 

Done.

[kaib, 2010-02-11 06:50:25]

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

stop the 5c optimizer from clobbering extern static registers.

R=rsc, ken2
CC=golang-dev
http://codereview.appspot.com/204064