cmd/compile/internal/gc: fix Node.copy and introduce (raw|sep)copy
Node.copy used to make a shallow copy of a node. Often, this is not
correct: If a node n's Orig field pointed to itself, the copy's Orig
field has to be adjusted to point to the copy. Otherwise, if n is
modified later, the copy's Orig appears modified as well (because it
points to n).
This was fixed for one specific case with
https://go-review.googlesource.com/c/go/+/136395 (issue #26855).
This change instead addresses copy in general:
In two cases we don't want the Orig adjustment as it causes escape
analysis output to fail (not match the existing error messages).
rawcopy is used in those cases.
In several cases Orig is set to the copy immediately after making
a copy; a new function sepcopy is used there.
Updates #26855.
Fixes #27765.
Change-Id: Idaadeb5c4b9a027daabd46a2361348f7a93f2b00
Reviewed-on: https://go-review.googlesource.com/136540
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
diff --git a/src/cmd/compile/internal/gc/const.go b/src/cmd/compile/internal/gc/const.go
index 3466472a..02d5167 100644
--- a/src/cmd/compile/internal/gc/const.go
+++ b/src/cmd/compile/internal/gc/const.go
@@ -234,7 +234,7 @@
if n.Op == OLITERAL && !reuse {
// Can't always set n.Type directly on OLITERAL nodes.
// See discussion on CL 20813.
- n = n.copy()
+ n = n.rawcopy()
reuse = true
}
@@ -1200,8 +1200,7 @@
// Ensure n.Orig still points to a semantically-equivalent
// expression after we rewrite n into a constant.
if n.Orig == n {
- n.Orig = n.copy()
- n.Orig.Orig = n.Orig
+ n.Orig = n.sepcopy()
}
*n = Node{
@@ -1331,7 +1330,7 @@
}
if n.Op == OLITERAL && !reuse {
- n = n.copy()
+ n = n.rawcopy()
reuse = true
}
diff --git a/src/cmd/compile/internal/gc/order.go b/src/cmd/compile/internal/gc/order.go
index dce68a6..1e22ecf 100644
--- a/src/cmd/compile/internal/gc/order.go
+++ b/src/cmd/compile/internal/gc/order.go
@@ -109,8 +109,7 @@
if l == n.Left {
return n
}
- a := n.copy()
- a.Orig = a
+ a := n.sepcopy()
a.Left = l
return typecheck(a, Erv)
}
@@ -135,8 +134,7 @@
if l == n.Left {
return n
}
- a := n.copy()
- a.Orig = a
+ a := n.sepcopy()
a.Left = l
return typecheck(a, Erv)
@@ -145,8 +143,7 @@
if l == n.Left {
return n
}
- a := n.copy()
- a.Orig = a
+ a := n.sepcopy()
a.Left = l
return typecheck(a, Erv)
@@ -161,8 +158,7 @@
if l == n.Left && r == n.Right {
return n
}
- a := n.copy()
- a.Orig = a
+ a := n.sepcopy()
a.Left = l
a.Right = r
return typecheck(a, Erv)
diff --git a/src/cmd/compile/internal/gc/sinit.go b/src/cmd/compile/internal/gc/sinit.go
index c6455c3..f76b028 100644
--- a/src/cmd/compile/internal/gc/sinit.go
+++ b/src/cmd/compile/internal/gc/sinit.go
@@ -349,15 +349,13 @@
gdata(n, e.Expr, int(n.Type.Width))
continue
}
- ll := n.copy()
- ll.Orig = ll // completely separate copy
+ ll := n.sepcopy()
if staticassign(ll, e.Expr, out) {
continue
}
// Requires computation, but we're
// copying someone else's computation.
- rr := orig.copy()
- rr.Orig = rr // completely separate copy
+ rr := orig.sepcopy()
rr.Type = ll.Type
rr.Xoffset += e.Xoffset
setlineno(rr)
@@ -453,8 +451,7 @@
continue
}
setlineno(e.Expr)
- a := n.copy()
- a.Orig = a // completely separate copy
+ a := n.sepcopy()
if !staticassign(a, e.Expr, out) {
*out = append(*out, nod(OAS, a, e.Expr))
}
@@ -518,8 +515,7 @@
// Copy val directly into n.
n.Type = val.Type
setlineno(val)
- a := n.copy()
- a.Orig = a
+ a := n.sepcopy()
if !staticassign(a, val, out) {
*out = append(*out, nod(OAS, a, val))
}
diff --git a/src/cmd/compile/internal/gc/subr.go b/src/cmd/compile/internal/gc/subr.go
index 61a3b23..7e450e2 100644
--- a/src/cmd/compile/internal/gc/subr.go
+++ b/src/cmd/compile/internal/gc/subr.go
@@ -364,9 +364,35 @@
return n
}
+// rawcopy returns a shallow copy of n.
+// Note: copy or sepcopy (rather than rawcopy) is usually the
+// correct choice (see comment with Node.copy, below).
+func (n *Node) rawcopy() *Node {
+ copy := *n
+ return ©
+}
+
+// sepcopy returns a separate shallow copy of n, with the copy's
+// Orig pointing to itself.
+func (n *Node) sepcopy() *Node {
+ copy := *n
+ copy.Orig = ©
+ return ©
+}
+
+// copy returns shallow copy of n and adjusts the copy's Orig if
+// necessary: In general, if n.Orig points to itself, the copy's
+// Orig should point to itself as well. Otherwise, if n is modified,
+// the copy's Orig node appears modified, too, and then doesn't
+// represent the original node anymore.
+// (This caused the wrong complit Op to be used when printing error
+// messages; see issues #26855, #27765).
func (n *Node) copy() *Node {
- n2 := *n
- return &n2
+ copy := *n
+ if n.Orig == n {
+ copy.Orig = ©
+ }
+ return ©
}
// methcmp sorts methods by symbol.
@@ -412,8 +438,7 @@
switch n.Op {
default:
- m := n.copy()
- m.Orig = m
+ m := n.sepcopy()
m.Left = treecopy(n.Left, pos)
m.Right = treecopy(n.Right, pos)
m.List.Set(listtreecopy(n.List.Slice(), pos))
diff --git a/src/cmd/compile/internal/gc/typecheck.go b/src/cmd/compile/internal/gc/typecheck.go
index 6b4673d..69dced0 100644
--- a/src/cmd/compile/internal/gc/typecheck.go
+++ b/src/cmd/compile/internal/gc/typecheck.go
@@ -2923,14 +2923,6 @@
// Save original node (including n.Right)
norig := n.copy()
- // If n.Orig points to itself, norig.Orig must point to itself, too.
- // Otherwise, because n.Op is changed below, n.Orig's Op is changed
- // as well because it (and the copy norig) still point to the original
- // node n. This caused the wrong complit Op to be used when printing
- // error messages (issue #26855).
- if n.Orig == n {
- norig.Orig = norig
- }
setlineno(n.Right)
n.Right = typecheck(n.Right, Etype|Ecomplit)
diff --git a/src/cmd/compile/internal/gc/walk.go b/src/cmd/compile/internal/gc/walk.go
index 0b382bb..1b1d36b 100644
--- a/src/cmd/compile/internal/gc/walk.go
+++ b/src/cmd/compile/internal/gc/walk.go
@@ -4052,7 +4052,7 @@
// The result of substArgTypes MUST be assigned back to old, e.g.
// n.Left = substArgTypes(n.Left, t1, t2)
func substArgTypes(old *Node, types_ ...*types.Type) *Node {
- n := old.copy() // make shallow copy
+ n := old.copy()
for _, t := range types_ {
dowidth(t)
