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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.) |
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.
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.
Hi Reid,
Yes, I will move the SEH directives to the AArch64TargetStreamer, etc., and upload a new patch.
Thanks,
Sanjin
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.
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. |
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.
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. |
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. |
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.
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:
- SEH_PrologEnd and SEH_EpilogStart/SEH_EpilogEnd act as barriers, or
- 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. |
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.
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.