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: copystack doesn't adjust frame pointers on arm64 #58432

Closed
felixge opened this issue Feb 9, 2023 · 13 comments
Closed

runtime: copystack doesn't adjust frame pointers on arm64 #58432

felixge opened this issue Feb 9, 2023 · 13 comments
Labels
arch-arm64 compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@felixge
Copy link
Contributor

felixge commented Feb 9, 2023 •
edited
Loading

What version of Go are you using (go version)?

$ go version
go version go1.20 darwin/arm64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using

any/arm64

What did you do?

Try to implement frame pointer unwinding for the tracer (gh 16638, wip patch).

What did you expect to see?

copystack should adjust frame pointers on the stack when growing the stack.

What did you see instead?

TODO what about arm64 frame pointer adjustment? 😅.

Seems like this issue was missed when the CL 61511 for this landed in 2018.

This is causing lots of crashes when trying to frame pointer unwind stacks that have grown.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 9, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/466315 mentions this issue: runtime: add missing fp adjustment for arm64

@bcmills
Copy link
Contributor

bcmills commented Feb 9, 2023

Possibly related to #53037?

@felixge
Copy link
Contributor Author

felixge commented Feb 9, 2023 •
edited
Loading

@mknyszek @prattmic @aclements FYI. Took me a while to debug the fp unwinding crashes I saw, but this seems to be the root cause and relatively easy to fix (see CL above). However, the fix reveals (or causes?) more issues with frame pointers on arm64,- details are in CL 466315.

I'm putting this on hold for now and will focus on making fp unwinding work on amd64 first. But if anybody has a good idea, please let me know :).

@felixge
Copy link
Contributor Author

felixge commented Feb 9, 2023

Possibly related to #53037?

Not sure. But I think #39524 is definitely related.

@bcmills
Copy link
Contributor

bcmills commented Feb 9, 2023

(CC @golang/arm)

@erifan
Copy link

erifan commented Feb 10, 2023 •
edited
Loading

There's a lot of work to be done here, can you try if this CL https://go-review.googlesource.com/c/go/+/241158 works for you first?

@felixge
Copy link
Contributor Author

felixge commented Feb 10, 2023

Thanks @erifan - I hadn't seen this CL. Will definitely try it out.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 10, 2023
@mknyszek mknyszek mentioned this issue Feb 16, 2023
Open
19 tasks
@mknyszek mknyszek added this to the Backlog milestone Feb 22, 2023
@felixge
Copy link
Contributor Author

felixge commented Mar 27, 2023

There's a lot of work to be done here, can you try if this CL https://go-review.googlesource.com/c/go/+/241158 works for you first?

My colleague @nsrip-dd and I are currently looking at this. Could you elaborate what you mean by "a lot of work to be done here"? We can't really figure out why the patch had stalled and what other things might need to be done here. cc @cherrymui

@cherrymui
Copy link
Member

I'm not sure there are a lot. But I'm also not sure if that CL is not enough to ensure frame pointer working everywhere. (It is probably enough for this particular issue.) There are probably a few places we smashed the frame pointer. CL https://golang.org/cl/241080 was one I found. There may be others. Syscall wrappers might be some. I'm happy to update that CL.

@felixge
Copy link
Contributor Author

felixge commented Mar 27, 2023

@cherrymui thanks for the fast reply! Yeah, we suspect that there are more problems like the one you mentioned. We see a lot of crashes when enabling debugCheckBP on arm64, even after applying the changes from you CL. That being said, if you could update the CL that'd be awesome. We'd be happy to review and test this 🙇 .

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/241158 mentions this issue: runtime: adjust frame pointer on stack copy on ARM64

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/481635 mentions this issue: runtime: save frame pointer to the stack in signal handlers for arm64

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/481636 mentions this issue: runtime: zero saved frame pointer when reusing goroutine stack on ARM

@nsrip-dd nsrip-dd mentioned this issue Apr 3, 2023
Open
gopherbot pushed a commit that referenced this issue Apr 5, 2023
@nsrip-dd @mknyszek
runtime: save frame pointer to the stack in signal handlers for arm64
c00e928 
When taking over the goroutine stack in the panic or preemption signal
handlers on arm64, the frame pointer should be saved on the stack (like
the link register) so that frame-pointer unwinding from a panic stack
works properly. Otherwise, tests like TestStackWrapperStackPanic will
fail with the frame pointer check in adjustframe (enabled with
debugCheckBP) when checking the sigpanic frame.

Updates #39524, #58432

Change-Id: I8b89e6fc4877af29b1b81e55e591e6398159855c
Reviewed-on: https://go-review.googlesource.com/c/go/+/481635
Reviewed-by: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Nick Ripley <nick.ripley@datadoghq.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 4, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Jul 4, 2023
gopherbot pushed a commit that referenced this issue Aug 15, 2023
@nsrip-dd @gopherbot
runtime: zero saved frame pointer when reusing goroutine stack on arm64
94d36fb 
When a goroutine stack is reused on arm64, the spot on the stack where
the "caller's" frame pointer goes for the topmost frame should be
explicitly zeroed. Otherwise, the frame pointer check in adjustframe
with debugCheckBP enabled will fail on the topmost frame of a call stack
the first time a reused stack is grown.

Updates #39524, #58432

Change-Id: Ic1210dc005e3ecdbf9cd5d7b98846566e56df8f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/481636
Reviewed-by: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators Jul 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-arm64 compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
8 participants
@felixge @mknyszek @dmitshur @dr2chase @bcmills @gopherbot @cherrymui @erifan