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.

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

cmd/compile: ICE at composite literal assignment with alignment > PtrSize [1.19 backport] #54697

Closed
gopherbot opened this issue Aug 26, 2022 · 2 comments
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone

Comments

@gopherbot
Copy link

gopherbot commented Aug 26, 2022

@cherrymui requested issue #54638 to be considered for backport to the next 1.19 minor release.

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

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Aug 26, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 26, 2022
@gopherbot gopherbot added this to the Go1.19.1 milestone Aug 26, 2022
@gopherbot
Copy link
Author

gopherbot commented Aug 26, 2022

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

@heschi heschi added the CherryPickApproved Used during the release process for point releases label Aug 31, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Aug 31, 2022
@gopherbot
Copy link
Author

gopherbot commented Aug 31, 2022

Closed by merging 86e9e0e to release-branch.go1.19.

gopherbot pushed a commit that referenced this issue Aug 31, 2022
…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
bradfitz pushed a commit to tailscale/go that referenced this issue Sep 8, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
None yet
Development

No branches or pull requests

2 participants