The implementation of AIX unwinder gets the return address from the link area of the stack frame of a function and uses the return address to walk up functions. However, when unwinding starts from a signal handler and the function that raised the signal happens to be a leaf function and it does not have its own stack frame, the return address of the stack frame of the leaf function points to the caller of the function that calls the leaf function because the leaf function and its caller share the same stack frame. As a result, the caller of the leaf function is skipped. This patch fixes the problem by saving the LR value in sigcontext when the unwinder hits the signal handler trampoline frame and using it as the return address of the leaf function. The LR value from sigcontext is saved in the unwinding context slot for LR currently unused.
Details
- Reviewers
MaskRay compnerd hubert.reinterpretcast stephenpeckham daltenty - Group Reviewers
Restricted Project - Commits
- rG45d151138008: [libunwind][AIX] Fix problem with stepping up from a leaf function when…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libunwind/src/UnwindCursor.hpp | ||
---|---|---|
2406 | Not all branches modify the link register. |
libunwind/src/UnwindCursor.hpp | ||
---|---|---|
2406 | Fixed the comment, thanks! |
libunwind/src/UnwindCursor.hpp | ||
---|---|---|
2334–2336 | This only covers the case when stores_bc is 1 in the traceback table. | |
2409 | The comment should say nothing about buying a stack frame. The link area is usable by a function for saving the return address even when said function does not increment the SP. As @stephenpeckham pointed out "offline", there can be even be non-leaf functions that do not buy a stack frame (e.g., because the functions they call are known to not write to the stack). | |
2415 | Might be good to explicitly set returnAddress to 0 if lastStack is 0. That is, make this else if (lastStack). | |
2415–2417 | https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements now states that braces should be used for each part of an if/else chain when one part needs braces. | |
2509–2510 | If lastStack truly "points to the stack frame of the next routine up" (as claimed where it is declared), then it is fine to restore the SP even given a leaf function. |
Addressed comments:
- removed mentioning of buying stack frame in a code comment
- added if (!lastStack) and under the condition, set returnAddress to NULL
- removed condition TBTable->tb.saves_lr || !lastStack when restoring SP
- added code to initialize the LR value in context to 0
libunwind/src/UnwindCursor.hpp | ||
---|---|---|
2409 | Removed the mentioning of buying a stack frame, thanks! | |
2415 | Changed as suggested, thanks! | |
2415–2417 | Changed as suggested, thanks! | |
2509–2510 | I agree. Removed those conditions, thanks! |
libunwind/src/UnwindCursor.hpp | ||
---|---|---|
2511 | I think the check should be for TBTable->tb.stores_bc. It is possible for stores_bc to be 0 but saves_lr to be 1. This case can occur if a "leaf" function calls an internal routine that the compiler knows is stackless. In this case, the LR is saved in the stack before calling the internal function, but the stack pointer is not updated. | |
2543 | This code wasn't changed, but the check could be done at the beginning of the function instead of the end. With this change, isSignalFrame does not need to be updated. |
libunwind/src/UnwindCursor.hpp | ||
---|---|---|
2334–2336 | @xingxue, is a change coming for this? I think this also falls into "cases that arise when unwinding past a signal handler". | |
2511 | I don't think there should a condition here at all. A fix needs to be made at line 2324 (otherwise the wrong link area would be used for retrieving a saved LR when stores_bc is 0 and saves_lr is 1). |
libunwind/src/UnwindCursor.hpp | ||
---|---|---|
2334–2336 | Yes, @stephenpeckham has something that deals with it. I will refresh once that is finalized. |
libunwind/src/UnwindCursor.hpp | ||
---|---|---|
2543 | isSignalFrame needs to be undated for libunwind call unw_is_signal_frame(). |
Addressed comments.
- add code to handle the case where a function does not store the back chain (stores_bc is 0)
- swap the order of getting sigcontext from a signal trampoline frame, now trying stack + STKMINALIGN first and then stack + STKMIN
- remove the condition check when restoring the SP
- add the test case for the case where a function does not store the back chain
libunwind/src/UnwindCursor.hpp | ||
---|---|---|
2334–2336 | Incorporated @stephenpeckham's fix and use the current stack as the lastStack if stores_bc is 0. Thanks, Steve! | |
2511 |
Code has been added for the case where stores_bc is 0. Removed the condition check here. Thanks! | |
2511 |
Code has been added for the case where stores_bc is 0, thanks! |
This only covers the case when stores_bc is 1 in the traceback table.