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

Jump to bottom

cmd/compile: 32-bit random data corruption [1.15 backport] #43575

Closed
gopherbot opened this issue Jan 7, 2021 · 6 comments
Closed

cmd/compile: 32-bit random data corruption [1.15 backport] #43575

gopherbot opened this issue Jan 7, 2021 · 6 comments
Labels
CherryPickApproved
Milestone
Go1.15.8

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 7, 2021

@randall77 requested issue #43570 to be considered for backport to the next 1.15 minor release.

I have a small repro that also works on 64 bit.
It's been broken since 1.12.
@gopherbot please open backport issues.
@gopherbot gopherbot mentioned this issue Jan 7, 2021
Closed
@gopherbot gopherbot added this to the Go1.15.7 milestone Jan 7, 2021
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jan 8, 2021

Change https://golang.org/cl/282558 mentions this issue: cmd/compile: don't short-circuit copies whose source is volatile

Loading

@randall77
Copy link
Contributor

@randall77 randall77 commented Jan 8, 2021

This bug causes random corruption in rare cases involving large structs being copied from function result to heap to function argument. Workaround are possible but difficult.

Loading

@dmitshur dmitshur removed this from the Go1.15.7 milestone Jan 19, 2021
@dmitshur dmitshur added this to the Go1.15.8 milestone Jan 19, 2021
@liggitt
Copy link
Contributor

@liggitt liggitt commented Jan 21, 2021

any ETA on this? would like to avoid sweeping our codebase adjusting anonymous functions modifying closure vars if possible

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Jan 21, 2021

Approving as a serious issue without a reasonable workaround. This backport applies to both 1.15 (this issue) and 1.14 (#43574).

@liggitt This will be included in the next minor release. The next minor release will likely happen closer to early February.

Loading

@dmitshur dmitshur mentioned this issue Jan 21, 2021
Closed
@dmitshur dmitshur changed the title 32-bit random data corruption [1.15 backport] cmd/compile: 32-bit random data corruption [1.15 backport] Jan 21, 2021
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jan 21, 2021

Closed by merging 27d5fcc to release-branch.go1.15.

Loading

@gopherbot gopherbot closed this Jan 21, 2021
gopherbot pushed a commit that referenced this issue Jan 21, 2021
@randall77 @dmitshur
[release-branch.go1.15] cmd/compile: don't short-circuit copies whose…
27d5fcc 
… source is volatile

Current optimization: When we copy a->b and then b->c, we might as well
copy a->c instead of b->c (then b might be dead and go away).

*Except* if a is a volatile location (might be clobbered by a call).
In that case, we really do want to copy a immediately, because there
might be a call before we can do the a->c copy.

User calls can't happen in between, because the rule matches up the
memory states. But calls inserted for memory barriers, particularly
runtime.typedmemmove, can.

(I guess we could introduce a register-calling-convention version
of runtime.typedmemmove, but that seems a bigger change than this one.)

Fixes #43575

Change-Id: Ifa518bb1a6f3a8dd46c352d4fd54ea9713b3eb1a
Reviewed-on: https://go-review.googlesource.com/c/go/+/282492
Trust: Keith Randall <khr@golang.org>
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
(cherry picked from commit 304f769)
Reviewed-on: https://go-review.googlesource.com/c/go/+/282558
Trust: Dmitri Shuralyov <dmitshur@golang.org>
@liggitt
Copy link
Contributor

@liggitt liggitt commented Jan 21, 2021

@liggitt This will be included in the next minor release. The next minor release will likely happen closer to early February.

ack, thanks

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CherryPickApproved
Projects
None yet
Milestone
Go1.15.8
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
@liggitt @dmitshur @randall77 @gopherbot and others