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

[xingxue]

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.

[hubert.reinterpretcast]

libunwind/src/UnwindCursor.hpp

2395

Not all branches modify the link register.

[xingxue]

Addressed a comment:

• fix the comment - not all branches change LR.

[xingxue]

libunwind/src/UnwindCursor.hpp

2395

Fixed the comment, thanks!

[hubert.reinterpretcast]

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]

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!

[stephenpeckham]

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.

[hubert.reinterpretcast]

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]

libunwind/src/UnwindCursor.hpp

2324

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

[xingxue]

libunwind/src/UnwindCursor.hpp

2538

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

[xingxue]

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]

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]

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]

Addressed comments:

• add instruction bl $+4 in the assembly code of the test case

[xingxue]

libunwind/test/aix_signal_unwind.pass.sh.S

160

Added the instruction as suggested, thanks!

209

Added the instruction as suggested, thanks!

[stephenpeckham]

Both the libunwind and testcase changes look good.

libunwind/src/Registers.hpp

   void      setIP(uint32_t value) { _registers.__srr0 = value; }
   uint64_t  getCR() const         { return _registers.__cr; }
   void      setCR(uint32_t value) { _registers.__cr = value; }
+  uint64_t  getLR() const         { return _registers.__lr; }
+  void      setLR(uint32_t value) { _registers.__lr = value; }
 
 private:
   struct ppc_thread_state_t {

   void      setIP(uint64_t value) { _registers.__srr0 = value; }
   uint64_t  getCR() const         { return _registers.__cr; }
   void      setCR(uint64_t value) { _registers.__cr = value; }
+  uint64_t  getLR() const         { return _registers.__lr; }
+  void      setLR(uint64_t value) { _registers.__lr = value; }
 
 private:
   struct ppc64_thread_state_t {

libunwind/src/UnwindCursor.hpp

     if (!getFunctionName(functionBuf, sizeof(functionBuf), &offset)) {
       functionName = ".anonymous.";
     }
-    _LIBUNWIND_TRACE_UNWINDING("%s: Look up traceback table of func=%s at %p",
-                               __func__, functionName,
-                               reinterpret_cast<void *>(TBTable));
+    _LIBUNWIND_TRACE_UNWINDING(
+        "%s: Look up traceback table of func=%s at %p, pc=%p, "
+        "SP=%p, saves_lr=%d, stores_bc=%d",
+        __func__, functionName, reinterpret_cast<void *>(TBTable),
+        reinterpret_cast<void *>(pc),
+        reinterpret_cast<void *>(registers.getSP()), TBTable->tb.saves_lr,
+        TBTable->tb.stores_bc);
   }
 
 #if defined(__powerpc64__)
-  // Instruction to reload TOC register "l r2,40(r1)"
+  // Instruction to reload TOC register "ld r2,40(r1)"
   const uint32_t loadTOCRegInst = 0xe8410028;
   const int32_t unwPPCF0Index = UNW_PPC64_F0;
   const int32_t unwPPCV0Index = UNW_PPC64_V0;
 #else
-  // Instruction to reload TOC register "l r2,20(r1)"
+  // Instruction to reload TOC register "lwz r2,20(r1)"
   const uint32_t loadTOCRegInst = 0x80410014;
   const int32_t unwPPCF0Index = UNW_PPC_F0;
   const int32_t unwPPCV0Index = UNW_PPC_V0;
 #endif
 
+  // lastStack points to the stack frame of the next routine up.
+  pint_t curStack = static_cast<pint_t>(registers.getSP());
+  pint_t lastStack = *reinterpret_cast<pint_t *>(curStack);
+
+  if (lastStack == 0)
+    return UNW_STEP_END;
+
   R newRegisters = registers;
 
-  // lastStack points to the stack frame of the next routine up.
-  pint_t lastStack = *(reinterpret_cast<pint_t *>(registers.getSP()));
+  // If backchain is not stored, use the current stack frame.
+  if (!TBTable->tb.stores_bc)
+    lastStack = curStack;
 
   // Return address is the address after call site instruction.
   pint_t returnAddress;

                                reinterpret_cast<void *>(lastStack));
 
     sigcontext *sigContext = reinterpret_cast<sigcontext *>(
-        reinterpret_cast<char *>(lastStack) + STKMIN);
+        reinterpret_cast<char *>(lastStack) + STKMINALIGN);
     returnAddress = sigContext->sc_jmpbuf.jmp_context.iar;
 
-    _LIBUNWIND_TRACE_UNWINDING("From sigContext=%p, returnAddress=%p\n",
-                               reinterpret_cast<void *>(sigContext),
-                               reinterpret_cast<void *>(returnAddress));
-
+    bool useSTKMIN = false;
     if (returnAddress < 0x10000000) {
-      // Try again using STKMINALIGN
+      // Try again using STKMIN.
       sigContext = reinterpret_cast<sigcontext *>(
-          reinterpret_cast<char *>(lastStack) + STKMINALIGN);
+          reinterpret_cast<char *>(lastStack) + STKMIN);
       returnAddress = sigContext->sc_jmpbuf.jmp_context.iar;
       if (returnAddress < 0x10000000) {
-        _LIBUNWIND_TRACE_UNWINDING("Bad returnAddress=%p\n",
-                                   reinterpret_cast<void *>(returnAddress));
+        _LIBUNWIND_TRACE_UNWINDING("Bad returnAddress=%p from sigcontext=%p",
+                                   reinterpret_cast<void *>(returnAddress),
+                                   reinterpret_cast<void *>(sigContext));
         return UNW_EBADFRAME;
-      } else {
-        _LIBUNWIND_TRACE_UNWINDING("Tried again using STKMINALIGN: "
-                                   "sigContext=%p, returnAddress=%p. "
-                                   "Seems to be a valid address\n",
-                                   reinterpret_cast<void *>(sigContext),
-                                   reinterpret_cast<void *>(returnAddress));
       }
+      useSTKMIN = true;
     }
+    _LIBUNWIND_TRACE_UNWINDING("Returning from a signal handler %s: "
+                               "sigContext=%p, returnAddress=%p. "
+                               "Seems to be a valid address",
+                               useSTKMIN ? "STKMIN" : "STKMINALIGN",
+                               reinterpret_cast<void *>(sigContext),
+                               reinterpret_cast<void *>(returnAddress));
+
     // Restore the condition register from sigcontext.
     newRegisters.setCR(sigContext->sc_jmpbuf.jmp_context.cr);
 
+    // Save the LR in sigcontext for stepping up when the function that
+    // raised the signal is a leaf function. This LR has the return address
+    // to the caller of the leaf function.
+    newRegisters.setLR(sigContext->sc_jmpbuf.jmp_context.lr);
+    _LIBUNWIND_TRACE_UNWINDING(
+        "Save LR=%p from sigcontext",
+        reinterpret_cast<void *>(sigContext->sc_jmpbuf.jmp_context.lr));
+
     // Restore GPRs from sigcontext.
     for (int i = 0; i < 32; ++i)
       newRegisters.setRegister(i, sigContext->sc_jmpbuf.jmp_context.gpr[i]);

     }
   } else {
     // Step up a normal frame.
-    returnAddress = reinterpret_cast<pint_t *>(lastStack)[2];
 
-    _LIBUNWIND_TRACE_UNWINDING("Extract info from lastStack=%p, "
-                               "returnAddress=%p\n",
-                               reinterpret_cast<void *>(lastStack),
-                               reinterpret_cast<void *>(returnAddress));
-    _LIBUNWIND_TRACE_UNWINDING("fpr_regs=%d, gpr_regs=%d, saves_cr=%d\n",
+    if (!TBTable->tb.saves_lr && registers.getLR()) {
+      // This case should only occur if we were called from a signal handler
+      // and the signal occurred in a function that doesn't save the LR.
+      returnAddress = registers.getLR();
+      _LIBUNWIND_TRACE_UNWINDING("Use saved LR=%p",
+                                 reinterpret_cast<void *>(returnAddress));
+    } else {
+      // Otherwise, use the LR value in the stack link area.
+      returnAddress = reinterpret_cast<pint_t *>(lastStack)[2];
+    }
+
+    // Reset LR in the current context.
+    newRegisters.setLR(NULL);
+
+    _LIBUNWIND_TRACE_UNWINDING(
+        "Extract info from lastStack=%p, returnAddress=%p",
+        reinterpret_cast<void *>(lastStack),
+        reinterpret_cast<void *>(returnAddress));
+    _LIBUNWIND_TRACE_UNWINDING("fpr_regs=%d, gpr_regs=%d, saves_cr=%d",
                                TBTable->tb.fpr_saved, TBTable->tb.gpr_saved,
                                TBTable->tb.saves_cr);
 

 
       struct vec_ext *vec_ext = reinterpret_cast<struct vec_ext *>(charPtr);
 
-      _LIBUNWIND_TRACE_UNWINDING("vr_saved=%d\n", vec_ext->vr_saved);
+      _LIBUNWIND_TRACE_UNWINDING("vr_saved=%d", vec_ext->vr_saved);
 
       // Restore vector register(s) if saved on the stack.
       if (vec_ext->vr_saved) {

 
     // Do we need to set the TOC register?
     _LIBUNWIND_TRACE_UNWINDING(
-        "Current gpr2=%p\n",
+        "Current gpr2=%p",
         reinterpret_cast<void *>(newRegisters.getRegister(2)));
     if (firstInstruction == loadTOCRegInst) {
       _LIBUNWIND_TRACE_UNWINDING(
-          "Set gpr2=%p from frame\n",
+          "Set gpr2=%p from frame",
           reinterpret_cast<void *>(reinterpret_cast<pint_t *>(lastStack)[5]));
       newRegisters.setRegister(2, reinterpret_cast<pint_t *>(lastStack)[5]);
     }

   } else {
     isSignalFrame = false;
   }
-
   return UNW_STEP_SUCCESS;
 }
 #endif // defined(_LIBUNWIND_SUPPORT_TBTAB_UNWIND)

libunwind/src/UnwindRegistersSave.S

   std   0,  PPC64_OFFS_CR(3)
   mfxer 0
   std   0,  PPC64_OFFS_XER(3)
+#if defined(_AIX)
+  // LR value saved from the register is not used, initialize it to 0.
+  li 0, 0
+#else
   mflr  0
+#endif
   std   0,  PPC64_OFFS_LR(3)
   mfctr 0
   std   0,  PPC64_OFFS_CTR(3)

   // save CR registers
   mfcr    0
   stw     0, 136(3)
+#if defined(_AIX)
+  // LR value from the register is not used, initialize it to 0.
+  li      0, 0
+  stw     0, 144(3)
+#endif
   // save CTR register
   mfctr   0
   stw     0, 148(3)

libunwind/test/aix_signal_unwind.pass.sh.S

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// Test that _Unwind_Backtrace() walks up from a signal handler and produces
+// a correct traceback when the function raising the signal does not save
+// the link register or does not store the stack back chain.
+
+// REQUIRES: target=powerpc{{(64)?}}-ibm-aix
+
+// Test when the function raising the signal does not save the link register
+// RUN: %{cxx} -x c++ %s -o %t.exe -DCXX_CODE %{flags} %{compile_flags}
+// RUN: %{exec} %t.exe
+
+// Test when the function raising the signal does not store stack back chain.
+// RUN: %{cxx} -x c++ -c %s -o %t1.o -DCXX_CODE -DNOBACKCHAIN %{flags} \
+// RUN:   %{compile_flags}
+// RUN: %{cxx} -c %s -o %t2.o %{flags} %{compile_flags}
+// RUN: %{cxx} -o %t1.exe %t1.o %t2.o %{flags} %{link_flags}
+// RUN: %{exec} %t1.exe
+
+#ifdef CXX_CODE
+
+#undef NDEBUG
+#include <assert.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/debug.h>
+#include <unwind.h>
+
+#define NAME_ARRAY_SIZE 10
+#define NAMES_EXPECTED   6
+
+const char* namesExpected[] = {"handler", "abc", "bar", "foo", "main",
+                               "__start"};
+char *namesObtained[NAME_ARRAY_SIZE] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+
+int funcIndex = 0;
+
+// Get the function name from traceback table.
+char *getFuncName(uintptr_t pc, uint16_t *nameLen) {
+  uint32_t *p = reinterpret_cast<uint32_t *>(pc);
+
+  // Keep looking forward until a word of 0 is found. The traceback
+  // table starts at the following word.
+  while (*p)
+    ++p;
+  tbtable *TBTable = reinterpret_cast<tbtable *>(p + 1);
+
+  if (!TBTable->tb.name_present)
+    return NULL;
+
+  // Get to the optional portion of the traceback table.
+  p = reinterpret_cast<uint32_t *>(&TBTable->tb_ext);
+
+  // Skip field parminfo if it exists.
+  if (TBTable->tb.fixedparms || TBTable->tb.floatparms)
+    ++p;
+
+  // Skip field tb_offset if it exists.
+  if (TBTable->tb.has_tboff)
+    ++p;
+
+  // Skip field hand_mask if it exists.
+  if (TBTable->tb.int_hndl)
+    ++p;
+
+  // Skip fields ctl_info and ctl_info_disp if they exist.
+  if (TBTable->tb.has_ctl)
+    p += 1 + *p;
+
+  *nameLen = *reinterpret_cast<uint16_t *>(p);
+  return reinterpret_cast<char *>(p) + sizeof(uint16_t);
+}
+
+_Unwind_Reason_Code callBack(struct _Unwind_Context *uc, void *arg) {
+  (void)arg;
+  uint16_t nameLen;
+  uintptr_t ip = _Unwind_GetIP(uc);
+  if (funcIndex < NAME_ARRAY_SIZE)
+    namesObtained[funcIndex++] = strndup(getFuncName(ip, &nameLen), nameLen);
+  return _URC_NO_REASON;
+}
+
+extern "C" void handler(int signum) {
+  (void)signum;
+  // Walk stack frames for traceback.
+  _Unwind_Backtrace(callBack, NULL);
+
+  // Verify the traceback.
+  assert(funcIndex <= NAMES_EXPECTED && "Obtained names more than expected");
+  for (int i = 0; i < funcIndex; ++i) {
+    assert(!strcmp(namesExpected[i], namesObtained[i]) &&
+           "Function names do not match");
+    free(namesObtained[i]);
+  }
+  exit(0);
+}
+
+#ifdef NOBACKCHAIN
+// abc() is in assembly. It raises signal SIGSEGV and does not store
+// the stack back chain.
+extern "C" void abc();
+
+#else
+volatile int *null = 0;
+
+// abc() raises signal SIGSEGV and does not save the link register.
+extern "C" __attribute__((noinline)) void abc() {
+  // Produce a SIGSEGV.
+  *null = 0;
+}
+#endif
+
+extern "C" __attribute__((noinline)) void bar() {
+  abc();
+}
+
+extern "C" __attribute__((noinline)) void foo() {
+  bar();
+}
+
+int main() {
+  // Set signal handler for SIGSEGV.
+  signal(SIGSEGV, handler);
+  foo();
+}
+
+#else // Assembly code for abc().
+// This assembly code is similar to the following C code but it saves the
+// link register.
+//
+// int *badp = 0;
+// void abc() {
+//   *badp = 0;
+// }
+
+#ifdef __64BIT__
+        .csect [PR],5
+        .file   "inline.c","IBM Open XL C/C++ for AIX 17.1.2 (5725-C72, 5765-J18), version 17.1.2.0, LLVM version 18.0.0git"
+        .globl  abc[DS]                 # -- Begin function abc
+        .globl  .abc
+        .align  4
+        .csect abc[DS],3
+        .vbyte  8, .abc                 # @abc
+        .vbyte  8, TOC[TC0]
+        .vbyte  8, 0
+        .csect [PR],5
+.abc:
+# %bb.0:                                # %entry
+        mflr 0
+        std 0, 16(1)
+        ld 3, L..C0(2)                  # @badp
+        bl $+4
+        ld 4, 0(3)
+        li 3, 0
+        stw 3, 0(4)
+        ld 0, 16(1)
+        mtlr 0
+        blr
+L..abc0:
+        .vbyte  4, 0x00000000           # Traceback table begin
+        .byte   0x00                    # Version = 0
+        .byte   0x09                    # Language = CPlusPlus
+        .byte   0x20                    # -IsGlobaLinkage, -IsOutOfLineEpilogOrPrologue
+                                        # +HasTraceBackTableOffset, -IsInternalProcedure
+                                        # -HasControlledStorage, -IsTOCless
+                                        # -IsFloatingPointPresent
+                                        # -IsFloatingPointOperationLogOrAbortEnabled
+        .byte   0x61                    # -IsInterruptHandler, +IsFunctionNamePresent, +IsAllocaUsed
+                                        # OnConditionDirective = 0, -IsCRSaved, +IsLRSaved
+        .byte   0x00                    # -IsBackChainStored, -IsFixup, NumOfFPRsSaved = 0
+        .byte   0x01                    # -HasExtensionTable, -HasVectorInfo, NumOfGPRsSaved = 1
+        .byte   0x00                    # NumberOfFixedParms = 0
+        .byte   0x01                    # NumberOfFPParms = 0, +HasParmsOnStack
+        .vbyte  4, L..abc0-.abc         # Function size
+        .vbyte  2, 0x0003               # Function name len = 3
+        .byte   "abc"                   # Function Name
+        .byte   0x1f                    # AllocaUsed
+                                        # -- End function
+        .csect badp[RW],3
+        .globl  badp[RW]                # @badp
+        .align  3
+        .vbyte  8, 0
+        .toc
+L..C0:
+        .tc badp[TC],badp[RW]
+#else
+        .csect [PR],5
+        .file   "inline.c","IBM Open XL C/C++ for AIX 17.1.2 (5725-C72, 5765-J18), version 17.1.2.0, LLVM version 18.0.0git"
+        .globl  abc[DS]                 # -- Begin function abc
+        .globl  .abc
+        .align  4
+        .csect abc[DS],2
+        .vbyte  4, .abc                 # @abc
+        .vbyte  4, TOC[TC0]
+        .vbyte  4, 0
+        .csect [PR],5
+.abc:
+# %bb.0:                                # %entry
+        mflr 0
+        stw 0, 8(1)
+        lwz 3, L..C0(2)                 # @badp
+        bl $+4
+        lwz 4, 0(3)
+        li 3, 0
+        stw 3, 0(4)
+        lwz 0, 8(1)
+        mtlr 0
+        blr
+L..abc0:
+        .vbyte  4, 0x00000000           # Traceback table begin
+        .byte   0x00                    # Version = 0
+        .byte   0x09                    # Language = CPlusPlus
+        .byte   0x20                    # -IsGlobaLinkage, -IsOutOfLineEpilogOrPrologue
+                                        # +HasTraceBackTableOffset, -IsInternalProcedure
+                                        # -HasControlledStorage, -IsTOCless
+                                        # -IsFloatingPointPresent
+                                        # -IsFloatingPointOperationLogOrAbortEnabled
+        .byte   0x61                    # -IsInterruptHandler, +IsFunctionNamePresent, +IsAllocaUsed
+                                        # OnConditionDirective = 0, -IsCRSaved, +IsLRSaved
+        .byte   0x00                    # -IsBackChainStored, -IsFixup, NumOfFPRsSaved = 0
+        .byte   0x01                    # -HasExtensionTable, -HasVectorInfo, NumOfGPRsSaved = 1
+        .byte   0x00                    # NumberOfFixedParms = 0
+        .byte   0x01                    # NumberOfFPParms = 0, +HasParmsOnStack
+        .vbyte  4, L..abc0-.abc         # Function size
+        .vbyte  2, 0x0003               # Function name len = 3
+        .byte   "abc"                   # Function Name
+        .byte   0x1f                    # AllocaUsed
+                                        # -- End function
+        .csect badp[RW],2
+        .globl  badp[RW]                # @badp
+        .align  2
+        .vbyte  4, 0
+        .toc
+L..C0:
+        .tc badp[TC],badp[RW]
+#endif // __64BIT__
+#endif // CXX_CODE