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

Issue 64170043: code review 64170043: cmd/gc: correct liveness for fat variables (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 1 month ago by rsc
Modified:
11 years, 1 month ago
Reviewers:
gobot, iant
CC:
golang-codereviews
Visibility:
Public.

Description

cmd/gc: correct liveness for fat variables The VARDEF placement must be before the initialization but after any final use. If you have something like s = ... using s ... the rhs must be evaluated, then the VARDEF, then the lhs assigned. There is a large comment in pgen.c on gvardef explaining this in more detail. This CL also includes Ian's suggestions from earlier CLs, namely commenting the use of mode in link.h and fixing the precedence of the ~r check in dcl.c. This CL enables the check that if liveness analysis decides a variable is live on entry to the function, that variable must be a function parameter (not a result, and not a local variable). If this check fails, it indicates a bug in the liveness analysis or in the generated code being analyzed. The race detector generates invalid code for append(x, y...). The code declares a temporary t and then uses cap(t) before initializing t. The new liveness check catches this bug and stops the compiler from writing out the buggy code. Consequently, this CL disables the race detector tests in run.bash until the race detector bug can be fixed (golang.org/issue/7334). Except for the race detector bug, the liveness analysis check does not detect any problems (this CL and the previous CLs fixed all the detected problems). The net test still fails with GOGC=0 but the rest of the tests now pass or time out (because GOGC=0 is so slow).

Patch Set 1 #

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

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

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+278 lines, -126 lines) Patch
M include/link.h View 1 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/5g/cgen.c View 1 2 3 6 chunks +11 lines, -4 lines 0 comments Download
M src/cmd/5g/ggen.c View 1 2 3 3 chunks +13 lines, -5 lines 0 comments Download
M src/cmd/5g/peep.c View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/5g/reg.c View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/6g/cgen.c View 1 2 3 7 chunks +14 lines, -4 lines 0 comments Download
M src/cmd/6g/ggen.c View 1 3 chunks +13 lines, -5 lines 0 comments Download
M src/cmd/6g/peep.c View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M src/cmd/6g/reg.c View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/8g/cgen.c View 1 2 3 6 chunks +11 lines, -4 lines 0 comments Download
M src/cmd/8g/ggen.c View 1 2 3 3 chunks +13 lines, -5 lines 0 comments Download
M src/cmd/8g/peep.c View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M src/cmd/8g/reg.c View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/gc/dcl.c View 1 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/gc/gen.c View 1 2 3 9 chunks +62 lines, -36 lines 0 comments Download
M src/cmd/gc/pgen.c View 1 2 1 chunk +54 lines, -0 lines 0 comments Download
M src/cmd/gc/plive.c View 1 4 chunks +33 lines, -54 lines 0 comments Download
M src/run.bash View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M test/live.go View 1 2 1 chunk +12 lines, -0 lines 0 comments Download
M test/live1.go View 1 2 chunks +22 lines, -6 lines 0 comments Download

Messages

Total messages: 4
rsc
Hello iant (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
11 years, 1 month ago (2014-02-15 15:58:52 UTC) #1
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=b0ecc1786896 *** cmd/gc: correct liveness for fat variables The VARDEF placement must ...
11 years, 1 month ago (2014-02-15 15:58:59 UTC) #2
gobot
This CL appears to have broken the windows-amd64 builder.
11 years, 1 month ago (2014-02-15 16:10:27 UTC) #3
iant
11 years, 1 month ago (2014-02-26 06:25:52 UTC) #4
Message was sent while issue was closed.
LGTM
Sign in to reply to this message.

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