Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/compile: ICE at composite literal assignment with alignment > PtrSize #54638

Closed
cherrymui opened this issue Aug 23, 2022 · 7 comments
Closed

cmd/compile: ICE at composite literal assignment with alignment > PtrSize #54638

cherrymui opened this issue Aug 23, 2022 · 7 comments
Assignees
@cherrymui
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@cherrymui
Copy link
Member

What version of Go are you using (go version)?

tip (60ad3c4)

Does this issue reproduce with the latest release?

Unsure. It doesn't immediately reproduce with Go 1.19, but based on the code I think it might be possible.

What operating system and processor architecture are you using (go env)?

Building for linux/arm.

What did you do?

package p

import "log"
import "net"
import "net/http"
import "sync"

type httpServer struct {
	l net.Listener
}

func (srv httpServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
	panic("XXX")
}

var errLogger *log.Logger

func F(l net.Listener) [3]*int {
	var wg sync.WaitGroup
	var x [3]*int // use some stack
	srv := http.Server{
		Handler:  httpServer{l: l},
		ErrorLog: errLogger,
	}
	wg.Add(1)
	go func() {
		srv.Serve(l)
		wg.Done()
	}()
	return x
}

Build this code for ARM
$ GOARCH=arm GOOS=linux go build s.go.

What did you expect to see?

Build successfully.

What did you see instead?

ICE.

$ GOARCH=arm GOOS=linux go-tmp build s.go
# command-line-arguments
<autogenerated>:1: internal compiler error: typebits.Set: invalid initial alignment: type http.Server has alignment 8, but offset is 28

goroutine 9 [running]:
runtime/debug.Stack()
	/Users/cherryyz/src/go-tmp/src/runtime/debug/stack.go:24 +0x65
cmd/compile/internal/base.FatalfAt({0x14?, 0x0?}, {0x1997dd3, 0x53}, {0xc000b1f5f8, 0x3, 0x3})
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/base/print.go:227 +0x1d7
cmd/compile/internal/base.Fatalf(...)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/base/print.go:196
cmd/compile/internal/typebits.Set(0xc0008dd880, 0x1c, {0xb1f6a0?, {0xc0005a67d0?, 0xc000680000?, 0x13?}})
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/typebits/typebits.go:18 +0x157
cmd/compile/internal/liveness.(*liveness).pointerMap(0xc000b82140, {0x2?, {0xc0005a67b4?, 0x9?, 0x1?}}, {0xc000450240, 0x7, 0x7?}, {0x5, {0xc0005a67c4, ...}}, ...)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/liveness/plive.go:433 +0x26c
cmd/compile/internal/liveness.(*liveness).emit(0xc000b82140)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/liveness/plive.go:1316 +0x309
cmd/compile/internal/liveness.Compute(0xc000732000, 0xc000502340, 0x19717ed?, 0xb?)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/liveness/plive.go:1372 +0x21d
cmd/compile/internal/ssagen.genssa(0xc000502340, 0xc000644620)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/ssagen/ssa.go:6894 +0xa5
cmd/compile/internal/ssagen.Compile(0xc000732000, 0xc000059f90?)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/ssagen/pgen.go:193 +0x272
cmd/compile/internal/gc.compileFunctions.func4.1(0x0?)
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/gc/compile.go:153 +0x3a
cmd/compile/internal/gc.compileFunctions.func3.1()
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/gc/compile.go:140 +0x4d
created by cmd/compile/internal/gc.compileFunctions.func3
	/Users/cherryyz/src/go-tmp/src/cmd/compile/internal/gc/compile.go:138 +0x78

Here, we have a local variable srv of type http.Server, which contains a sync.WaitGroup which in turn contains a atomic.Uint64, which makes it 8-byte aligned even on 32-bit platforms.

The compiler already force variables with alignment larger than the stack alignment (PtrSize) to heap. But here for the composite literal assignment the compiler generates an autotmp

.   AS Def tc(1) # s.go:21:6
.   .   NAME-p.srv esc(h) Class:PAUTO Offset:0 Addrtaken Used http.Server tc(1) # s.go:21:2
.   .   NAME-p..autotmp_6 esc(N) Class:PAUTO Offset:0 AutoTemp OnStack Used http.Server tc(1) # s.go:21:20

That autotmp lives on stack, and doesn't have 8-byte alignment. This triggers an assertion when emitting the stack maps https://cs.opensource.google/go/go/+/master:src/cmd/compile/internal/typebits/typebits.go;l=17

I think it is probably okay to have an under-aligned autotmp on stack. 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. So maybe we can relax the check to allow under-aligned values on stack? (It would be good to check that it is only used for copying and doesn't need the alignment beyond PtrSize, but I'm not sure how to.)

Not sure if there is a better fix. Not using such autotmp would need much more work.

We could align the offset of the autotmp to 8 bytes, which would satisfy the check. But it is just faking. The actual address at run time may not necessarily be 8-byte aligned.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 23, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/425256 mentions this issue: cmd/compile: relax typebits.Set alignment check

@randall77
Copy link
Contributor

We could raise the alignment of stack frames to 8 bytes. Callbacks from assembly might make that difficult, especially if we're still on abi0 though...

@cherrymui
Copy link
Member Author

Yeah, increase stack alignment is a possibility, and as you said assembly code may be tricky. (There are probably some discussions about increasing stack alignment in the past.)

@stv0g
Copy link

stv0g commented Aug 24, 2022

I can reproduce the same on linux/arm with Go 1.19

@dr2chase dr2chase added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 26, 2022
@cherrymui
Copy link
Member Author

@gopherbot please backport this to Go 1.19. This may cause compiler crash on valid real programs. Thanks.

@gopherbot gopherbot mentioned this issue Aug 26, 2022
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #54697 (for 1.19).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/425935 mentions this issue: [release-branch.go1.19 cmd/compile: align stack offset to alignment larger than PtrSize

gopherbot pushed a commit that referenced this issue Aug 31, 2022
@cherrymui @heschi
[release-branch.go1.19 cmd/compile: align stack offset to alignment l…
86e9e0e 
…arger 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 1211a62)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425935
rajbarik pushed a commit to rajbarik/go that referenced this issue Sep 1, 2022
@cherrymui
cmd/compile: align stack offset to alignment larger than PtrSize
520691c 
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.

Fixes golang#54638.

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>
bradfitz pushed a commit to tailscale/go that referenced this issue Sep 8, 2022
@cherrymui @bradfitz
[release-branch.go1.19 cmd/compile: align stack offset to alignment l…
6217bd8 
…arger 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 golang#54638.
Fixes golang#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 1211a62)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425935
@stv0g stv0g mentioned this issue Sep 10, 2022
Closed
@golang golang locked and limited conversation to collaborators Aug 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@cherrymui cherrymui

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@stv0g @dr2chase @randall77 @gopherbot @cherrymui