Page MenuHomePhabricator

[ARM64] [Windows] MCLayer support for exception handling
ClosedPublic

Authored by ssijaric on Aug 1 2018, 5:33 PM.

Details

Summary

This patch adds the ARM64 unwind opcodes to the MCLayer, as well as SEH directives that will be emitted by the frame lowering in a patch that will follow. This patch, in conjunction with the frame lowering patch, was tested on a couple of SPEC CPU2006 C++ benchmarks that make use of exception handling. We only emit the unwinding opcodes into object files for now.

Diff Detail

Repository
rL LLVM

Event Timeline

ssijaric created this revision.Aug 1 2018, 5:33 PM
ssijaric added inline comments.Aug 2 2018, 3:03 PM
lib/MC/MCAsmStreamer.cpp
1709 ↗(On Diff #158669)

Will turn asserts into errs, as there are existing test cases that require unwinding info, and will hit these asserts when generating .s files.

Perhaps I can create a MIR test case with the SEH opcodes, and use llvm-objdump to check the .xdata section?

This seems reasonable.

lib/MC/MCWin64EH.cpp
537 ↗(On Diff #158669)

Non-deterministic output, I think, since the keys of EpilogMap are pointers.

lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFStreamer.cpp
66 ↗(On Diff #158669)

Please commit separately? (Although, I'm not sure it matters: IIRC AArch64 assemblers can't relax anything anyway.)

ssijaric updated this revision to Diff 158864.Aug 2 2018, 5:39 PM

Use errs instead of assert MCAsmPrinter for unwind opcodes that are not yet supported. Address Eli's non-determinism comment. Use range-based for loops. Still need to add a test case.

rnk added a comment.Aug 6 2018, 11:38 AM

I think it was a mistake to put the Windows x64 .seh_ directives directly in MCStreamer. They should've been implemented similar to the way ARM CFI directives are implemented inside the ARMTargetStreamer class and the way CodeView FPO data is implemented entirely inside lib/Target/X86. Otherwise, we will continue to have a massive proliferation of every target's unwind info directives implemented in our main cross-platform assembler class, MCStreamer.

Does that make sense? You should be able to modify AArch64MCInstrLower.cpp to direct these AArch64-specific pseudo instructions over to the AArch64TargetStreamer class. The .cv_fpo_* directive implementations are probably your best guide.

ssijaric marked an inline comment as done.Aug 6 2018, 1:12 PM

Hi Reid,

Yes, I will move the SEH directives to the AArch64TargetStreamer, etc., and upload a new patch.

Thanks,
Sanjin

ssijaric updated this revision to Diff 160492.Aug 13 2018, 5:03 PM
ssijaric edited the summary of this revision. (Show Details)

Move SEH opcode handling to AArch64 target streamers.

Working on MIR test cases.

ssijaric updated this revision to Diff 162315.Aug 23 2018, 5:53 PM

Add a test case and fix a couple of encodings.

I also moved the AArch64 specific changes to the Windows exception handling tables to this patch, as they are needed to get the test case to pass (they are required in order to populate .xdata correctly). The test case also has a dependence on https://reviews.llvm.org/D51201, which adds HasWinCFI to the MIRParser.

There's also a dependence on making MachineFunction::HasWinCFI just a plain bool defaulting to false, instead of Optional<bool>. Otherwise, some existing Windows test cases will fail due to not having HasWinCFI set which is checked during .xdata generation. Reid, can you please submit this change as you mentioned it in D50288? Alternatively, I can go through the failing test cases (about 10 in total) and mark the functions with the "nounwind" attribute.

rnk added a comment.Sep 10 2018, 5:01 PM

I like the code, but I think this needs more tests, and better testing infrastructure before we can commit it. I would recommend beefing up COFFDumper::printUnwindInfo in llvm-readobj to print the ARM64 unwind opcodes in a human readable format, and then you won't need to write tests with hexadecimal CHECKs like you have now.

lib/MC/MCWin64EH.cpp
30 ↗(On Diff #162315)

llvm_unreachable() is preferred for this

69 ↗(On Diff #162315)

ditto

271 ↗(On Diff #162315)

llvm_unreachable

lib/Target/AArch64/MCTargetDesc/AArch64ELFStreamer.cpp
225 ↗(On Diff #162315)

This should probably live in AArch64TargetStreamer.cpp of AArch64MCTargetDesc.cpp, and call through a similar factory free function for ELF.

ssijaric updated this revision to Diff 165186.Sep 12 2018, 5:32 PM

Thanks for reviewing, Reid. This update uses llvm_unreachable instead of asserts, and moves createAArch64TargetObjectStreamer to AArch64TargetStreamer.h/.cpp. To do this, I also moved AArch64TargetELFStreamer to the llvm namespace.

I am working on adding support to llvm-objdump to dump the unwind info for Windows ARM64 in a meaningful way. Once that is complete, I will redo the test case, as well as add more test cases.

dmajor added a subscriber: dmajor.Oct 22 2018, 2:48 PM
ssijaric updated this revision to Diff 170546.Oct 22 2018, 6:01 PM

Add test cases. These depend on D53264 and D51201.

ssijaric updated this revision to Diff 170697.Oct 23 2018, 11:06 AM

Re-uploaed with full context and remove an unused declaration of getLabelPlusOne.

Missing test coverage for SEH_SaveFPLR_X, SEH_SaveReg, SEH_SaveFReg, SEH_SetFP, SEH_AddFP, SEH_Nop, and the alloc_s variant of SEH_StackAlloc. (I'd like to see at least one use of each opcode to show the encoding is correct/consistent with llvm-readobj.)

test/CodeGen/AArch64/wineh5.mir
164 ↗(On Diff #170697)

It looks like the SEH_StackAlloc is after the PrologEnd here; is that supposed to be valid? If not, can we report_fatal_error on MIR which does that?

Even ignoring that, the opcode sequence here is clearly wrong; you aren't allowed to call another function in the prolog. (From the unwinder's point of view, __chkstk is dynamic allocation, like alloca.) I guess you generated this using clang; that indicates a bug in the frame lowering patch, not this patch. But it would be nice to have valid testcases anyway.

ssijaric added inline comments.Oct 23 2018, 12:56 PM
test/CodeGen/AArch64/wineh5.mir
164 ↗(On Diff #170697)

Yes, this is wrong. Cannot come after PrologueEnd. I was doing some experiments with the frame lowering patch and the stack probe, which I clearly got wrong. I will check what cl.exe does.

ssijaric updated this revision to Diff 170787.Oct 23 2018, 4:48 PM

Fix the test case, wineh5.mir, to conform to what cl.exe produces.

rnk added a comment.Oct 24 2018, 1:34 PM

Missing test coverage for SEH_SaveFPLR_X, SEH_SaveReg, SEH_SaveFReg, SEH_SetFP, SEH_AddFP, SEH_Nop, and the alloc_s variant of SEH_StackAlloc. (I'd like to see at least one use of each opcode to show the encoding is correct/consistent with llvm-readobj.)

I'd also like to see a test for shrink-wrapping. You may have to disable it if you cannot yet describe such prologues it with SEH_ opcodes.

In D50166#1274871, @rnk wrote:

Missing test coverage for SEH_SaveFPLR_X, SEH_SaveReg, SEH_SaveFReg, SEH_SetFP, SEH_AddFP, SEH_Nop, and the alloc_s variant of SEH_StackAlloc. (I'd like to see at least one use of each opcode to show the encoding is correct/consistent with llvm-readobj.)

I'd also like to see a test for shrink-wrapping. You may have to disable it if you cannot yet describe such prologues it with SEH_ opcodes.

Hi Reid,

I think shrink wrapping is disabled by default for AArch64. I have a small test case that gets shrink-wrapped, but only if I enable it manually using -enable-shrink-wrap. Do we need a test case if it's disabled by default?

In D50166#1274871, @rnk wrote:

Missing test coverage for SEH_SaveFPLR_X, SEH_SaveReg, SEH_SaveFReg, SEH_SetFP, SEH_AddFP, SEH_Nop, and the alloc_s variant of SEH_StackAlloc. (I'd like to see at least one use of each opcode to show the encoding is correct/consistent with llvm-readobj.)

I'd also like to see a test for shrink-wrapping. You may have to disable it if you cannot yet describe such prologues it with SEH_ opcodes.

Hi Reid,

I think shrink wrapping is disabled by default for AArch64. I have a small test case that gets shrink-wrapped, but only if I enable it manually using -enable-shrink-wrap. Do we need a test case if it's disabled by default?

Wrong observation on my part. Shrink wrapping is enabled by default on AArch64, but only if it has no Windows CFI. There is even a comment in Shrink Wrapping that says:

Windows with CFI has some limitations that make it impossible
to use shrink-wrapping.

To handle shrink wrapping, we need to be able to break functions into regions, each with each own .pdata and .xdata. We don't currently support this. But it is definitely something we should handle in the future.

So, I will skip a test case for shrink wrapping. Will add a few more tests to cover remaining the unwind codes.

ssijaric updated this revision to Diff 171263.Oct 26 2018, 2:07 AM

Add test cases for the remaining SEH opcodes. Disable post RA scheduling for the test cases.

A follow on patch will take care of scheduling to make sure that either:

  1. SEH_PrologEnd and SEH_EpilogStart/SEH_EpilogEnd act as barriers, or
  2. Insert a SEH_Nop for any instruction that ends up between SEH_EpilogStart/SEH_EpilogEnd

Also, need a patch to fix the formatting in llvm-readobj (e.g. the indentation in wineh2.mir is off for floating point saves/restores).

A follow on patch will take care of scheduling

That seems fine. The fix is likely orthogonal to the other changes. (Maybe you need to override isSchedulingBoundary?)

Also, need a patch to fix the formatting in llvm-readobj (e.g. the indentation in wineh2.mir is off for floating point saves/restores).

FileCheck collapses whitespace; you can fix that in your MIR testcases independently of the readobj fix.

lib/MC/MCWin64EH.cpp
505 ↗(On Diff #171263)

The extended code word handling is still wrong.

rnk added a comment.Oct 26 2018, 1:32 PM

So, I will skip a test case for shrink wrapping. Will add a few more tests to cover remaining the unwind codes.

Can you copy a test where shrink wrapping would fire, and validate that it does not for aarch64-windows-gnu? Without the test, someone could remove the logic you found and no tests would fail. We have a test like this for x86_64.

In D50166#1277766, @rnk wrote:

So, I will skip a test case for shrink wrapping. Will add a few more tests to cover remaining the unwind codes.

Can you copy a test where shrink wrapping would fire, and validate that it does not for aarch64-windows-gnu? Without the test, someone could remove the logic you found and no tests would fail. We have a test like this for x86_64.

Sure, I will add a test case that gets shrink wrapped on aarch64-linux, but doesn't get shrink wrapped on aarch64-windows.

ssijaric updated this revision to Diff 171372.Oct 26 2018, 4:11 PM

Remove spaces in wineh2.mit test case. Fix the extended code words logic that Eli pointed out. Add a test case to make sure that shrink wrapping doesn't kick in.

This revision is now accepted and ready to land.Oct 26 2018, 5:05 PM
This revision was automatically updated to reflect the committed changes.