This is an archive of the discontinued LLVM Phabricator instance.

[ARM64] [Windows] Add unwind support to llvm-readobj
ClosedPublic

Authored by ssijaric on Oct 14 2018, 8:02 PM.

Details

Summary

This patch adds support for dumping the unwind info from ARM64 COFF object files. This is required so that we can add test cases to https://reviews.llvm.org/D50166 (MCLayer support for ARM64 exception handling). Only unpacked unwind data is supported, as that is what D50166 currently supports.

I tested this with objects produced by both cl.exe and clang that includes the Windows ARM64 exception handling patches. The output that is produced looks like:

Case 1: single epilogue
File: test.obj
Format: COFF-ARM64
Arch: aarch64
AddressSize: 64bit
UnwindInformation [

RuntimeFunction {
  Function: $LN6 (0x0)
  ExceptionRecord: $unwind$?func@@YAHH@Z (0x0)
  ExceptionData {
    FunctionLength: 340
    Version: 0
    ExceptionData: No
    EpiloguePacked: Yes
    EpilogueOffset: 15
    ByteCodeLength: 28
    Prologue [
      0xe002dac8          ; sub sp, #2993280
      0xe3                ; nop
      0xe3                ; nop
      0xe3                ; nop
      0xd885              ; stp d10, d11, [sp, #40]
      0xd803              ; stp d8, d9, [sp, #24]
      0xd2c2              ; str x30, [sp, #16]
      0x28                ; stp x19, x20, [sp, #-64]!
      0xe4                ; end
    ]
    Epilogue [
      0xe002dac8          ; add sp, #2993280
      0xd885              ; ldp d10, d11, [sp, #40]
      0xd803              ; ldp d8, d9, [sp, #24]
      0xd2c2              ; ldr x30, [sp, #16]
      0x28                ; ldp x19, x20, [sp], #64
      0xe4                ; end
    ]
  }
}

]

Case 2: multiple epilogues
File: test1.obj
Format: COFF-ARM64
Arch: aarch64
AddressSize: 64bit
UnwindInformation [

RuntimeFunction {
  Function: $LN6 (0x0)
  ExceptionRecord: $unwind$?func@@YAHH@Z (0x0)
  ExceptionData {
    FunctionLength: 72
    Version: 0
    ExceptionData: No
    EpiloguePacked: No
    EpilogueScopes: 2
    ByteCodeLength: 8
    Prologue [
      0x1f                ; sub sp, #496
      0xd600              ; stp x19, lr, [sp, #0]
      0x01                ; sub sp, #16
      0xe4                ; end
    ]
    EpilogueScopes [
      EpilogueScope {
        StartOffset: 8
        EpilogueStartIndex: 0
        Opcodes [
          0x1f                ; add sp, #496
          0xd600              ; ldp x19, lr, [sp, #0]
          0x01                ; add sp, #16
          0xe4                ; end
        ]
      }
      EpilogueScope {
        StartOffset: 14
        EpilogueStartIndex: 0
        Opcodes [
          0x1f                ; add sp, #496
          0xd600              ; ldp x19, lr, [sp, #0]
          0x01                ; add sp, #16
          0xe4                ; end
        ]
      }
    ]
  }
}

]

Testing uncovered an encoding issue for the large stack size unwind opcode in D50166. I will update that patch with the fix and test cases that rely on this patch.

Diff Detail

Repository
rL LLVM

Event Timeline

ssijaric created this revision.Oct 14 2018, 8:02 PM

This probably needs at least a few testcases... not so much to duplicate the tests you're going to add for exception handling emission, but rather to make sure the error handling works correctly.

include/llvm/Support/ARMWinEH.h
349 ↗(On Diff #169642)

Using a template here is not helping make the code more readable.

Maybe there should be one function for both ARM and AArch64 that takes a reference to the ExceptionDataRecord?

tools/llvm-readobj/ARMWinEHPrinter.cpp
788 ↗(On Diff #169642)

Not new with your patch, but we need real error handling here, in case of an unknown opcode, or a multi-byte opcode which would run past the end of the opcode array.

804 ↗(On Diff #169642)

Missing bounds check? There must be at least 8 bytes of data in the section after the offset, and you need to check the total number of bytes specified by the header vs. the remaining bytes in the section.

807 ↗(On Diff #169642)

Probably makes sense to add ExceptionDataRecord::FunctionLengthInBytes(), or something like that.

820 ↗(On Diff #169642)

Not really part of your patch, but I'm not sure why we're checking XData.F() here. Maybe just leave a FIXME.

829 ↗(On Diff #169642)

"if (true)"?

1044 ↗(On Diff #169642)

It's okay not to implement this for now, but it should probably do something other than crash (e.g. print a message that packed unwind is not yet implemented).

ssijaric added inline comments.Oct 15 2018, 4:52 PM
tools/llvm-readobj/ARMWinEHPrinter.cpp
788 ↗(On Diff #169642)

I will change this to print something like "bad opcode" and continue scanning.

804 ↗(On Diff #169642)

Will add a check that the .xdata must be at least 8 bytes.

820 ↗(On Diff #169642)

The check for F() is applicable to ARM only, and indicates that there is no prologue (e.g. a function fragment) So !XData.F() indicates that there is a prologue, and we dump the opcodes for ARM. Not used for AArch64.

829 ↗(On Diff #169642)

Yes, otherwise ListScope will not align the prologue printing with epilogue printing when there multiple epilogues.

1044 ↗(On Diff #169642)

Ok, will do that.

ssijaric updated this revision to Diff 169904.Oct 16 2018, 3:37 PM

Addressed Eli's comments.

Added two obj files produced by cl.exe for testing purposes.

Still need to add a test case or two to check the error handling that was added in this patch.

efriedma added inline comments.
tools/llvm-readobj/ARMWinEHPrinter.cpp
788 ↗(On Diff #169904)

Ring64

790 ↗(On Diff #169904)

80 columns

791 ↗(On Diff #169904)

break;

836 ↗(On Diff #169904)

I thought you implemented this...? At least at first glance, ExceptionDataRecord tries to do the right thing.

840 ↗(On Diff #169904)

I think the subtraction here can overflow.

ssijaric added inline comments.Oct 16 2018, 4:38 PM
tools/llvm-readobj/ARMWinEHPrinter.cpp
836 ↗(On Diff #169904)

I don't see that this is supported in the current implementation of llvm-readobj. I don't plan to add it right now.

It is supported in the MCLayer part, however. It needs testing, but is not part of this patch.

ssijaric added inline comments.Oct 16 2018, 4:53 PM
tools/llvm-readobj/ARMWinEHPrinter.cpp
836 ↗(On Diff #169904)

It is actually supported, thanks for pointing it out.

ssijaric updated this revision to Diff 170042.Oct 17 2018, 6:22 PM

Fix bugs and add two assembly test cases constructed by hand to test the error handling.

I think this is looking good; hopefully Reid will get a chance to look briefly to make sure this matches what he was expecting.

tools/llvm-readobj/ARMWinEHPrinter.cpp
785 ↗(On Diff #170042)

This can still read a few bytes past the end of the unwind data if, for example, the last byte is 0xe0.

833 ↗(On Diff #170042)

Needs another 8 bytes if X() is set.

The literal "4" for the header should be 4 * HeaderWords().

ssijaric updated this revision to Diff 170124.Oct 18 2018, 2:19 PM

Address Eli's latest feedback, and an add an additional test case.

efriedma added inline comments.Oct 19 2018, 11:50 AM
tools/llvm-readobj/ARMWinEHPrinter.cpp
789 ↗(On Diff #170124)

It's wrong to read past the end of the section, and then print an error afterwards, rather than checking before you read the data; you can't print an error after llvm-readobj crashes. (You're only reading a few bytes past the end of the section, so you can get away with it sometimes, but not in general.)

Maybe the RingEntry should contain the opcode size.

ssijaric added inline comments.Oct 19 2018, 12:42 PM
tools/llvm-readobj/ARMWinEHPrinter.cpp
789 ↗(On Diff #170124)

I will change it, but I also have to account for the ARM opcode length as well then. This can just introduce more problems if I get the unwind length wrong. Error handling in llvm-readobj is not the intent of this particular patch.

efriedma added inline comments.Oct 19 2018, 1:08 PM
tools/llvm-readobj/ARMWinEHPrinter.cpp
789 ↗(On Diff #170124)

If you want to add a FIXME and leave it for a followup, that's okay, I think.

ssijaric updated this revision to Diff 170253.Oct 19 2018, 2:30 PM

Better error handling. Fix alloc_m decoding.

ssijaric updated this revision to Diff 170266.Oct 19 2018, 3:40 PM

Also fix a few unwind code hex dumps. Wasn't printing all the bytes.

ssijaric updated this revision to Diff 170352.Oct 21 2018, 1:24 PM

Fix a few operator precedence bugs uncovered while writing test cases for the MCLayer patch (D50166).

rnk accepted this revision.Oct 22 2018, 10:43 AM

I'm happy with this if @efriedma is happy with this, it looks like he thoroughly reviewed it. Sorry for the delay, I was at the dev meeting last week.

tools/llvm-readobj/ARMWinEHPrinter.cpp
119 ↗(On Diff #170352)

Hah. Thanks for using symbolic names. :)

This revision is now accepted and ready to land.Oct 22 2018, 10:43 AM
efriedma accepted this revision.Oct 22 2018, 12:47 PM

LGTM, with a couple minor comments.

tools/llvm-readobj/ARMWinEHPrinter.cpp
746 ↗(On Diff #170352)

Please remove this assertion; even if it isn't valid, it shouldn't crash. Same for addfp. (I guess you could add an error message to the output, but that doesn't seem too important.)

801 ↗(On Diff #170352)

Please make sure this isn't reading past the end of the Ring64 array. (It looks like it might read one element past the end?)

dmajor added a subscriber: dmajor.Oct 22 2018, 2:47 PM
ssijaric updated this revision to Diff 170529.Oct 22 2018, 5:13 PM

Address Eli's latest comments.

ssijaric updated this revision to Diff 170534.Oct 22 2018, 5:34 PM

Fix the printing of save_freg and save_freg_x that Eli just pointed out.

This revision was automatically updated to reflect the committed changes.