cmd/compile: don't mutate shared nodes in orderinit

A few gc.Node ops may be shared across functions.
The compiler is (mostly) already careful to avoid mutating them.
However, from a concurrency perspective, replacing (say)
an empty list with an empty list still counts as a mutation.
One place this occurs is orderinit. Avoid it.

This requires fixing one spot where shared nodes were mutated.
It doesn't result in any functional or performance changes.

Passes toolstash-check.

Updates #15756

Change-Id: I63c93b31baeeac62d7574804acb6b7f2bc9d14a9
Reviewed-on: https://go-review.googlesource.com/39196
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

diff --git a/src/cmd/compile/internal/gc/order.go b/src/cmd/compile/internal/gc/order.go
index 940cf1b4fb94e9ea30beb1bc38be6aaef488866b..c4c7a9d76517708c61bd49b7ef95734f355f5045 100644 (file)
--- a/src/cmd/compile/internal/gc/order.go
+++ b/src/cmd/compile/internal/gc/order.go
@@ -314,6 +314,14 @@ func orderstmtinplace(n *Node) *Node {
 
 // Orderinit moves n's init list to order->out.
 func orderinit(n *Node, order *Order) {
+       if n.mayBeShared() {
+               // For concurrency safety, don't mutate potentially shared nodes.
+               // First, ensure that no work is required here.
+               if n.Ninit.Len() > 0 {
+                       Fatalf("orderinit shared node with ninit")
+               }
+               return
+       }
        orderstmtlist(n.Ninit, order)
        n.Ninit.Set(nil)
 }
@@ -1107,7 +1115,7 @@ func orderexpr(n *Node, order *Order, lhs *Node) *Node {
                var s []*Node
 
                cleantempnopop(mark, order, &s)
-               n.Right.Ninit.Prepend(s...)
+               n.Right = addinit(n.Right, s)
                n.Right = orderexprinplace(n.Right, order)
 
        case OCALLFUNC,

diff --git a/src/cmd/compile/internal/gc/subr.go b/src/cmd/compile/internal/gc/subr.go
index 9bdecec5ce7573ad4315e6ea0e4aa70999727a4c..b358db2d0d5d021f37e4c3532fee1c6b591577b9 100644 (file)
--- a/src/cmd/compile/internal/gc/subr.go
+++ b/src/cmd/compile/internal/gc/subr.go
@@ -2043,11 +2043,8 @@ func addinit(n *Node, init []*Node) *Node {
        if len(init) == 0 {
                return n
        }
-
-       switch n.Op {
-       // There may be multiple refs to this node;
-       // introduce OCONVNOP to hold init list.
-       case ONAME, OLITERAL:
+       if n.mayBeShared() {
+               // Introduce OCONVNOP to hold init list.
                n = nod(OCONVNOP, n, nil)
                n.Type = n.Left.Type
                n.Typecheck = 1

diff --git a/src/cmd/compile/internal/gc/syntax.go b/src/cmd/compile/internal/gc/syntax.go
index 3bc3baee8a4b08bfe03d0d12659c257e7c07da81..c009c0ce65682684057a5701af4a62a4df962d60 100644 (file)
--- a/src/cmd/compile/internal/gc/syntax.go
+++ b/src/cmd/compile/internal/gc/syntax.go
@@ -14,7 +14,7 @@ import (
 // A Node is a single node in the syntax tree.
 // Actually the syntax tree is a syntax DAG, because there is only one
 // node with Op=ONAME for a given instance of a variable x.
-// The same is true for Op=OTYPE and Op=OLITERAL.
+// The same is true for Op=OTYPE and Op=OLITERAL. See Node.mayBeShared.
 type Node struct {
        // Tree structure.
        // Generic recursive walks should follow these fields.
@@ -179,6 +179,16 @@ func (n *Node) SetIota(x int64) {
        n.Xoffset = x
 }
 
+// mayBeShared reports whether n may occur in multiple places in the AST.
+// Extra care must be taken when mutating such a node.
+func (n *Node) mayBeShared() bool {
+       switch n.Op {
+       case ONAME, OLITERAL, OTYPE:
+               return true
+       }
+       return false
+}
+
 // Name holds Node fields used only by named nodes (ONAME, OTYPE, OPACK, OLABEL, some OLITERAL).
 type Name struct {
        Pack      *Node  // real package for import . names

diff --git a/src/cmd/compile/internal/gc/walk.go b/src/cmd/compile/internal/gc/walk.go
index 2fb14caba171c79936be6574ac69ceace80dda3b..214844f55b1fa02248aec6e9f5b7462c3b164644 100644 (file)
--- a/src/cmd/compile/internal/gc/walk.go
+++ b/src/cmd/compile/internal/gc/walk.go
@@ -500,6 +500,7 @@ opswitch:
 
        case OTYPE, ONAME, OLITERAL:
                // TODO(mdempsky): Just return n; see discussion on CL 38655.
+               // Perhaps refactor to use Node.mayBeShared for these instead.
 
        case ONOT, OMINUS, OPLUS, OCOM, OREAL, OIMAG, ODOTMETH, ODOTINTER,
                OIND, OSPTR, OITAB, OIDATA, OADDR: