From: Cherry Zhang Date: Thu, 21 Mar 2019 17:40:28 +0000 (-0400) Subject: [release-branch.go1.12] cmd/compile: copy volatile values before emitting write barri... X-Git-Tag: go1.12.2~11 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=8acc2ea68ba126c6f91afef2545fd6022b039c51;p=gostls13.git [release-branch.go1.12] cmd/compile: copy volatile values before emitting write barrier call It is possible that a "volatile" value (one that can be clobbered by preparing args of a call) to be used in multiple write barrier calls. We used to copy the volatile value right before each call. But this doesn't work if the value is used the second time, after the first call where it is already clobbered. Copy it before emitting any call. Updates #30977. Fixes #30996. Change-Id: Iedcc91ad848d5ded547bf37a8359c125d32e994c Reviewed-on: https://go-review.googlesource.com/c/go/+/168677 Run-TryBot: Cherry Zhang TryBot-Result: Gobot Gobot Reviewed-by: Keith Randall (cherry picked from commit f23c601bf9f66ef4a0d9d2dcd003b95e78fb28f8) Reviewed-on: https://go-review.googlesource.com/c/go/+/168817 Run-TryBot: Brad Fitzpatrick --- diff --git a/src/cmd/compile/internal/ssa/writebarrier.go b/src/cmd/compile/internal/ssa/writebarrier.go index 1024ab25ab..8cb41b2935 100644 --- a/src/cmd/compile/internal/ssa/writebarrier.go +++ b/src/cmd/compile/internal/ssa/writebarrier.go @@ -196,6 +196,43 @@ func writebarrier(f *Func) { // and simple store version to bElse memThen := mem memElse := mem + + // If the source of a MoveWB is volatile (will be clobbered by a + // function call), we need to copy it to a temporary location, as + // marshaling the args of typedmemmove might clobber the value we're + // trying to move. + // Look for volatile source, copy it to temporary before we emit any + // call. + // It is unlikely to have more than one of them. Just do a linear + // search instead of using a map. + type volatileCopy struct { + src *Value // address of original volatile value + tmp *Value // address of temporary we've copied the volatile value into + } + var volatiles []volatileCopy + copyLoop: + for _, w := range stores { + if w.Op == OpMoveWB { + val := w.Args[1] + if isVolatile(val) { + for _, c := range volatiles { + if val == c.src { + continue copyLoop // already copied + } + } + + t := val.Type.Elem() + tmp := f.fe.Auto(w.Pos, t) + memThen = bThen.NewValue1A(w.Pos, OpVarDef, types.TypeMem, tmp, memThen) + tmpaddr := bThen.NewValue2A(w.Pos, OpLocalAddr, t.PtrTo(), tmp, sp, memThen) + siz := t.Size() + memThen = bThen.NewValue3I(w.Pos, OpMove, types.TypeMem, siz, tmpaddr, val, memThen) + memThen.Aux = t + volatiles = append(volatiles, volatileCopy{val, tmpaddr}) + } + } + } + for _, w := range stores { ptr := w.Args[0] pos := w.Pos @@ -222,11 +259,19 @@ func writebarrier(f *Func) { // then block: emit write barrier call switch w.Op { case OpStoreWB, OpMoveWB, OpZeroWB: - volatile := w.Op == OpMoveWB && isVolatile(val) if w.Op == OpStoreWB { memThen = bThen.NewValue3A(pos, OpWB, types.TypeMem, gcWriteBarrier, ptr, val, memThen) } else { - memThen = wbcall(pos, bThen, fn, typ, ptr, val, memThen, sp, sb, volatile) + srcval := val + if w.Op == OpMoveWB && isVolatile(srcval) { + for _, c := range volatiles { + if srcval == c.src { + srcval = c.tmp + break + } + } + } + memThen = wbcall(pos, bThen, fn, typ, ptr, srcval, memThen, sp, sb) } // Note that we set up a writebarrier function call. f.fe.SetWBPos(pos) @@ -249,6 +294,12 @@ func writebarrier(f *Func) { } } + // mark volatile temps dead + for _, c := range volatiles { + tmpNode := c.tmp.Aux + memThen = bThen.NewValue1A(memThen.Pos, OpVarKill, types.TypeMem, tmpNode, memThen) + } + // merge memory // Splice memory Phi into the last memory of the original sequence, // which may be used in subsequent blocks. Other memories in the @@ -302,25 +353,9 @@ func writebarrier(f *Func) { } // wbcall emits write barrier runtime call in b, returns memory. -// if valIsVolatile, it moves val into temp space before making the call. -func wbcall(pos src.XPos, b *Block, fn, typ *obj.LSym, ptr, val, mem, sp, sb *Value, valIsVolatile bool) *Value { +func wbcall(pos src.XPos, b *Block, fn, typ *obj.LSym, ptr, val, mem, sp, sb *Value) *Value { config := b.Func.Config - var tmp GCNode - if valIsVolatile { - // Copy to temp location if the source is volatile (will be clobbered by - // a function call). Marshaling the args to typedmemmove might clobber the - // value we're trying to move. - t := val.Type.Elem() - tmp = b.Func.fe.Auto(val.Pos, t) - mem = b.NewValue1A(pos, OpVarDef, types.TypeMem, tmp, mem) - tmpaddr := b.NewValue2A(pos, OpLocalAddr, t.PtrTo(), tmp, sp, mem) - siz := t.Size() - mem = b.NewValue3I(pos, OpMove, types.TypeMem, siz, tmpaddr, val, mem) - mem.Aux = t - val = tmpaddr - } - // put arguments on stack off := config.ctxt.FixedFrameSize() @@ -348,11 +383,6 @@ func wbcall(pos src.XPos, b *Block, fn, typ *obj.LSym, ptr, val, mem, sp, sb *Va // issue call mem = b.NewValue1A(pos, OpStaticCall, types.TypeMem, fn, mem) mem.AuxInt = off - config.ctxt.FixedFrameSize() - - if valIsVolatile { - mem = b.NewValue1A(pos, OpVarKill, types.TypeMem, tmp, mem) // mark temp dead - } - return mem } diff --git a/test/fixedbugs/issue30977.go b/test/fixedbugs/issue30977.go new file mode 100644 index 0000000000..2ca040d79a --- /dev/null +++ b/test/fixedbugs/issue30977.go @@ -0,0 +1,52 @@ +// run + +// Copyright 2019 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 30977: write barrier call clobbers volatile +// value when there are multiple uses of the value. + +package main + +import "runtime" + +type T struct { + a, b, c, d, e string +} + +//go:noinline +func g() T { + return T{"a", "b", "c", "d", "e"} +} + +//go:noinline +func f() { + // The compiler optimizes this to direct copying + // the call result to both globals, with write + // barriers. The first write barrier call clobbers + // the result of g on stack. + X = g() + Y = X +} + +var X, Y T + +const N = 1000 + +func main() { + // Keep GC running so the write barrier is on. + go func() { + for { + runtime.GC() + } + }() + + for i := 0; i < N; i++ { + runtime.Gosched() + f() + if X != Y { + panic("FAIL") + } + } +}