cmd/gc: correct liveness for wrappers containing tail jumps

A normal RET is treated as using the return values,
but a tail jump RET does not - it is jumping to the
function that is going to fill in the return values.
If a tail jump RET is recorded as using the return values,
since nothing initializes them they will be marked as
live on entry to the function, which is clearly wrong.

Found and tested by the new code in plive.c that looks
for variables that are incorrectly live on entry.
That code is disabled for now because there are other
cases remaining to be fixed. But once it is enabled,
test/live1.go becomes a real test of this CL.

TBR=iant
CC=golang-codereviews
https://golang.org/cl/63570045

diff --git a/src/cmd/gc/plive.c b/src/cmd/gc/plive.c
index de868a672a92a4451de9f39cdd46a42ad97865b3..20aeb51557b32b3e7fd9a5a58a4c4ce997fa308e 100644 (file)
--- a/src/cmd/gc/plive.c
+++ b/src/cmd/gc/plive.c
@@ -664,18 +664,29 @@ progeffects(Prog *prog, Array *vars, Bvec *uevar, Bvec *varkill, Bvec *avarinit)
                // Return instructions implicitly read all the arguments.  For
                // the sake of correctness, out arguments must be read.  For the
                // sake of backtrace quality, we read in arguments as well.
+               //
+               // A return instruction with a p->to is a tail return, which brings
+               // the stack pointer back up (if it ever went down) and then jumps
+               // to a new function entirely. That form of instruction must read
+               // all the parameters for correctness, and similarly it must not
+               // read the out arguments - they won't be set until the new
+               // function runs.
                for(i = 0; i < arraylength(vars); i++) {
                        node = *(Node**)arrayget(vars, i);
                        switch(node->class & ~PHEAP) {
                        case PPARAM:
                                bvset(uevar, i);
+                               break;
                        case PPARAMOUT:
                                // If the result had its address taken, it is being tracked
                                // by the avarinit code, which does not use uevar.
                                // If we added it to uevar too, we'd not see any kill
                                // and decide that the varible was live entry, which it is not.
                                // So only use uevar in the non-addrtaken case.
-                               if(!node->addrtaken)
+                               // The p->to.type == D_NONE limits the bvset to
+                               // non-tail-call return instructions; see note above
+                               // the for loop for details.
+                               if(!node->addrtaken && prog->to.type == D_NONE)
                                        bvset(uevar, i);
                                break;
                        }
@@ -1596,6 +1607,19 @@ livenessepilogue(Liveness *lv)
                        if(issafepoint(p)) {
                                // Found an interesting instruction, record the
                                // corresponding liveness information.  
+                               
+                               // Useful sanity check: on entry to the function,
+                               // the only things that can possibly be live are the
+                               // input parameters.
+                               if(0 && p->as == ATEXT) {
+                                       for(j = 0; j < liveout->n; j++) {
+                                               if(!bvget(liveout, j))
+                                                       continue;
+                                               n = *(Node**)arrayget(lv->vars, j);
+                                               if(n->class != PPARAM)
+                                                       yyerrorl(p->lineno, "internal error: %N %N recorded as live on entry", curfn->nname, n);
+                                       }
+                               }
 
                                // Record live pointers.
                                args = *(Bvec**)arrayget(lv->argslivepointers, pos);

diff --git a/test/live1.go b/test/live1.go
new file mode 100644 (file)
index 0000000..d0a2d0e
--- /dev/null
+++ b/test/live1.go
@@ -0,0 +1,30 @@
+// compile
+
+// Copyright 2014 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Test that code compiles without
+// "internal error: ... recorded as live on entry" errors
+// from the liveness code.
+
+package main
+
+// The liveness analysis used to get confused by the tail return
+// instruction in the wrapper methods generated for T1.M and (*T1).M,
+// causing a spurious "live at entry: ~r1" for the return result.
+// This test is checking that there is no such message.
+// We cannot use live.go because it runs with -live on, which will
+// generate (correct) messages about the wrapper's receivers
+// being live on entry, but those messages correspond to no
+// source line in the file, so they are given at line 1, which we
+// cannot annotate. Not using -live here avoids that problem.
+
+type T struct {
+}
+
+func (t *T) M() *int
+
+type T1 struct {
+       *T
+}