code review 56910045: cmd/6g, cmd/8g, cmd/5g: make the undefined instruction ...

cmd/6g, cmd/8g, cmd/5g: make the undefined instruction have no successors

The UNDEF instruction was listed in the instruction data as having the next instruction in the stream as its successor. This confused the optimizer into adding a load where it wasn't needed, in turn confusing the liveness analysis pass for GC bitmaps into thinking that the variable was live.

Fixes issue 7229.

[DMorsing, 2014-02-03 21:14:40]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2014-02-03 21:21:08]

Any test possible?

Something like khr's broken demos?
 On Feb 3, 2014 4:14 PM, <daniel.morsing@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/6g, cmd/8g, cmd/5g: make the undefined instruction have no
> successors
>
> The UNDEF instruction was listed in the instruction data as having the
> next instruction in the stream as its successor. This confused the
> optimizer into adding a load where it wasn't needed, in turn confusing
> the liveness analysis pass for GC bitmaps into thinking that the
> variable was live.
>
> Fixes issue 7229.
>
> Please review this at https://codereview.appspot.com/56910045/
>
> Affected files (+3, -3 lines):
>   M src/cmd/5g/prog.c
>   M src/cmd/6g/prog.c
>   M src/cmd/8g/prog.c
>
>
> Index: src/cmd/5g/prog.c
> ===================================================================
> --- a/src/cmd/5g/prog.c
> +++ b/src/cmd/5g/prog.c
> @@ -26,7 +26,7 @@
>         [ATEXT]=        {Pseudo},
>         [AFUNCDATA]=    {Pseudo},
>         [APCDATA]=      {Pseudo},
> -       [AUNDEF]=       {OK},
> +       [AUNDEF]=       {Break},
>         [AUSEFIELD]=    {OK},
>         [ACHECKNIL]=    {LeftRead},
>         [AFATVARDEF]=   {Pseudo | RightWrite},
> Index: src/cmd/6g/prog.c
> ===================================================================
> --- a/src/cmd/6g/prog.c
> +++ b/src/cmd/6g/prog.c
> @@ -38,7 +38,7 @@
>         [ATEXT]=        {Pseudo},
>         [AFUNCDATA]=    {Pseudo},
>         [APCDATA]=      {Pseudo},
> -       [AUNDEF]=       {OK},
> +       [AUNDEF]=       {Break},
>         [AUSEFIELD]=    {OK},
>         [ACHECKNIL]=    {LeftRead},
>         [AFATVARDEF]=   {Pseudo | RightWrite},
> Index: src/cmd/8g/prog.c
> ===================================================================
> --- a/src/cmd/8g/prog.c
> +++ b/src/cmd/8g/prog.c
> @@ -38,7 +38,7 @@
>         [ATEXT]=        {Pseudo},
>         [AFUNCDATA]=    {Pseudo},
>         [APCDATA]=      {Pseudo},
> -       [AUNDEF]=       {OK},
> +       [AUNDEF]=       {Break},
>         [AUSEFIELD]=    {OK},
>         [ACHECKNIL]=    {LeftRead},
>         [AFATVARDEF]=   {Pseudo | RightWrite},
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[DMorsing, 2014-02-04 21:09:33]

On Mon, Feb 3, 2014 at 9:21 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Any test possible?
>
> Something like khr's broken demos?
>

Not sure how you could. Tests pretty much require parsing the output
of 6g -R -v, which you can't do with errorcheck.

[iant, 2014-02-04 21:15:46]

LGTM

[bradfitz, 2014-02-04 21:25:50]

Right. I had misremembered khr's example.
 On Feb 4, 2014 1:09 PM, "Daniel Morsing" <daniel.morsing@gmail.com> wrote:

> On Mon, Feb 3, 2014 at 9:21 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > Any test possible?
> >
> > Something like khr's broken demos?
> >
>
> Not sure how you could. Tests pretty much require parsing the output
> of 6g -R -v, which you can't do with errorcheck.
>

[dave_cheney.net, 2014-02-05 03:09:13]

This breaks arm :(

cmd/go
panic: runtime error: index out of range

goroutine 16 [running]:
runtime.panic(0x1ec420, 0x467b5b)
	/home/dfc/go/src/pkg/runtime/panic.c:250 +0x138
math.init·1()
	/home/dfc/go/src/pkg/math/pow10.go:38 +0x94
math.init()
	/home/dfc/go/src/pkg/math/unsafe.go:21 +0x60
strconv.init()
	/home/dfc/go/src/pkg/strconv/quote.go:442 +0x60
go/parser.init()
	/home/dfc/go/src/pkg/go/parser/parser.go:2441 +0x64
main.init()
	/home/dfc/go/src/cmd/go/zdefaultcc.go:6 +0x60

I'm sure your fix is correct, but it has exposed another issue.

[DMorsing, 2014-02-05 08:46:00]

Well that's a bummer.

Can you send me the output of "go build -gcflags '-R -v -S' math". The
instruction offsets don't seem to match up with your panic when I
compile on this machine.

On Wed, Feb 5, 2014 at 3:09 AM,  <dave@cheney.net> wrote:
> This breaks arm :(
>
> cmd/go
> panic: runtime error: index out of range
>
> goroutine 16 [running]:
> runtime.panic(0x1ec420, 0x467b5b)
>         /home/dfc/go/src/pkg/runtime/panic.c:250 +0x138
> math.init·1()
>         /home/dfc/go/src/pkg/math/pow10.go:38 +0x94
> math.init()
>         /home/dfc/go/src/pkg/math/unsafe.go:21 +0x60
> strconv.init()
>         /home/dfc/go/src/pkg/strconv/quote.go:442 +0x60
> go/parser.init()
>         /home/dfc/go/src/pkg/go/parser/parser.go:2441 +0x64
> main.init()
>         /home/dfc/go/src/cmd/go/zdefaultcc.go:6 +0x60
>
> I'm sure your fix is correct, but it has exposed another issue.
>
> https://codereview.appspot.com/56910045/

[DMorsing, 2014-02-09 17:31:01]

Took a look at the arm assembly, this is the diff on 5g -S before and after my
change:
 (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) ADD    	R4<<3,R0
 (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) MOVD   	0(R0),F1
 (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) SUB    	R4,R2,R3
 (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) MOVW  
	$"".pow10tab+0(SB),R1
-(/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) MOVW   	R3,R0
 (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) CMP    	$70,R0,
 (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) BLO    	,18580(APC)

Looks like this only happens when the peephole optimizer is turned on, but
passing -P to 5g causes it to crash.

[dave_cheney.net, 2014-02-10 01:25:21]

Oh poo. https://code.google.com/p/go/issues/detail?id=7294


On Mon, Feb 10, 2014 at 4:31 AM, <daniel.morsing@gmail.com> wrote:

> Took a look at the arm assembly, this is the diff on 5g -S before and
> after my change:
>  (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) ADD        R4<<3,R0
>  (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) MOVD       0(R0),F1
>  (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) SUB        R4,R2,R3
>  (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) MOVW
>         $"".pow10tab+0(SB),R1
> -(/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) MOVW       R3,R0
>  (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) CMP        $70,R0,
>  (/home/dmorsing/src/gotip/src/pkg/math/pow10.go:38) BLO
>  ,18580(APC)
>
> Looks like this only happens when the peephole optimizer is turned on,
> but passing -P to 5g causes it to crash.
>
> https://codereview.appspot.com/56910045/
>

[rsc, 2014-02-10 14:42:57]

LGTM

If it breaks arm, that means arm is broken already and the tests just aren't
catching it. Break it.

[DMorsing, 2014-02-11 20:20:35]

On Mon, Feb 10, 2014 at 2:42 PM,  <rsc@golang.org> wrote:
> LGTM
>
> If it breaks arm, that means arm is broken already and the tests just
> aren't catching it. Break it.
>

OK. arm going down.

[DMorsing, 2014-02-11 20:25:46]

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

cmd/6g, cmd/8g, cmd/5g: make the undefined instruction have no successors

The UNDEF instruction was listed in the instruction data as having the next
instruction in the stream as its successor. This confused the optimizer into
adding a load where it wasn't needed, in turn confusing the liveness analysis
pass for GC bitmaps into thinking that the variable was live.

Fixes issue 7229.

LGTM=iant, rsc
R=golang-codereviews, bradfitz, iant, dave, rsc
CC=golang-codereviews
https://codereview.appspot.com/56910045

[gobot, 2014-02-11 20:29:24]

This CL appears to have broken the darwin-amd64 builder.