[libcxx-commits] [PATCH] D59694: [PPC64][libunwind] Fix r2 not properly restored

[Leandro Lupori via Phabricator via libcxx-commits]

luporl marked 4 inline comments as done.
luporl added a comment.

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.



================
Comment at: libunwind/src/DwarfInstructions.hpp:238
+      // then r2 was saved and needs to be restored
+      if (R::getArch() == REGISTERS_PPC64 && returnAddress != 0 &&
+          addressSpace.get32(returnAddress)
----------------
nemanjai wrote:
> 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);
> }
1. Yes, get32() is implemented with memcpy().
2. Nice, the code without macros looks better indeed. I've changed this in this revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59694