This is an archive of the discontinued LLVM Phabricator instance.

[libunwind][AIX] Fix problem with stepping up from a leaf function when unwinding started in a signal handler
ClosedPublic

Authored by xingxue on Aug 23 2023, 11:49 AM.

Details

Summary

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.

Diff Detail

Event Timeline

xingxue created this revision.Aug 23 2023, 11:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 11:49 AM
xingxue requested review of this revision.Aug 23 2023, 11:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 11:49 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
xingxue edited the summary of this revision. (Show Details)Aug 23 2023, 12:39 PM
libunwind/src/UnwindCursor.hpp
2395

Not all branches modify the link register.

xingxue updated this revision to Diff 553072.Aug 24 2023, 4:18 AM

Addressed a comment:

  • fix the comment - not all branches change LR.
xingxue marked an inline comment as done.Aug 24 2023, 4:20 AM
xingxue added inline comments.
libunwind/src/UnwindCursor.hpp
2395

Fixed the comment, thanks!

xingxue marked an inline comment as done.Aug 24 2023, 4:21 AM
libunwind/src/UnwindCursor.hpp
2324

This only covers the case when stores_bc is 1 in the traceback table.

2398

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).

2404

Might be good to explicitly set returnAddress to 0 if lastStack is 0. That is, make this else if (lastStack).

2404–2406

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.

2501–2504

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.

xingxue updated this revision to Diff 553541.Aug 25 2023, 10:56 AM
xingxue marked 4 inline comments as done.

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
2398

Removed the mentioning of buying a stack frame, thanks!

2404

Changed as suggested, thanks!

2404–2406

Changed as suggested, thanks!

2501–2504

I agree. Removed those conditions, thanks!

libunwind/src/UnwindCursor.hpp
2503

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.

2538

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
2324

@xingxue, is a change coming for this? I think this also falls into "cases that arise when unwinding past a signal handler".

2504

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).

xingxue added inline comments.Aug 25 2023, 1:22 PM
libunwind/src/UnwindCursor.hpp
2324

Yes, @stephenpeckham has something that deals with it. I will refresh once that is finalized.

xingxue marked an inline comment as done.Aug 25 2023, 1:30 PM
xingxue added inline comments.
libunwind/src/UnwindCursor.hpp
2538

isSignalFrame needs to be undated for libunwind call unw_is_signal_frame().

xingxue updated this revision to Diff 554785.Aug 30 2023, 11:11 AM
xingxue marked 5 inline comments as done.

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
xingxue added inline comments.Aug 30 2023, 11:13 AM
libunwind/src/UnwindCursor.hpp
2324

Incorporated @stephenpeckham's fix and use the current stack as the lastStack if stores_bc is 0. Thanks, Steve!

2503

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.

Code has been added for the case where stores_bc is 0, thanks!

2504

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).

Code has been added for the case where stores_bc is 0. Removed the condition check here. Thanks!

stephenpeckham added inline comments.Aug 31 2023, 7:28 AM
libunwind/src/UnwindCursor.hpp
2538

Retracting this comment. The value of _isSignalFrame can be queried, so the value has to be computed here.

libunwind/test/aix_signal_unwind.pass.sh.S
160

I suggest you add "bl $+4" to modify LR as if an internal function had been called.

209

add "bl $+4"

xingxue updated this revision to Diff 555053.Aug 31 2023, 8:01 AM

Addressed comments:

  • add instruction bl $+4 in the assembly code of the test case
xingxue marked 3 inline comments as done.Aug 31 2023, 8:03 AM
xingxue added inline comments.
libunwind/test/aix_signal_unwind.pass.sh.S
160

Added the instruction as suggested, thanks!

209

Added the instruction as suggested, thanks!

stephenpeckham accepted this revision.Aug 31 2023, 12:25 PM

Both the libunwind and testcase changes look good.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 16 2023, 9:25 AM
This revision was automatically updated to reflect the committed changes.
xingxue marked 2 inline comments as done.