[PPC64][libunwind] Fix r2 not properly restored

[luporl]

Summary

This change makes each unwind step inspect the instruction at the
return address and, if needed, read r2 from its saved location and
modify the context appropriately.

The unwind logic is able to handle both ELFv1 and ELFv2 stacks.

Reported by Bug 41050

[compnerd]

LG with the minor changes requested.

libunwind/src/DwarfInstructions.hpp

244 ↗

(On Diff #191875)

Could you please expand the comment above to explain that we are checking explicit displacements in the encoding because of parameter spill area (32-byte in ELF ABIv2, so offset is 24, 48-byte in ELF ABIv1, so offset is 40 bytes).

253 ↗

(On Diff #191875)

I think it would be nicer if you hoisted the constants out:

#if _CALL_ELF-0 == 2
#define PPC64_R2_LOAD_INST_ENCODING 0xe8410018
#define PPC64_R2_OFFSET 24
#else
#define PPC64_R2_LOAD_INST_ENCODING 0xe8410028
#define PPC64_R2_OFFSET 40
#endif

...
  pint_t r2 = addressSpace.get64(sp + PPC64_R2_OFFSET);
...

[nemanjai]

Also, is it possible to have a test case for this? I don't actually know what libunwind test cases look like, but seems pertinent.

libunwind/src/DwarfInstructions.hpp

238 ↗

(On Diff #191875)

If I follow this correctly, this will just look at the return address and look at the 4 bytes at the address (i.e. the TOC restore that the linker filled in). This seems perfectly reasonable to me but I have a couple of questions:

1. Does get32() account for endianness correctly?
2. Why the macro checks at all?

For 2. what I am suggesting is something like:

// Look for the TOC restore at the return address.
if (R::getArch() == REGISTERS_PPC64 && returnAddress != 0) {
  pint_t sp = addressSpace.get64(sp + 24);
  pint_t r2 = 0;
  switch (addressSpace.get32(returnAddress)) {
    case 0xe8410018: // ld r2,24(r1) - ELFv2
      pint_t r2 = addressSpace.get64(sp + 24);
      break;
    case 0xe8410028: // ld r2,40(r1) - ELFv1
      r2 = addressSpace.get64(sp + 40);
      break;
  }
  if (r2)
    newRegisters.setRegister(UNW_PPC64_R2, r2);
}

[luporl]

• Addressed review comments
• Fixed ELFv1 ASM functions

[luporl]

Sorry for the delay, I ran into some issues.
This change addresses the review comments.
It also fixes assembly functions when targeting PowerPC64 with the ELFv1 ABI, as it requires an entry in .opd section for each function.
The compiler usually does this, but it must be inserted by hand for assembly functions.

I still need to check if/how to make a test case for this change.

libunwind/src/DwarfInstructions.hpp

238 ↗

(On Diff #191875)

1. Yes, get32() is implemented with memcpy().
2. Nice, the code without macros looks better indeed. I've changed this in this revision.

[markmi]

This comment was removed by markmi.

[nemanjai]

I don't really know much about how libunwind is tested so I can't really comment on how to write one (or the need for one). But the code looks good to me.

[luporl]

• Test libunwind exception handling fix for PPC64

[luporl]

@nemanjai, I've included a test case in the last revision. Do you think this is ok now?
Also, if there are no further issues, could you please check this in for me?

[nemanjai]

I like the patch now and the test case seems to cover what it needs to (I like the indirect call to ensure a call through a global entry point trick).

I have never committed anything to libunwind, but I suppose now that it is part of the monorepo, it shouldn't be a problem. I'll try to get it committed tomorrow.

[mstorsjo]

Can @mclow.lists or @EricWF give this an ack before it is committed? (@ldionne resigned as reviewer from this one already.)

[nemanjai]

Sorry, could you please rebase to latest? It does not apply cleanly for me (assembly.h does not apply).

[luporl]

Rebasing with latest master

[luporl]

In D59694#1476888, @nemanjai wrote:

Sorry, could you please rebase to latest? It does not apply cleanly for me (assembly.h does not apply).

Ok, it should apply cleanly now.

[luporl]

Any update on this?

[mstorsjo]

In D59694#1498186, @luporl wrote:

Any update on this?

Iirc @mclow.lists is away this week, dunno about @EricWF, but their approval is needed.

[mclow.lists]

This looks reasonable to me (but my PPC-foo is pretty rusty).

[luporl]

Thanks for the reviews @compnerd, @nemanjai, and @mclow.lists.
As I don't have permission to commit this to LLVM repository, can anyone commit this change for me?

libunwind/trunk/src/DwarfInstructions.hpp

#if defined(_LIBUNWIND_TARGET_SPARC)
         // Skip call site instruction and delay slot
         returnAddress += 8;
         // Skip unimp instruction if function returns a struct
         if ((addressSpace.get32(returnAddress) & 0xC1C00000) == 0)
           returnAddress += 4;
       }
 #endif
 
+#if defined(_LIBUNWIND_TARGET_PPC64)
+#define PPC64_ELFV1_R2_LOAD_INST_ENCODING 0xe8410028u // ld r2,40(r1)
+#define PPC64_ELFV1_R2_OFFSET 40
+#define PPC64_ELFV2_R2_LOAD_INST_ENCODING 0xe8410018u // ld r2,24(r1)
+#define PPC64_ELFV2_R2_OFFSET 24
+      // If the instruction at return address is a TOC (r2) restore,
+      // then r2 was saved and needs to be restored.
+      // ELFv2 ABI specifies that the TOC Pointer must be saved at SP + 24,
+      // while in ELFv1 ABI it is saved at SP + 40.
+      if (R::getArch() == REGISTERS_PPC64 && returnAddress != 0) {
+        pint_t sp = newRegisters.getRegister(UNW_REG_SP);
+        pint_t r2 = 0;
+        switch (addressSpace.get32(returnAddress)) {
+        case PPC64_ELFV1_R2_LOAD_INST_ENCODING:
+          r2 = addressSpace.get64(sp + PPC64_ELFV1_R2_OFFSET);
+          break;
+        case PPC64_ELFV2_R2_LOAD_INST_ENCODING:
+          r2 = addressSpace.get64(sp + PPC64_ELFV2_R2_OFFSET);
+          break;
+        }
+        if (r2)
+          newRegisters.setRegister(UNW_PPC64_R2, r2);
+      }
+#endif
+
       // Return address is address after call site instruction, so setting IP to
       // that does simualates a return.
       newRegisters.setIP(returnAddress);
 
       // Simulate the step by replacing the register set with the new ones.
       registers = newRegisters;
 
       return UNW_STEP_SUCCESS;

libunwind/trunk/src/assembly.h

 #elif defined(__POWERPC__) || defined(__powerpc__) || defined(__ppc__)
 #define SEPARATOR @
 #elif defined(__arm64__)
 #define SEPARATOR %%
 #else
 #define SEPARATOR ;
 #endif
 
+#if defined(__powerpc64__) && (!defined(_CALL_ELF) || _CALL_ELF == 1)
+#define PPC64_OPD1 .section .opd,"aw",@progbits SEPARATOR
+#define PPC64_OPD2 SEPARATOR \
+  .p2align 3 SEPARATOR \
+  .quad .Lfunc_begin0 SEPARATOR \
+  .quad .TOC.@tocbase SEPARATOR \
+  .quad 0 SEPARATOR \
+  .text SEPARATOR \
+.Lfunc_begin0:
+#else
+#define PPC64_OPD1
+#define PPC64_OPD2
+#endif
+
 #define GLUE2(a, b) a ## b
 #define GLUE(a, b) GLUE2(a, b)
 #define SYMBOL_NAME(name) GLUE(__USER_LABEL_PREFIX__, name)
 
 #if defined(__APPLE__)
 
 #define SYMBOL_IS_FUNC(name)
 #define EXPORT_SYMBOL(name)

 #error Unsupported target
 
 #endif
 
 #define DEFINE_LIBUNWIND_FUNCTION(name)                                        \
   .globl SYMBOL_NAME(name) SEPARATOR                                           \
   HIDDEN_SYMBOL(SYMBOL_NAME(name)) SEPARATOR                                   \
   SYMBOL_IS_FUNC(SYMBOL_NAME(name)) SEPARATOR                                  \
-  SYMBOL_NAME(name):
+  PPC64_OPD1                                                                   \
+  SYMBOL_NAME(name):                                                           \
+  PPC64_OPD2
 
 #if defined(__arm__)
 #if !defined(__ARM_ARCH)
 #define __ARM_ARCH 4
 #endif
 
 #if defined(__ARM_ARCH_4T__) || __ARM_ARCH >= 5
 #define ARM_HAS_BX

libunwind/trunk/test/lit.cfg

 config.name = 'libunwind'
 
 # suffixes: A list of file extensions to treat as test files.
 config.suffixes = ['.cpp', '.s']
 
 # test_source_root: The root path where tests are located.
 config.test_source_root = os.path.dirname(__file__)
 
+# needed to test libunwind with code that throws exceptions
+config.enable_exceptions = True
+
 # Infer the libcxx_test_source_root for configuration import.
 # If libcxx_source_root isn't specified in the config, assume that the libcxx
 # and libunwind source directories are sibling directories.
 libcxx_src_root = getattr(config, 'libcxx_src_root', None)
 if not libcxx_src_root:
     libcxx_src_root = os.path.join(config.test_source_root, '../../libcxx')
 libcxx_test_src_root = os.path.join(libcxx_src_root, 'utils')
 if os.path.isfile(os.path.join(libcxx_test_src_root, 'libcxx', '__init__.py')):