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

runtime: gentraceback() dead loop on arm64 casued the process hang [1.17 backport] #53111

Closed
gopherbot opened this issue May 27, 2022 · 6 comments
Closed

runtime: gentraceback() dead loop on arm64 casued the process hang [1.17 backport] #53111

gopherbot opened this issue May 27, 2022 · 6 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.17.13

Comments

@gopherbot
Copy link

@cherrymui requested issue #52116 to be considered for backport to the next 1.17 minor release.

@gopherbot please backport this to previous releases. This is a runtime bug which can cause programs to hang or crash. Thanks.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label May 27, 2022
@gopherbot gopherbot mentioned this issue May 27, 2022
Closed
@gopherbot gopherbot added this to the Go1.17.11 milestone May 27, 2022
@gopherbot
Copy link
Author

Change https://go.dev/cl/408822 mentions this issue: [release-branch.go1.17] runtime: use saved LR when unwinding through morestack

@dmitshur dmitshur modified the milestones: Go1.17.11, Go1.17.12 Jun 1, 2022
@heschi
Copy link
Contributor

heschi commented Jun 15, 2022

@cherrymui there was some confusion in the release meeting about whether this CL had solved the problem. Is this (and its 1.18 sibling) still a good cherrypick?

@cherrymui
Copy link
Member

I think the CL solves the problem. What was the confusion? Thanks.

@heschi
Copy link
Contributor

heschi commented Jun 15, 2022

cc @aclements

@aclements
Copy link
Member

We just weren't sure whether this was still a hypothesis or it had been verified to fix the issue. Since it sounds like it's been verified, I think we should approve this cherry-pick.

@joedian joedian added the CherryPickApproved Used during the release process for point releases label Jun 22, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Jun 22, 2022
@gopherbot gopherbot modified the milestones: Go1.17.12, Go1.17.13 Jul 12, 2022
@knz knz mentioned this issue Jul 18, 2022
Closed
gopherbot pushed a commit that referenced this issue Jul 22, 2022
@cherrymui @heschi
[release-branch.go1.17] runtime: use saved LR when unwinding through …
c25b12f 
…morestack

On LR machine, consider F calling G calling H, which grows stack.
The stack looks like
...
G's frame:
	... locals ...
	saved LR = return PC in F  <- SP points here at morestack
H's frame (to be created)

At morestack, we save
	gp.sched.pc = H's morestack call
	gp.sched.sp = H's entry SP (the arrow above)
	gp.sched.lr = return PC in G

Currently, when unwinding through morestack (if _TraceJumpStack
is set), we switch PC and SP but not LR. We then have
	frame.pc = H's morestack call
	frame.sp = H's entry SP (the arrow above)
As LR is not set, we load it from stack at *sp, so
	frame.lr = return PC in F
As the SP hasn't decremented at the morestack call,
	frame.fp = frame.sp = H's entry SP

Unwinding a frame, we have
	frame.pc = old frame.lr = return PC in F
	frame.sp = old frame.fp = H's entry SP a.k.a. G's SP
The PC and SP don't match. The unwinding will go off if F and G
have different frame sizes.

Fix this by preserving the LR when switching stack.

Also add code to detect infinite loop in unwinding.

TODO: add some test. I can reproduce the infinite loop (or throw
with added check) but the frequency is low.

Fixes #53111.
Updates #52116.

Change-Id: I6e1294f1c6e55f664c962767a1cf6c466a0c0eff
Reviewed-on: https://go-review.googlesource.com/c/go/+/400575
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Eric Fang <eric.fang@arm.com>
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
(cherry picked from commit 74f0009)
Reviewed-on: https://go-review.googlesource.com/c/go/+/408822
Reviewed-by: Austin Clements <austin@google.com>
@gopherbot
Copy link
Author

Closed by merging c25b12f to release-branch.go1.17.

@golang golang locked and limited conversation to collaborators Jul 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.17.13
Development

No branches or pull requests
6 participants
@dmitshur @aclements @gopherbot @cherrymui @joedian @heschi