Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1973)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 11 months ago by DMorsing
Modified:
9 years, 11 months ago
Reviewers:
CC:
rsc, minux1, golang-dev
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : diff -r d8ad9cc047e1 https://code.google.com/p/go/ #

Patch Set 3 : diff -r d8ad9cc047e1 https://code.google.com/p/go/ #

Total comments: 2

Patch Set 4 : diff -r 84a2c61092a9 https://code.google.com/p/go/ #

Patch Set 5 : diff -r 84a2c61092a9 https://code.google.com/p/go/ #

Patch Set 6 : diff -r bcc567c00842 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+52 lines, -3 lines) Patch
M src/cmd/5g/reg.c View 1 2 3 4 5 1 chunk +5 lines, -1 line 0 comments Download
M src/cmd/6g/reg.c View 1 2 3 1 chunk +5 lines, -1 line 0 comments Download
M src/cmd/8g/reg.c View 1 2 3 1 chunk +5 lines, -1 line 0 comments Download
A test/fixedbugs/issue4066.go View 1 2 3 4 1 chunk +37 lines, -0 lines 0 comments Download

Messages

Total messages: 8
DMorsing
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
9 years, 11 months ago (2013-01-01 16:36:25 UTC) #1
minux1
please add a test for it. so we keep working around the real issue instead ...
9 years, 11 months ago (2013-01-01 16:44:11 UTC) #2
DMorsing
Before and afters for the test in issue 4066: before: --- prog list "foo" --- ...
9 years, 11 months ago (2013-01-01 16:52:35 UTC) #3
rsc
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): ...
9 years, 11 months ago (2013-01-02 20:08:26 UTC) #4
DMorsing
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 ...
9 years, 11 months ago (2013-01-02 23:32:44 UTC) #5
rsc
LGTM Please make the CL description say cmd/5g, cmd/6g, cmd/8g. The shorthands like *g or ...
9 years, 11 months ago (2013-01-04 15:28:44 UTC) #6
DMorsing
*** 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. ...
9 years, 11 months ago (2013-01-04 16:07:50 UTC) #7
DMorsing
9 years, 11 months ago (2013-01-04 16:21:27 UTC) #8
Message was sent while issue was closed.
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 :)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b