[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:2398
+      // sigcontext no longer points to the caller of the leaf function.
+      // However, in this case the compiler will buy a stack frame and save
+      // the original LR value in the link area and therefore, the LR from
----------------
hubert.reinterpretcast wrote:
> 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).
Removed the mentioning of buying a stack frame, thanks!


================
Comment at: libunwind/src/UnwindCursor.hpp:2404
+                                 reinterpret_cast<void *>(returnAddress));
+    } else
+      // Otherwise, use the LR value in the stack link area.
----------------
xingxue wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > 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.
> > Might be good to explicitly set `returnAddress` to `0` if `lastStack` is `0`. That is, make this `else if (lastStack)`.
> Changed as suggested, thanks!
Changed as suggested, thanks!


================
Comment at: libunwind/src/UnwindCursor.hpp:2404-2406
+    } else
+      // Otherwise, use the LR value in the stack link area.
+      returnAddress = reinterpret_cast<pint_t *>(lastStack)[2];
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > 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.
> Might be good to explicitly set `returnAddress` to `0` if `lastStack` is `0`. That is, make this `else if (lastStack)`.
Changed as suggested, thanks!


================
Comment at: libunwind/src/UnwindCursor.hpp:2501-2504
+    // Restore the SP and move one frame up if it is non-leaf function,
+    // or the function is '__start', or LR is not set in the current context.
+    if (TBTable->tb.saves_lr || !lastStack || !sigContextLRValue)
+      newRegisters.setSP(lastStack);
----------------
hubert.reinterpretcast wrote:
> 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.
I agree. Removed those conditions, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158655