[go] runtime: adjust frame pointer of dead frames interrupted by sigpanic
Nick Ripley (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Cherry Mui
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 2 comments![]( "Open in Gerrit")
Seems reasonable.
I have one question - can we just get rid of the continpc==0 special case? Does that also fix this issue?
If it is just an optimization, I'm happy to get rid of it.
runtime.KeepAlive(&i)Is there a point to this KeepAlive call?
(Maybe making sure deref gets a frame?)
Related details
- Cherry Mui
- Nick Ripley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted and added 2 comments![]( "Open in Gerrit")
Votes added by Cherry Mui
| Code-Review | +1 |
2 comments
f := frame.fn
if stackDebug >= 2 {
print(" adjusting ", funcname(f), " frame=[", hex(frame.sp), ",", hex(frame.fp), "] pc=", hex(frame.pc), " continpc=", hex(frame.continpc), "\n")
}
We probably should keep the debug print at the top. Otherwise the print of "saved bp" above would be weird.
// saved. See the diagram. Note that preparePanic also saves a
// "frame pointer" in sigpanic's frame, but this frame pointer
// is not linked in to the frame pointer sequence since sigpanic
// will see the frame pointer register value as of the panicking
// function call.Should preparePanic just update the FP register (R29) in the signal context, so it is linked?
Related details
- Nick Ripley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Ripley (Gerrit)
Nick Ripley uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Nick Ripley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Ripley (Gerrit)
Nick Ripley voted and added 4 comments![]( "Open in Gerrit")
Votes added by Nick Ripley
| Commit-Queue | +1 |
Seems reasonable.
I have one question - can we just get rid of the continpc==0 special case? Does that also fix this issue?
If it is just an optimization, I'm happy to get rid of it.
It's just an optimization, yeah. The frame.getStackMap function already returns empty values if continpc is 0. In that case rest of adjustframe does very little since the rest of the function is just using those empty values. I went ahead and dropped the check instead.
We'll still need additional the fixup for arm64, though.
Is there a point to this KeepAlive call?
(Maybe making sure deref gets a frame?)
Just making sure deref gets a frame. I added a comment.
f := frame.fn
if stackDebug >= 2 {
print(" adjusting ", funcname(f), " frame=[", hex(frame.sp), ",", hex(frame.fp), "] pc=", hex(frame.pc), " continpc=", hex(frame.continpc), "\n")
}
We probably should keep the debug print at the top. Otherwise the print of "saved bp" above would be weird.
Done as part of responding to Keith's comment about removing the continpc == 0 special case.
// saved. See the diagram. Note that preparePanic also saves a
// "frame pointer" in sigpanic's frame, but this frame pointer
// is not linked in to the frame pointer sequence since sigpanic
// will see the frame pointer register value as of the panicking
// function call.Should preparePanic just update the FP register (R29) in the signal context, so it is linked?
We could update R29 to link in what preparePanic saves, but we would still need to fix the frame pointer saved below the panicking function's call frame since we'd still visit it.
Also, if we link in the frame pointer that preparePanic saves, we can end up with duplicate frames in the traceback. preparePanic saves the LR value from the panicking function onto the call stack. That will be the same LR value that the panicking function saved, if it has not updated LR. If I understand the internal ABI correctly, LR can be used as a scratch register in a function body. So the LR that prepare panic saves onto the stack might not even be a valid PC.
My inclination would be to skip it. In CL 481635 I made preparePanic save a valid frame pointer so that debugCheckBP is happy when it sees the frame pointer saved by preparePanic at the top of the sigpanic frame. But if we don't link it, perhaps preparePanic can just put 0 there. WDYT?
Related details
- Cherry Mui
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |