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: inconsistent integer arithmetic result on Go 1.22+arm64 with/without -race [1.22 backport] #68230

Closed
gopherbot opened this issue Jun 28, 2024 · 7 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
Contributor

@randall77 requested issue #68227 to be considered for backport to the next 1.22 minor release.

@gopherbot please open backport issue for 1.22.

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Jun 28, 2024
@gopherbot gopherbot added this to the Go1.22.5 milestone Jun 28, 2024
@seankhliao seankhliao changed the title Inconsistent integer arithmetic result on Go 1.22+arm64 with/without -race [1.22 backport] cmd/compile: inconsistent integer arithmetic result on Go 1.22+arm64 with/without -race [1.22 backport] Jun 28, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jun 28, 2024
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/595696 mentions this issue: [release-branch.go1.22] cmd/compile: don't elide zero extension on top of signed values

@randall77
Copy link
Contributor

This issue leads to miscompilation in rare cases involving sign/zero extension and register spill/restore paths.

@gopherbot gopherbot modified the milestones: Go1.22.5, Go1.22.6 Jul 2, 2024
@thanm thanm added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Jul 3, 2024
gopherbot pushed a commit that referenced this issue Jul 10, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…p of signed values

v = ... compute some value, which zeros top 32 bits ...
w = zero-extend v

We want to remove the zero-extension operation, as it doesn't do anything.
But if v is typed as a signed value, and it gets spilled/restored, it
might be re-sign-extended upon restore. So the zero-extend isn't actually
a NOP when there might be calls or other reasons to spill in between v and w.

Fixes #68230

Change-Id: I3b30b8e56c7d70deac1fb09d2becc7395acbadf8
Reviewed-on: https://go-review.googlesource.com/c/go/+/595675
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Joedian Reid <joedian@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
(cherry picked from commit 7f90b96)
Reviewed-on: https://go-review.googlesource.com/c/go/+/595696
Reviewed-by: Keith Randall <khr@google.com>
@gopherbot
Copy link
Contributor Author

Closed by merging 45f9ded to release-branch.go1.22.

@RaduBerinde
Copy link
Contributor

@randall77 is it within the realm of possibility that this issue could lead to a GC crash like the one below? (perhaps in the presence of unsafe code that does non-trivial pointer arithmetic)

SIGSEGV: segmentation violation
PC=0x42b81c m=19 sigcode=1 addr=0x20

goroutine 0 gp=0x400c2121c0 m=19 mp=0x400c210008 [idle]:
runtime.(*mspan).typePointersOfUnchecked(0x40168850e0?, 0x4015086c00?)
  GOROOT/src/runtime/mbitmap_allocheaders.go:202 +0x3c fp=0xffff4f3fccd0 sp=0xffff4f3fccb0 pc=0x42b81c
runtime.scanobject(0x400c792000, 0x40000dc168)
  GOROOT/src/runtime/mgcmark.go:1441 +0x1c4 fp=0xffff4f3fcd60 sp=0xffff4f3fccd0 pc=0x437fd4
runtime.gcDrain(0x40000dc168, 0x2)
  GOROOT/src/runtime/mgcmark.go:1242 +0x1d4 fp=0xffff4f3fcdd0 sp=0xffff4f3fcd60 pc=0x437774
runtime.gcDrainMarkWorkerDedicated(...)
  GOROOT/src/runtime/mgcmark.go:1124
runtime.gcBgMarkWorker.func2()
  GOROOT/src/runtime/mgc.go:1402 +0x154 fp=0xffff4f3fce20 sp=0xffff4f3fcdd0 pc=0x433a34
runtime.systemstack(0x0)
  src/runtime/asm_arm64.s:243 +0x6c fp=0xffff4f3fce30 sp=0xffff4f3fce20 pc=0x48c3fc

(this is from cockroachdb/cockroach#134020)

@randall77
Copy link
Contributor

I guess it is possible, but not terribly likely.

That fault is because the runtime's internal data is corrupted. For that to happen, we'd have to use the bug to calculate a bad offset during some unsafe.Pointer arithmetic. The bug only causes errors in the high 32 bits, so the bad address would be off by a multiple of 4GB. I guess if your heap is that big, it's possible. For smaller heaps I don't see how you could corrupt the heap using this bug.

@RaduBerinde
Copy link
Contributor

The high 32 bit error could apply to an intermediary result, which then causes us to get an incorrect pointer that is less than 4GB away no? For example things like (x * largeConstant) >> 32 are sometimes used as hash functions.

@randall77
Copy link
Contributor

The high 32 bit error could apply to an intermediary result, which then causes us to get an incorrect pointer that is less than 4GB away no?

Sure, it is possible.

For example things like (x * largeConstant) >> 32 are sometimes used as hash functions.

That particular case seems unlikely to be the problem, as a hash function would certainly be followed by a bounds check or mask. It might cause a hash miss where there should be a hit, but that shouldn't corrupt runtime state.

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

4 participants