cmd/compile: fix code duplication in race-instrumentation

instrumentnode() accidentally copies parent's already-instrumented nodes
into child's Ninit block. This generates repeated code in race-instrumentation.
This case surfaces only when it duplicates inline-labels, because of
compile time error. In other cases, it silently generates incorrect
instrumented code. This change prevents it from doing so.

Fixes #17449.

Change-Id: Icddf2198990442166307e176b7e20aa0cf6c171c
Reviewed-on: https://go-review.googlesource.com/31317
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/cmd/compile/internal/gc/racewalk.go b/src/cmd/compile/internal/gc/racewalk.go
index 8f57ef33fe81d056bc5bf4a7fe3341c69b930a97..c8ab6038aa9372341e67b655687b59b5da484440 100644 (file)
--- a/src/cmd/compile/internal/gc/racewalk.go
+++ b/src/cmd/compile/internal/gc/racewalk.go
@@ -145,36 +145,21 @@ func instrumentnode(np **Node, init *Nodes, wr int, skip int) {
                goto ret
 
        case OBLOCK:
-               var out []*Node
                ls := n.List.Slice()
-               for i := 0; i < len(ls); i++ {
-                       switch ls[i].Op {
-                       case OCALLFUNC, OCALLMETH, OCALLINTER:
-                               instrumentnode(&ls[i], &ls[i].Ninit, 0, 0)
-                               out = append(out, ls[i])
-                               // Scan past OAS nodes copying results off stack.
-                               // Those must not be instrumented, because the
-                               // instrumentation calls will smash the results.
-                               // The assignments are to temporaries, so they cannot
-                               // be involved in races and need not be instrumented.
-                               for i+1 < len(ls) && ls[i+1].Op == OAS && iscallret(ls[i+1].Right) {
-                                       i++
-                                       out = append(out, ls[i])
-                               }
-                       default:
-                               var outn Nodes
-                               outn.Set(out)
-                               instrumentnode(&ls[i], &outn, 0, 0)
-                               if ls[i].Op != OAS && ls[i].Op != OASWB && ls[i].Op != OAS2FUNC || ls[i].Ninit.Len() == 0 {
-                                       out = append(outn.Slice(), ls[i])
-                               } else {
-                                       // Splice outn onto end of ls[i].Ninit
-                                       ls[i].Ninit.AppendNodes(&outn)
-                                       out = append(out, ls[i])
-                               }
+               afterCall := false
+               for i := range ls {
+                       op := ls[i].Op
+                       // Scan past OAS nodes copying results off stack.
+                       // Those must not be instrumented, because the
+                       // instrumentation calls will smash the results.
+                       // The assignments are to temporaries, so they cannot
+                       // be involved in races and need not be instrumented.
+                       if afterCall && op == OAS && iscallret(ls[i].Right) {
+                               continue
                        }
+                       instrumentnode(&ls[i], &ls[i].Ninit, 0, 0)
+                       afterCall = (op == OCALLFUNC || op == OCALLMETH || op == OCALLINTER)
                }
-               n.List.Set(out)
                goto ret
 
        case ODEFER:

diff --git a/test/fixedbugs/issue17449.go b/test/fixedbugs/issue17449.go
new file mode 100644 (file)
index 0000000..2302917
--- /dev/null
+++ b/test/fixedbugs/issue17449.go
@@ -0,0 +1,34 @@
+// errorcheck -0 -race
+
+// Copyright 2016 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.
+
+// Issue 17449: race instrumentation copies over previous instrumented nodes from parents block into child's Ninit block.
+// This code surfaces the duplication at compile time because of generated inline labels.
+
+package master
+
+type PriorityList struct {
+    elems []interface{}
+}
+
+func (x *PriorityList) Len() int { return len(x.elems) }
+
+func (l *PriorityList) remove(i int) interface{} {
+    elem := l.elems[i]
+    l.elems = append(l.elems[:i], l.elems[i+1:]...)
+    return elem
+}
+
+func (l *PriorityList) Next() interface{} {
+    return l.remove(l.Len() - 1)
+}
+
+var l *PriorityList
+
+func Foo() {
+    // It would fail here if instrumented code (including inline-label) was copied.
+    for elem := l.Next(); elem != nil; elem = l.Next() {
+    }
+}