[release-branch.go1.8] cmd/compile: zero ambiguously live variables at VARKILLs

This is a redo of CL 41076 backported to the 1.8 release branch.
There were major conflicts, so I had to basically rewrite it again
from scratch.  The way Progs are allocated changed.  Liveness analysis
and Prog generation got reordered.  Liveness analysis changed from
running on gc.BasicBlock to ssa.Block.  All that makes the logic quite
a bit different.

Please review carefully.

From CL 41076:

At VARKILLs, zero a variable if it is ambiguously live.
After the VARKILL anything this variable references
might be collected. If it were to become live again later,
the GC will see references to already-collected objects.

We don't know a variable is ambiguously live until very
late in compilation (after lowering, register allocation, ...),
so it is hard to generate the code in an arch-independent way.
We also have to be careful not to clobber any registers.
Fortunately, this almost never happens so performance is ~irrelevant.

There are only 2 instances where this triggers in the stdlib.

Fixes #20029

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

diff --git a/src/cmd/compile/internal/amd64/galign.go b/src/cmd/compile/internal/amd64/galign.go
index bb3830bca548ce2c15645d9971bbed5fe8862193..6fd7f316172d85318cb165b373f71ee91ba980bc 100644 (file)
--- a/src/cmd/compile/internal/amd64/galign.go
+++ b/src/cmd/compile/internal/amd64/galign.go
@@ -27,4 +27,5 @@ func Init() {
        gc.Thearch.SSAMarkMoves = ssaMarkMoves
        gc.Thearch.SSAGenValue = ssaGenValue
        gc.Thearch.SSAGenBlock = ssaGenBlock
+       gc.Thearch.ZeroAuto = zeroAuto
 }

diff --git a/src/cmd/compile/internal/amd64/ggen.go b/src/cmd/compile/internal/amd64/ggen.go
index c137b52d8042fa49f5a6f850b4f427809c42784a..d12153383e1d5984a9daece9ff77b45f71cb5550 100644 (file)
--- a/src/cmd/compile/internal/amd64/ggen.go
+++ b/src/cmd/compile/internal/amd64/ggen.go
@@ -166,6 +166,27 @@ func zerorange(p *obj.Prog, frame int64, lo int64, hi int64, ax *uint32, x0 *uin
        return p
 }
 
+func zeroAuto(n *gc.Node, pp *obj.Prog) {
+       // Note: this code must not clobber any registers.
+       op := x86.AMOVQ
+       if gc.Widthptr == 4 {
+               op = x86.AMOVL
+       }
+       sym := gc.Linksym(n.Sym)
+       size := n.Type.Size()
+       for i := int64(0); i < size; i += int64(gc.Widthptr) {
+               p := gc.AddAsmAfter(op, pp)
+               pp = p
+               p.From.Type = obj.TYPE_CONST
+               p.From.Offset = 0
+               p.To.Type = obj.TYPE_MEM
+               p.To.Name = obj.NAME_AUTO
+               p.To.Reg = x86.REG_SP
+               p.To.Offset = n.Xoffset + i
+               p.To.Sym = sym
+       }
+}
+
 func ginsnop() {
        // This is actually not the x86 NOP anymore,
        // but at the point where it gets used, AX is dead

diff --git a/src/cmd/compile/internal/arm/galign.go b/src/cmd/compile/internal/arm/galign.go
index 308b016026df5b15e419d73768c5ed2c4bf32edf..53f57cd978c1d3a3a3e0cbb31bb1f3c76f6ee6ef 100644 (file)
--- a/src/cmd/compile/internal/arm/galign.go
+++ b/src/cmd/compile/internal/arm/galign.go
@@ -21,4 +21,5 @@ func Init() {
        gc.Thearch.SSAMarkMoves = func(s *gc.SSAGenState, b *ssa.Block) {}
        gc.Thearch.SSAGenValue = ssaGenValue
        gc.Thearch.SSAGenBlock = ssaGenBlock
+       gc.Thearch.ZeroAuto = zeroAuto
 }

diff --git a/src/cmd/compile/internal/arm/ggen.go b/src/cmd/compile/internal/arm/ggen.go
index 6dce0a4e80678e0b9b6ea2de5847bea25c9ed045..1b97a4d0c95a50d619f3af584dabb3c260ed3af6 100644 (file)
--- a/src/cmd/compile/internal/arm/ggen.go
+++ b/src/cmd/compile/internal/arm/ggen.go
@@ -92,6 +92,27 @@ func zerorange(p *obj.Prog, frame int64, lo int64, hi int64, r0 *uint32) *obj.Pr
 
        return p
 }
+func zeroAuto(n *gc.Node, pp *obj.Prog) {
+       // Note: this code must not clobber any registers.
+       sym := gc.Linksym(n.Sym)
+       size := n.Type.Size()
+       p := gc.Prog(arm.AMOVW)
+       p.From.Type = obj.TYPE_CONST
+       p.From.Offset = 0
+       p.To.Type = obj.TYPE_REG
+       p.To.Reg = arm.REGTMP
+       for i := int64(0); i < size; i += 4 {
+               p := gc.AddAsmAfter(arm.AMOVW, pp)
+               pp = p
+               p.From.Type = obj.TYPE_REG
+               p.From.Reg = arm.REGTMP
+               p.To.Type = obj.TYPE_MEM
+               p.To.Name = obj.NAME_AUTO
+               p.To.Reg = arm.REGSP
+               p.To.Offset = n.Xoffset + i
+               p.To.Sym = sym
+       }
+}
 
 func ginsnop() {
        p := gc.Prog(arm.AAND)

diff --git a/src/cmd/compile/internal/arm64/galign.go b/src/cmd/compile/internal/arm64/galign.go
index 20a67e398d6924f4ed9ad7e13504116297aec477..15680dd16885296c8e7c1960031d25f3c29ecb45 100644 (file)
--- a/src/cmd/compile/internal/arm64/galign.go
+++ b/src/cmd/compile/internal/arm64/galign.go
@@ -21,4 +21,5 @@ func Init() {
        gc.Thearch.SSAMarkMoves = func(s *gc.SSAGenState, b *ssa.Block) {}
        gc.Thearch.SSAGenValue = ssaGenValue
        gc.Thearch.SSAGenBlock = ssaGenBlock
+       gc.Thearch.ZeroAuto = zeroAuto
 }

diff --git a/src/cmd/compile/internal/arm64/ggen.go b/src/cmd/compile/internal/arm64/ggen.go
index 16813b642ab4bcb5922ec529f03e39d10c0c29d4..83966a493342bee79df0b2dfea6b52381b1b6829 100644 (file)
--- a/src/cmd/compile/internal/arm64/ggen.go
+++ b/src/cmd/compile/internal/arm64/ggen.go
@@ -103,6 +103,23 @@ func zerorange(p *obj.Prog, frame int64, lo int64, hi int64) *obj.Prog {
        return p
 }
 
+func zeroAuto(n *gc.Node, pp *obj.Prog) {
+       // Note: this code must not clobber any registers.
+       sym := gc.Linksym(n.Sym)
+       size := n.Type.Size()
+       for i := int64(0); i < size; i += 8 {
+               p := gc.AddAsmAfter(arm64.AMOVD, pp)
+               pp = p
+               p.From.Type = obj.TYPE_REG
+               p.From.Reg = arm64.REGZERO
+               p.To.Type = obj.TYPE_MEM
+               p.To.Name = obj.NAME_AUTO
+               p.To.Reg = arm64.REGSP
+               p.To.Offset = n.Xoffset + i
+               p.To.Sym = sym
+       }
+}
+
 func ginsnop() {
        p := gc.Prog(arm64.AHINT)
        p.From.Type = obj.TYPE_CONST

diff --git a/src/cmd/compile/internal/gc/go.go b/src/cmd/compile/internal/gc/go.go
index ff33e9c1c4b1bc4069c98b3c4902837cf3d239cc..a529ca40f7eb6ef5ee81103e7970a11e90d6b41a 100644 (file)
--- a/src/cmd/compile/internal/gc/go.go
+++ b/src/cmd/compile/internal/gc/go.go
@@ -361,6 +361,12 @@ type Arch struct {
        // SSAGenBlock emits end-of-block Progs. SSAGenValue should be called
        // for all values in the block before SSAGenBlock.
        SSAGenBlock func(s *SSAGenState, b, next *ssa.Block)
+
+       // ZeroAuto emits code to zero the given auto stack variable.
+       // Code is added immediately after pp.
+       // ZeroAuto must not use any non-temporary registers.
+       // ZeroAuto will only be called for variables which contain a pointer.
+       ZeroAuto func(n *Node, pp *obj.Prog)
 }
 
 var pcloc int32

diff --git a/src/cmd/compile/internal/gc/gsubr.go b/src/cmd/compile/internal/gc/gsubr.go
index 1e8636347a6ef81e64cf1ddcc3f3d2d9c5d70bde..2a8bedff962786d1cd4149de9e02ee2145e481f1 100644 (file)
--- a/src/cmd/compile/internal/gc/gsubr.go
+++ b/src/cmd/compile/internal/gc/gsubr.go
@@ -72,6 +72,15 @@ func Appendpp(p *obj.Prog, as obj.As, ftype obj.AddrType, freg int16, foffset in
        return q
 }
 
+func AddAsmAfter(as obj.As, p *obj.Prog) *obj.Prog {
+       q := Ctxt.NewProg()
+       Clearp(q)
+       q.As = as
+       q.Link = p.Link
+       p.Link = q
+       return q
+}
+
 func ggloblnod(nam *Node) {
        s := Linksym(nam.Sym)
        s.Gotype = Linksym(ngotype(nam))

diff --git a/src/cmd/compile/internal/gc/pgen.go b/src/cmd/compile/internal/gc/pgen.go
index 643ba79d631036923b4228094c8fad1624f101dd..dde28f670befa9b7cffd1008c293a46d3e809bc2 100644 (file)
--- a/src/cmd/compile/internal/gc/pgen.go
+++ b/src/cmd/compile/internal/gc/pgen.go
@@ -120,7 +120,30 @@ func Gvarlive(n *Node) {
 }
 
 func removevardef(firstp *obj.Prog) {
+       // At VARKILLs, zero variable if it is ambiguously live.
+       // After the VARKILL anything this variable references
+       // might be collected. If it were to become live again later,
+       // the GC will see references to already-collected objects.
+       // See issue 20029.
        for p := firstp; p != nil; p = p.Link {
+               if p.As != obj.AVARKILL {
+                       continue
+               }
+               n := p.To.Node.(*Node)
+               if !n.Name.Needzero {
+                       continue
+               }
+               if n.Class != PAUTO {
+                       Fatalf("zero of variable which isn't PAUTO %v", n)
+               }
+               if n.Type.Size()%int64(Widthptr) != 0 {
+                       Fatalf("zero of variable not a multiple of ptr size %v", n)
+               }
+               Thearch.ZeroAuto(n, p)
+       }
+
+       for p := firstp; p != nil; p = p.Link {
+
                for p.Link != nil && (p.Link.As == obj.AVARDEF || p.Link.As == obj.AVARKILL || p.Link.As == obj.AVARLIVE) {
                        p.Link = p.Link.Link
                }

diff --git a/src/cmd/compile/internal/mips/galign.go b/src/cmd/compile/internal/mips/galign.go
index 39f5d2bf641516d92cb81a26cd77cb6af70680f1..e6d117a806d6f594999b14fe4612842aad14d629 100644 (file)
--- a/src/cmd/compile/internal/mips/galign.go
+++ b/src/cmd/compile/internal/mips/galign.go
@@ -23,4 +23,5 @@ func Init() {
        gc.Thearch.SSAMarkMoves = func(s *gc.SSAGenState, b *ssa.Block) {}
        gc.Thearch.SSAGenValue = ssaGenValue
        gc.Thearch.SSAGenBlock = ssaGenBlock
+       gc.Thearch.ZeroAuto = zeroAuto
 }

diff --git a/src/cmd/compile/internal/mips/ggen.go b/src/cmd/compile/internal/mips/ggen.go
index ec540f8102aa4f6c3c91e47df477dabac796d8ab..a95db57ffe55be5a5e49d14e019f29391feb2313 100644 (file)
--- a/src/cmd/compile/internal/mips/ggen.go
+++ b/src/cmd/compile/internal/mips/ggen.go
@@ -92,6 +92,23 @@ func zerorange(p *obj.Prog, frame int64, lo int64, hi int64) *obj.Prog {
        return p
 }
 
+func zeroAuto(n *gc.Node, pp *obj.Prog) {
+       // Note: this code must not clobber any registers.
+       sym := gc.Linksym(n.Sym)
+       size := n.Type.Size()
+       for i := int64(0); i < size; i += 4 {
+               p := gc.AddAsmAfter(mips.AMOVW, pp)
+               pp = p
+               p.From.Type = obj.TYPE_REG
+               p.From.Reg = mips.REGZERO
+               p.To.Type = obj.TYPE_MEM
+               p.To.Name = obj.NAME_AUTO
+               p.To.Reg = mips.REGSP
+               p.To.Offset = n.Xoffset + i
+               p.To.Sym = sym
+       }
+}
+
 func ginsnop() {
        p := gc.Prog(mips.ANOR)
        p.From.Type = obj.TYPE_REG

diff --git a/src/cmd/compile/internal/mips64/galign.go b/src/cmd/compile/internal/mips64/galign.go
index 4a36a4ce5b44fac87b335a5b750af3f0241fc309..e8ea073057761ee1da7ff82c22a893f8d4a0a258 100644 (file)
--- a/src/cmd/compile/internal/mips64/galign.go
+++ b/src/cmd/compile/internal/mips64/galign.go
@@ -25,4 +25,5 @@ func Init() {
        gc.Thearch.SSAMarkMoves = func(s *gc.SSAGenState, b *ssa.Block) {}
        gc.Thearch.SSAGenValue = ssaGenValue
        gc.Thearch.SSAGenBlock = ssaGenBlock
+       gc.Thearch.ZeroAuto = zeroAuto
 }

diff --git a/src/cmd/compile/internal/mips64/ggen.go b/src/cmd/compile/internal/mips64/ggen.go
index 2af4a8b1ce620432605338981c9c92956ab89059..eb48e2b26166d1d80b2ee5e79e1d3e2d6e1182bc 100644 (file)
--- a/src/cmd/compile/internal/mips64/ggen.go
+++ b/src/cmd/compile/internal/mips64/ggen.go
@@ -95,6 +95,23 @@ func zerorange(p *obj.Prog, frame int64, lo int64, hi int64) *obj.Prog {
        return p
 }
 
+func zeroAuto(n *gc.Node, pp *obj.Prog) {
+       // Note: this code must not clobber any registers.
+       sym := gc.Linksym(n.Sym)
+       size := n.Type.Size()
+       for i := int64(0); i < size; i += 8 {
+               p := gc.AddAsmAfter(mips.AMOVV, pp)
+               pp = p
+               p.From.Type = obj.TYPE_REG
+               p.From.Reg = mips.REGZERO
+               p.To.Type = obj.TYPE_MEM
+               p.To.Name = obj.NAME_AUTO
+               p.To.Reg = mips.REGSP
+               p.To.Offset = n.Xoffset + i
+               p.To.Sym = sym
+       }
+}
+
 func ginsnop() {
        p := gc.Prog(mips.ANOR)
        p.From.Type = obj.TYPE_REG

diff --git a/src/cmd/compile/internal/ppc64/galign.go b/src/cmd/compile/internal/ppc64/galign.go
index 186aa2946a892e4c689fe49a4bfe654d90f93368..37802477310d0d0c4afd16f4b27af7e11e36c592 100644 (file)
--- a/src/cmd/compile/internal/ppc64/galign.go
+++ b/src/cmd/compile/internal/ppc64/galign.go
@@ -24,6 +24,7 @@ func Init() {
        gc.Thearch.SSAMarkMoves = ssaMarkMoves
        gc.Thearch.SSAGenValue = ssaGenValue
        gc.Thearch.SSAGenBlock = ssaGenBlock
+       gc.Thearch.ZeroAuto = zeroAuto
 
        initvariants()
        initproginfo()

diff --git a/src/cmd/compile/internal/ppc64/ggen.go b/src/cmd/compile/internal/ppc64/ggen.go
index b3ce96856721949f14e25151ba3631654c5000ff..4abd18dc3da0dad52a7f2913cf9b82f816e71d66 100644 (file)
--- a/src/cmd/compile/internal/ppc64/ggen.go
+++ b/src/cmd/compile/internal/ppc64/ggen.go
@@ -90,6 +90,23 @@ func zerorange(p *obj.Prog, frame int64, lo int64, hi int64) *obj.Prog {
        return p
 }
 
+func zeroAuto(n *gc.Node, pp *obj.Prog) {
+       // Note: this code must not clobber any registers.
+       sym := gc.Linksym(n.Sym)
+       size := n.Type.Size()
+       for i := int64(0); i < size; i += 8 {
+               p := gc.AddAsmAfter(ppc64.AMOVD, pp)
+               pp = p
+               p.From.Type = obj.TYPE_REG
+               p.From.Reg = ppc64.REGZERO
+               p.To.Type = obj.TYPE_MEM
+               p.To.Name = obj.NAME_AUTO
+               p.To.Reg = ppc64.REGSP
+               p.To.Offset = n.Xoffset + i
+               p.To.Sym = sym
+       }
+}
+
 func ginsnop() {
        p := gc.Prog(ppc64.AOR)
        p.From.Type = obj.TYPE_REG

diff --git a/src/cmd/compile/internal/s390x/galign.go b/src/cmd/compile/internal/s390x/galign.go
index 91b9ed07775984b0f08929db935c0f367d85bd93..9424d204fa01ff85dc2d21f374a0e4a7aeb07680 100644 (file)
--- a/src/cmd/compile/internal/s390x/galign.go
+++ b/src/cmd/compile/internal/s390x/galign.go
@@ -20,4 +20,5 @@ func Init() {
        gc.Thearch.SSAMarkMoves = ssaMarkMoves
        gc.Thearch.SSAGenValue = ssaGenValue
        gc.Thearch.SSAGenBlock = ssaGenBlock
+       gc.Thearch.ZeroAuto = zeroAuto
 }

diff --git a/src/cmd/compile/internal/s390x/ggen.go b/src/cmd/compile/internal/s390x/ggen.go
index 15c65546d6914a406ff2130ce1165f62c4a72192..5ecfaa517747433afc78264c9d7d2b3e94d07cb2 100644 (file)
--- a/src/cmd/compile/internal/s390x/ggen.go
+++ b/src/cmd/compile/internal/s390x/ggen.go
@@ -143,6 +143,19 @@ func zerorange(p *obj.Prog, frame int64, lo int64, hi int64) *obj.Prog {
        return p
 }
 
+func zeroAuto(n *gc.Node, pp *obj.Prog) {
+       // Note: this code must not clobber any registers.
+       p := gc.AddAsmAfter(s390x.ACLEAR, pp)
+       pp = p
+       p.From.Type = obj.TYPE_CONST
+       p.From.Offset = n.Type.Size()
+       p.To.Type = obj.TYPE_MEM
+       p.To.Name = obj.NAME_AUTO
+       p.To.Reg = s390x.REGSP
+       p.To.Offset = n.Xoffset
+       p.To.Sym = gc.Linksym(n.Sym)
+}
+
 func ginsnop() {
        p := gc.Prog(s390x.AOR)
        p.From.Type = obj.TYPE_REG

diff --git a/src/cmd/compile/internal/x86/galign.go b/src/cmd/compile/internal/x86/galign.go
index edac6a002abe2a72cb315785da6c503b740430db..bb29d2a02f103ab5ae1474c1215db9edda282974 100644 (file)
--- a/src/cmd/compile/internal/x86/galign.go
+++ b/src/cmd/compile/internal/x86/galign.go
@@ -31,4 +31,5 @@ func Init() {
        gc.Thearch.SSAMarkMoves = ssaMarkMoves
        gc.Thearch.SSAGenValue = ssaGenValue
        gc.Thearch.SSAGenBlock = ssaGenBlock
+       gc.Thearch.ZeroAuto = zeroAuto
 }

diff --git a/src/cmd/compile/internal/x86/ggen.go b/src/cmd/compile/internal/x86/ggen.go
index 25769b4de031ea87bc0f3fe220f952fc999d64aa..33ffc5f534f03597170517d951bde1266450b632 100644 (file)
--- a/src/cmd/compile/internal/x86/ggen.go
+++ b/src/cmd/compile/internal/x86/ggen.go
@@ -84,6 +84,23 @@ func zerorange(p *obj.Prog, frame int64, lo int64, hi int64, ax *uint32) *obj.Pr
        return p
 }
 
+func zeroAuto(n *gc.Node, pp *obj.Prog) {
+       // Note: this code must not clobber any registers.
+       sym := gc.Linksym(n.Sym)
+       size := n.Type.Size()
+       for i := int64(0); i < size; i += 4 {
+               p := gc.AddAsmAfter(x86.AMOVL, pp)
+               pp = p
+               p.From.Type = obj.TYPE_CONST
+               p.From.Offset = 0
+               p.To.Type = obj.TYPE_MEM
+               p.To.Name = obj.NAME_AUTO
+               p.To.Reg = x86.REG_SP
+               p.To.Offset = n.Xoffset + i
+               p.To.Sym = sym
+       }
+}
+
 func ginsnop() {
        p := gc.Prog(x86.AXCHGL)
        p.From.Type = obj.TYPE_REG

diff --git a/test/fixedbugs/issue20029.go b/test/fixedbugs/issue20029.go
new file mode 100644 (file)
index 0000000..db3f8aa
--- /dev/null
+++ b/test/fixedbugs/issue20029.go
@@ -0,0 +1,32 @@
+// run
+
+// Copyright 2017 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 20029: make sure we zero at VARKILLs of
+// ambiguously live variables.
+// The ambiguously live variable here is the hiter
+// for the inner range loop.
+
+package main
+
+import "runtime"
+
+func f(m map[int]int) {
+outer:
+       for i := 0; i < 10; i++ {
+               for k := range m {
+                       if k == 5 {
+                               continue outer
+                       }
+               }
+               runtime.GC()
+               break
+       }
+       runtime.GC()
+}
+func main() {
+       m := map[int]int{1: 2, 2: 3, 3: 4}
+       f(m)
+}