[libcxx-commits] [PATCH] D158655: [libunwind][AIX] Fix problem with stepping up from a leaf function when unwinding started in a signal handler

[Xing Xue via Phabricator via libcxx-commits]

xingxue added inline comments.


================
Comment at: libunwind/src/UnwindCursor.hpp:2504
+    // sigcontext is used as the return address.
+    if (!usedLRFromSigContext)
+      newRegisters.setSP(lastStack);
----------------
hubert.reinterpretcast wrote:
> 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`).
> 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!


================
Comment at: libunwind/src/UnwindCursor.hpp:2324
   // lastStack points to the stack frame of the next routine up.
   pint_t lastStack = *(reinterpret_cast<pint_t *>(registers.getSP()));
 
----------------
xingxue wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > This only covers the case when `stores_bc` is `1` in the traceback table.
> > @xingxue, is a change coming for this? I think this also falls into "cases that arise when unwinding past a signal handler".
> Yes, @stephenpeckham has something that deals with it. I will refresh once that is finalized.
Incorporated @stephenpeckham's fix and use the current stack as the `lastStack` if `stores_bc` is 0. Thanks, Steve!


================
Comment at: libunwind/src/UnwindCursor.hpp:2503
+    // or the function is '__start', or LR is not set in the current context.
+    if (TBTable->tb.saves_lr || !lastStack || !sigContextLRValue)
+      newRegisters.setSP(lastStack);
----------------
stephenpeckham wrote:
> 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. 
> 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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158655/new/

https://reviews.llvm.org/D158655