This is an archive of the discontinued LLVM Phabricator instance.

[PPC64][libunwind] Fix r2 not properly restored
ClosedPublic

Authored by luporl on Mar 22 2019, 6:53 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

luporl created this revision.Mar 22 2019, 6:53 AM
luporl edited the summary of this revision. (Show Details)
luporl added subscribers: lbianc, adalava.
compnerd accepted this revision.Mar 22 2019, 2:19 PM

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);
...
This revision is now accepted and ready to land.Mar 22 2019, 2:19 PM

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);
}
ldionne resigned from this revision.Mar 25 2019, 5:49 AM
luporl updated this revision to Diff 192895.Mar 29 2019, 12:59 PM
  • Addressed review comments
  • Fixed ELFv1 ASM functions
luporl marked 4 inline comments as done.Mar 29 2019, 1:11 PM

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 added a subscriber: markmi.Mar 30 2019, 12:32 PM
This comment was removed by markmi.
nemanjai accepted this revision.Mar 31 2019, 5:31 AM

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 updated this revision to Diff 193591.Apr 3 2019, 2:03 PM
luporl marked an inline comment as done.
  • Test libunwind exception handling fix for PPC64
luporl edited the summary of this revision. (Show Details)Apr 3 2019, 2:45 PM
emaste added a subscriber: emaste.Apr 5 2019, 2:49 PM

@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?

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.

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

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

luporl updated this revision to Diff 196628.Apr 25 2019, 7:13 AM

Rebasing with latest master

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.

Any update on this?

Any update on this?

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

mclow.lists accepted this revision.May 13 2019, 11:39 AM

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

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?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 11:48 PM