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

runtime: scheduler change causes Delve's function call injection to fail intermittently [1.21 backport] #62509

Closed
gopherbot opened this issue Sep 7, 2023 · 2 comments
Closed

runtime: scheduler change causes Delve's function call injection to fail intermittently [1.21 backport] #62509

gopherbot opened this issue Sep 7, 2023 · 2 comments
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone
Go1.21.2

Comments

@gopherbot
Copy link
Contributor

@mknyszek requested issue #61732 to be considered for backport to the next 1.21 minor release.

Ha, OK. Backport it is.

@gopherbot Please open a backport issue for Go 1.21. This issue causes significant debugger friction due to an issue that was latent in earlier releases but only really started cropping up in Go 1.21 due to a scheduler change in that release. The fix is very low risk, since it only impacts the debugger function call protocol, which should have no impact on production applications. The fix is also fairly small and straightforward.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Sep 7, 2023
@gopherbot gopherbot mentioned this issue Sep 7, 2023
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 7, 2023
@gopherbot gopherbot added this to the Go1.21.2 milestone Sep 7, 2023
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/526576 mentions this issue: [release-branch.go1.21] runtime: always lock OS thread in debugcall

@joedian joedian added the CherryPickApproved Used during the release process for point releases label Sep 20, 2023
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Sep 20, 2023
gopherbot pushed a commit that referenced this issue Sep 25, 2023
@mknyszek @thanm
[release-branch.go1.21] runtime: always lock OS thread in debugcall
0b6b0a2 
Right now debuggers like Delve rely on the new goroutine created to run
a debugcall function to run on the same thread it started on, up until
it hits itself with a SIGINT as part of the debugcall protocol.

That's all well and good, except debugCallWrap1 isn't particularly
careful about not growing the stack. For example, if the new goroutine
happens to have a stale preempt flag, then it's possible a stack growth
will cause a roundtrip into the scheduler, possibly causing the
goroutine to switch to another thread.

Previous attempts to just be more careful around debugCallWrap1 were
helpful, but insufficient. This change takes everything a step further
and always locks the debug call goroutine and the new goroutine it
creates to the OS thread.

For #61732.
Fixes #62509.

Change-Id: I038f3a4df30072833e27e6a5a1ec01806a32891f
Reviewed-on: https://go-review.googlesource.com/c/go/+/515637
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alessandro Arzilli <alessandro.arzilli@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
(cherry picked from commit d9a4b24)
Reviewed-on: https://go-review.googlesource.com/c/go/+/526576
@gopherbot
Copy link
Contributor Author

Closed by merging 0b6b0a2 to release-branch.go1.21.

bradfitz pushed a commit to tailscale/go that referenced this issue Sep 25, 2023
@mknyszek @bradfitz
[release-branch.go1.21] runtime: always lock OS thread in debugcall
3ad76fe 
Right now debuggers like Delve rely on the new goroutine created to run
a debugcall function to run on the same thread it started on, up until
it hits itself with a SIGINT as part of the debugcall protocol.

That's all well and good, except debugCallWrap1 isn't particularly
careful about not growing the stack. For example, if the new goroutine
happens to have a stale preempt flag, then it's possible a stack growth
will cause a roundtrip into the scheduler, possibly causing the
goroutine to switch to another thread.

Previous attempts to just be more careful around debugCallWrap1 were
helpful, but insufficient. This change takes everything a step further
and always locks the debug call goroutine and the new goroutine it
creates to the OS thread.

For golang#61732.
Fixes golang#62509.

Change-Id: I038f3a4df30072833e27e6a5a1ec01806a32891f
Reviewed-on: https://go-review.googlesource.com/c/go/+/515637
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alessandro Arzilli <alessandro.arzilli@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
(cherry picked from commit d9a4b24)
Reviewed-on: https://go-review.googlesource.com/c/go/+/526576
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
None yet
Milestone
Go1.21.2
Development

No branches or pull requests
3 participants
@gopherbot @joedian and others