Grokbase › Groups › Go › golang-dev › January 2013
FAQ
Reviewers: rsc,

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

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


Description:
cmd/*g: Flush return parameters in case of panic.

Fixes issue 4066.

Please review this at https://codereview.appspot.com/7040044/

Affected files:
M src/cmd/5g/reg.c
M src/cmd/6g/reg.c
M src/cmd/8g/reg.c


Index: src/cmd/5g/reg.c
===================================================================
--- a/src/cmd/5g/reg.c
+++ b/src/cmd/5g/reg.c
@@ -1074,10 +1074,10 @@
break;

default:
- // Work around for issue 1304:
- // flush modified globals before each instruction.
+ // Work around for issue 1304 & 4066:
+ // flush modified globals and return parameters before each instruction.
for(z=0; z<BITS; z++)
- cal.b[z] |= externs.b[z];
+ cal.b[z] |= externs.b[z] | ovar.b[z];
break;
}
for(z=0; z<BITS; z++) {
Index: src/cmd/6g/reg.c
===================================================================
--- a/src/cmd/6g/reg.c
+++ b/src/cmd/6g/reg.c
@@ -1119,10 +1119,10 @@
break;

default:
- // Work around for issue 1304:
- // flush modified globals before each instruction.
+ // Work around for issue 1304 & 4066:
+ // flush modified globals and return parameters before each instruction.
for(z=0; z<BITS; z++)
- cal.b[z] |= externs.b[z];
+ cal.b[z] |= externs.b[z] | ovar.b[z];
break;
}
for(z=0; z<BITS; z++) {
Index: src/cmd/8g/reg.c
===================================================================
--- a/src/cmd/8g/reg.c
+++ b/src/cmd/8g/reg.c
@@ -989,10 +989,10 @@
break;

default:
- // Work around for issue 1304:
- // flush modified globals before each instruction.
+ // Work around for issue 1304 & 4066:
+ // flush modified globals and return parameters before each instruction.
for(z=0; z<BITS; z++)
- cal.b[z] |= externs.b[z];
+ cal.b[z] |= externs.b[z] | ovar.b[z];
break;
}
for(z=0; z<BITS; z++) {

Search Discussions

  • Minux at Jan 1, 2013 at 4:44 pm ⇧
    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?
  • Daniel Morsing at Jan 1, 2013 at 4:52 pm ⇧
    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.



    https://codereview.appspot.com/7040044/
  • Rsc at Jan 2, 2013 at 8:08 pm ⇧
    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.

    https://codereview.appspot.com/7040044/
  • Daniel Morsing at Jan 2, 2013 at 11:32 pm ⇧
    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.

    https://codereview.appspot.com/7040044/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedJan 1, '13 at 4:36p
activeJan 2, '13 at 11:32p
posts5
users3
websitegolang.org

3 users in discussion

Daniel Morsing: 3 posts Rsc: 1 post Minux: 1 post

Content

People

Support

Translate

site design / logo © 2022 Grokbase