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

Debugframesize #2625

Closed
wants to merge 2 commits into from
Closed

Debugframesize #2625

wants to merge 2 commits into from

Conversation

adinn
Copy link
Collaborator

@adinn adinn commented Jun 29, 2020

This PR fixes issue #2624. The changes in the compiler code ensure that a Frame.returned() callback occurs at the end of any basic block that also includes a call to Frame.leave(). This is needed to ensure that marks inserted by the Substrate implementors of Frame are correctly located. Substrate Frame.leave implementations still add marks for EPILOGUE_START and EPILOGUE_INCD_RSP. Generation of EPILOGUE_END occurs in Substrate Frame.returned implementations i.e. at the end of the basic block terminated by the return (or, in certain cases, a method exiting jump to a handler). The current code places the EPILOGUE_END mark before the terminating return (or jump). In deed, in some cases it also precedes safepoint code or other code that operates with the empty frame.

The result of making this compiler change is to allow the debug info frame record generator to decrement the stack frame size at EPILOGUE_INCD_RSP but then restore it again at EPILOGUE_END if it finds there is code that follows the EPILOGUE_END.

@adinn
Copy link
Collaborator Author

adinn commented Jul 1, 2020

@olpaw @tkrodriguez Any chance of a review for this small patch?

@tkrodriguez
Copy link
Member

It looks ok to me. I'll run it through our internal gates today.

@adinn
Copy link
Collaborator Author

adinn commented Jul 1, 2020

Thanks Tom!

@olpaw
Copy link
Member

olpaw commented Jul 3, 2020

@adinn thanks for the fix. Unfortunately this breaks debug_frame generation of EE debuginfo. I haved to fix that first & then I can merge your PR. Sorry for the inconvenience.

@adinn
Copy link
Collaborator Author

adinn commented Jul 3, 2020

@olpaw No problem. Thanks for letting me know. I hope the compiler changes offer a better/simpler approach for the EE code to generate frame info.

graalvmbot pushed a commit that referenced this pull request Jul 6, 2020
@olpaw
Copy link
Member

olpaw commented Jul 7, 2020

Merged on master: 7b174f1

BTW, the test that failed and made me aware that I need to adjust EE debuginfo is a good one. You might want to add something similar on CE. It builds a simple image (that has methods with multiple exit points) and then runs that image with GDB in "assembly single stepping mode" (si) until it reaches the breakpoint for exit@plt / exit. After each si it dumps the stack with bt and searches for in ?? in the stacktrace. If it finds such a string the test fails (because it found a code location where getting a stack trace does not work).

@olpaw olpaw closed this Jul 7, 2020
@adinn
Copy link
Collaborator Author

adinn commented Jul 7, 2020

BTW, the test that failed and made me aware that I need to adjust EE debuginfo is a good one. You might want to add something similar on CE. It builds a simple image (that has methods with multiple exit points) and then runs that image with GDB in "assembly single stepping mode" (si) until it reaches the breakpoint for exit@plt / exit. After each si it dumps the stack with bt and searches for in ?? in the stacktrace. If it finds such a string the test fails (because it found a code location where getting a stack trace does not work).

Hmm, that's a very good idea! I'll develop a test along those lines.

@adinn adinn deleted the debugframesize branch February 12, 2021 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debug info frame size changes are not being recorded and processed correctly
5 participants