code review 7040044: cmd/*g: Flush return parameters in case of panic.

cmd/5g, cmd/6g, cmd/8g: flush return parameters in case of panic.

Fixes issue 4066.

[DMorsing, 2013-01-01 16:36:25]

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

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

[minux1, 2013-01-01 16:44:11]

please add a test for it.

so we keep working around the real issue instead of adopting the
other solution russ commented in issue 1304 or finding another that
doesn't affect performance?

[DMorsing, 2013-01-01 16:52:35]

Before and afters for the test in issue 4066:

before:
--- prog list "foo" ---
0028 (/home/daniel/src/test/test.go:13) TEXT    foo+0(SB),$40-8
0029 (/home/daniel/src/test/test.go:13) MOVQ    $0,DX
0030 (/home/daniel/src/test/test.go:14) MOVQ    $0,val+0(FP)
0031 (/home/daniel/src/test/test.go:17) PUSHQ   $func·001+0(SB),
0032 (/home/daniel/src/test/test.go:17) PUSHQ   $0,
0033 (/home/daniel/src/test/test.go:17) CALL    ,runtime.deferproc+0(SB)
0034 (/home/daniel/src/test/test.go:17) POPQ    ,CX
0035 (/home/daniel/src/test/test.go:17) POPQ    ,CX
0036 (/home/daniel/src/test/test.go:17) TESTQ   AX,AX
0037 (/home/daniel/src/test/test.go:17) JNE     $0,40
0038 (/home/daniel/src/test/test.go:24) CALL    ,foo1+0(SB)
0039 (/home/daniel/src/test/test.go:18) JMP     ,38
0040 (/home/daniel/src/test/test.go:27) CALL    ,runtime.deferreturn+0(SB)
0041 (/home/daniel/src/test/test.go:27) RET     ,

After:
--- prog list "foo" ---
0028 (/home/daniel/src/test/test.go:13) TEXT    foo+0(SB),$40-8
0029 (/home/daniel/src/test/test.go:13) MOVQ    $0,DX
0030 (/home/daniel/src/test/test.go:14) MOVQ    $0,val+0(FP)
0031 (/home/daniel/src/test/test.go:17) PUSHQ   $func·001+0(SB),
0032 (/home/daniel/src/test/test.go:17) PUSHQ   $0,
0033 (/home/daniel/src/test/test.go:17) CALL    ,runtime.deferproc+0(SB)
0034 (/home/daniel/src/test/test.go:17) POPQ    ,CX
0035 (/home/daniel/src/test/test.go:17) POPQ    ,CX
0036 (/home/daniel/src/test/test.go:17) TESTQ   AX,AX
0037 (/home/daniel/src/test/test.go:17) JNE     $0,41
0038 (/home/daniel/src/test/test.go:19) MOVQ    $2,val+0(FP)
0039 (/home/daniel/src/test/test.go:24) CALL    ,foo1+0(SB)
0040 (/home/daniel/src/test/test.go:18) JMP     ,38
0041 (/home/daniel/src/test/test.go:27) CALL    ,runtime.deferreturn+0(SB)
0042 (/home/daniel/src/test/test.go:27) RET     ,

Long term, This fix should be replaced by one that looks up if an instruction or
address might panic instead of this sledgehammer. Ideally something only spills
the last time before a panicking instruction.

[rsc, 2013-01-02 20:08:26]

I don't mind this as the long-term fix. It's fairly cheap.

https://codereview.appspot.com/7040044/diff/5001/src/cmd/5g/reg.c
File src/cmd/5g/reg.c (right):

https://codereview.appspot.com/7040044/diff/5001/src/cmd/5g/reg.c#newcode1078
src/cmd/5g/reg.c:1078: // flush modified globals and return parameters before
each instruction.
You should be able to avoid this unless the function body contains a defer. That
is already known.

[DMorsing, 2013-01-02 23:32:44]

PTAL

Added a test.

https://codereview.appspot.com/7040044/diff/5001/src/cmd/5g/reg.c
File src/cmd/5g/reg.c (right):

https://codereview.appspot.com/7040044/diff/5001/src/cmd/5g/reg.c#newcode1078
src/cmd/5g/reg.c:1078: // flush modified globals and return parameters before
each instruction.
On 2013/01/02 20:08:27, rsc wrote:
> You should be able to avoid this unless the function body contains a defer.
That
> is already known.

Done.

[rsc, 2013-01-04 15:28:44]

LGTM

Please make the CL description say cmd/5g, cmd/6g, cmd/8g.
The shorthands like *g or {5,6,8}g are more difficult to find with
hg log -k.

Also s/Flush/flush/.

FWIW, the bug is not that they are not spilled 'eagerly enough', it's that
they're
not flushed back at all in the case where the value appears to be dead.

[DMorsing, 2013-01-04 16:07:50]

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

cmd/5g, cmd/6g, cmd/8g: flush return parameters in case of panic.

Fixes issue 4066.

R=rsc, minux.ma
CC=golang-dev
https://codereview.appspot.com/7040044

[DMorsing, 2013-01-04 16:21:27]

On 2013/01/04 15:28:44, rsc wrote:
> FWIW, the bug is not that they are not spilled 'eagerly enough', it's that
> they're
> not flushed back at all in the case where the value appears to be dead.

I think eagerly works in this case. The bug would also trigger on:

func foo() (n int) {
    defer func() { _ = recover() }()
    n = 10
    panicingfunc()
    n = 20
}

However we describe it, the important part is that the bug is gone :)