Code review - Issue 7040044: code review 7040044: cmd/*g: Flush return parameters in case of panic.https://codereview.appspot.com/2013-01-04T16:21:27+00:00rietveld
Message from unknown
2013-01-01T16:31:56+00:00DMorsingurn:md5:05952b9127606d82edba82d232fae3ac
Message from unknown
2013-01-01T16:32:00+00:00DMorsingurn:md5:71eb3f8f05a303beabcf4c1a5f9ee778
Message from unknown
2013-01-01T16:36:19+00:00DMorsingurn:md5:c08506f1eb38344434731cd09e70bd88
Message from daniel.morsing@gmail.com
2013-01-01T16:36:25+00:00DMorsingurn:md5:e43883916f48c345a52a64cbd52f0253
Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://code.google.com/p/go/
Message from minux.ma@gmail.com
2013-01-01T16:44:11+00:00minux1urn:md5:106fc961467375c5b274c166e3e47049
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?
Message from daniel.morsing@gmail.com
2013-01-01T16:52:35+00:00DMorsingurn:md5:8abaf1af79e7b5d4a953b2e36774000b
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.
Message from rsc@golang.org
2013-01-02T20:08:26+00:00rscurn:md5:7fa71b41d78704793070a4adc38b344c
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.
Message from unknown
2013-01-02T23:26:13+00:00DMorsingurn:md5:a2f598604d6d9622d839370d92724a48
Message from daniel.morsing@gmail.com
2013-01-02T23:32:44+00:00DMorsingurn:md5:61bb00d4e71dcd923c6c5f57ad15d50d
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.
Message from rsc@golang.org
2013-01-04T15:28:44+00:00rscurn:md5:cf69d659090071b3f4c9523a01df5e1e
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.
Message from unknown
2013-01-04T15:57:53+00:00DMorsingurn:md5:9477211b5935977f1c6d7bfd387ebc33
Message from unknown
2013-01-04T16:07:14+00:00DMorsingurn:md5:2ba7d5dc71dd6383b5997e7a0cac3fc5
Message from daniel.morsing@gmail.com
2013-01-04T16:07:50+00:00DMorsingurn:md5:e8776fc1d50f377473a3d3b24a614a02
*** 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
Message from daniel.morsing@gmail.com
2013-01-04T16:21:27+00:00DMorsingurn:md5:820e54ea0dcb427c057faf09c643b9e4
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 :)