aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorCherry Mui <cherryyz@google.com>2022-08-23 18:19:06 -0400
committerHeschi Kreinick <heschi@google.com>2022-08-31 16:31:45 +0000
commit86e9e0ea87982d117c96575eba3e78e2099fdc98 (patch)
tree036ce05644212fcaf763308c04eb3efe3e6199e9
parent0bba4d2fe614a2b54aa2bdaaaf47ae1eafeb561c (diff)
downloadgo-86e9e0ea87982d117c96575eba3e78e2099fdc98.tar.gz
go-86e9e0ea87982d117c96575eba3e78e2099fdc98.zip
[release-branch.go1.19 cmd/compile: align stack offset to alignment larger than PtrSize
In typebits.Set we check that the offset is a multiple of the alignment, which makes perfect sense. But for values like atomic.Int64, which has 8-byte alignment even on 32-bit platforms (i.e. the alignment is larger than PtrSize), if it is on stack it may be under-aligned, as the stack frame is only PtrSize aligned. Normally we would prevent such values on stack, as the escape analysis force values with higher alignment to heap. But for a composite literal assignment like x = AlignedType{...}, the compiler creates an autotmp for the RHS then copies it to the LHS. The autotmp is on stack and may be under-aligned. Currently this may cause an ICE in the typebits.Set check. This CL makes it align the _offset_ of the autotmp to 8 bytes, which satisfies the check. Note that this is actually lying: the actual address at run time may not necessarily be 8-byte aligned as we only align SP to 4 bytes. The under-alignment is probably okay. The only purpose for the autotmp is to copy the value to the LHS, and the copying code we generate (at least currently) doesn't care the alignment beyond stack alignment. Updates #54638. Fixes #54697. Change-Id: I13c16afde2eea017479ff11dfc24092bcb8aba6a Reviewed-on: https://go-review.googlesource.com/c/go/+/425256 Run-TryBot: Cherry Mui <cherryyz@google.com> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: David Chase <drchase@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> (cherry picked from commit 1211a62bdcb0f070c5082255bcc90e1a14c16bb2) Reviewed-on: https://go-review.googlesource.com/c/go/+/425935
-rw-r--r--src/cmd/compile/internal/ssagen/pgen.go8
-rw-r--r--src/cmd/compile/internal/ssagen/ssa.go12
-rw-r--r--test/fixedbugs/issue54638.go40
3 files changed, 56 insertions, 4 deletions
diff --git a/src/cmd/compile/internal/ssagen/pgen.go b/src/cmd/compile/internal/ssagen/pgen.go
index 825b32aa80..711bdef906 100644
--- a/src/cmd/compile/internal/ssagen/pgen.go
+++ b/src/cmd/compile/internal/ssagen/pgen.go
@@ -96,6 +96,7 @@ func needAlloc(n *ir.Name) bool {
func (s *ssafn) AllocFrame(f *ssa.Func) {
s.stksize = 0
s.stkptrsize = 0
+ s.stkalign = int64(types.RegSize)
fn := s.curfn
// Mark the PAUTO's unused.
@@ -160,6 +161,9 @@ func (s *ssafn) AllocFrame(f *ssa.Func) {
}
s.stksize += w
s.stksize = types.Rnd(s.stksize, n.Type().Alignment())
+ if n.Type().Alignment() > int64(types.RegSize) {
+ s.stkalign = n.Type().Alignment()
+ }
if n.Type().HasPointers() {
s.stkptrsize = s.stksize
lastHasPtr = true
@@ -169,8 +173,8 @@ func (s *ssafn) AllocFrame(f *ssa.Func) {
n.SetFrameOffset(-s.stksize)
}
- s.stksize = types.Rnd(s.stksize, int64(types.RegSize))
- s.stkptrsize = types.Rnd(s.stkptrsize, int64(types.RegSize))
+ s.stksize = types.Rnd(s.stksize, s.stkalign)
+ s.stkptrsize = types.Rnd(s.stkptrsize, s.stkalign)
}
const maxStackSize = 1 << 30
diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go
index 83def791f7..809395875c 100644
--- a/src/cmd/compile/internal/ssagen/ssa.go
+++ b/src/cmd/compile/internal/ssagen/ssa.go
@@ -7337,7 +7337,8 @@ func genssa(f *ssa.Func, pp *objw.Progs) {
func defframe(s *State, e *ssafn, f *ssa.Func) {
pp := s.pp
- frame := types.Rnd(s.maxarg+e.stksize, int64(types.RegSize))
+ s.maxarg = types.Rnd(s.maxarg, e.stkalign)
+ frame := s.maxarg + e.stksize
if Arch.PadFrame != nil {
frame = Arch.PadFrame(frame)
}
@@ -7775,7 +7776,14 @@ type ssafn struct {
strings map[string]*obj.LSym // map from constant string to data symbols
stksize int64 // stack size for current frame
stkptrsize int64 // prefix of stack containing pointers
- log bool // print ssa debug to the stdout
+
+ // alignment for current frame.
+ // NOTE: when stkalign > PtrSize, currently this only ensures the offsets of
+ // objects in the stack frame are aligned. The stack pointer is still aligned
+ // only PtrSize.
+ stkalign int64
+
+ log bool // print ssa debug to the stdout
}
// StringData returns a symbol which
diff --git a/test/fixedbugs/issue54638.go b/test/fixedbugs/issue54638.go
new file mode 100644
index 0000000000..d0258b0c68
--- /dev/null
+++ b/test/fixedbugs/issue54638.go
@@ -0,0 +1,40 @@
+// compile
+
+// Copyright 2022 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 54638: composite literal assignment with
+// alignment > PtrSize causes ICE.
+
+package p
+
+import "sync/atomic"
+
+type S struct{ l any }
+
+type T struct {
+ H any
+ a [14]int64
+ f func()
+ x atomic.Int64
+}
+
+//go:noinline
+func (T) M(any) {}
+
+type W [2]int64
+
+//go:noinline
+func (W) Done() {}
+
+func F(l any) [3]*int {
+ var w W
+ var x [3]*int // use some stack
+ t := T{H: S{l: l}}
+ go func() {
+ t.M(l)
+ w.Done()
+ }()
+ return x
+}