This is an archive of the discontinued LLVM Phabricator instance.

[5/5] [AArch64] Generate and parse SEH assembly directives
ClosedPublic

Authored by mstorsjo on Aug 25 2020, 4:43 AM.

Details

Summary

This ensures that you get the same output regardless if generating code directly to an object file or if generating assembly and assembling that.

Add implementations of the EmitARM64WinCFI*() methods in AArch64TargetAsmStreamer, and fill in one blank in MCAsmStreamer.

Add corresponding directive handlers in AArch64AsmParser and COFFAsmParser.

The SEH directives have been picked to match the prior art for SEH assembly directives for x86_64, e.g. the spelling of ".seh_startepilogue" matching the preexisting ".seh_endprologue". For the directives for saving registers, the exact spelling from the arm64 documentation is picked, e.g. ".seh_save_reg" (to follow that naming for all the other ones, e.g. ".seh_save_fregp_x"), while the corresponding one for x86_64 is plain ".seh_savereg" without the second underscore.

Directives in the epilogues have the same names as in prologues, e.g. .seh_savereg, even though the registers are restored, not saved, at that point.

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 25 2020, 4:43 AM
efriedma added inline comments.Aug 25 2020, 2:15 PM
llvm/test/CodeGen/AArch64/lrint-conv-fp16-win.ll
7

I think this indicates a bug somewhere else: we shouldn't be emitting seh_startepilogue in functions that don't have any other unwind info.

mstorsjo added inline comments.Aug 26 2020, 2:03 AM
llvm/test/CodeGen/AArch64/lrint-conv-fp16-win.ll
7

No, we do generate full proper directives for start/endproc, prologue and all that. It's just that the current testcase doesn't break if there's changes in the prologue of the function, but the sequence of CHECK-NEXT from fcvtzs to ret requires including these directives here in order not to break the test.

So for these testcases, that focus on testing some other aspect, I did the minimal changes to include the new directives only where they're needed in CHECK-NEXT sequences, but didn't go full out and added checks for all the generated directives. But I'll include one such case as well.

Alternatively, one could add nounwind attributes to the tested functions, to avoid the noise from the added directives. (Is it possible to achieve the same with some llc flag? I tried looking for one but didn't find any.)

mstorsjo updated this revision to Diff 287875.Aug 26 2020, 2:05 AM
mstorsjo edited the summary of this revision. (Show Details)

Added a testcase that checks the full sequence of generated .seh directives (in the preexisting wineh1.mir), and added a test within seh.s, that makes llvm-mc output assembly, which is fed back to llvm-mc, to make sure that it can parse what it outputs and that it produces the same output as if assembling directly to object files.

efriedma added inline comments.Aug 26 2020, 2:06 PM
llvm/test/CodeGen/AArch64/lrint-conv-fp16-win.ll
7

Should we be generating any .seh_* directives in this case? If I'm not mistaken, we don't generate any actual unwind data in the object file.

mstorsjo added inline comments.Aug 26 2020, 2:10 PM
llvm/test/CodeGen/AArch64/lrint-conv-fp16-win.ll
7

The llc command in this testcase does generate unwind data in the generated object file, if it's written to an object file instead of output as asm.

efriedma accepted this revision.Aug 26 2020, 2:29 PM

LGTM

llvm/test/CodeGen/AArch64/lrint-conv-fp16-win.ll
7

Hmm, I see, we emit a trivial xdata with just an "end" opcode. We probably shouldn't, but I guess you don't need to address that in this patch.

This revision is now accepted and ready to land.Aug 26 2020, 2:29 PM
mstorsjo added inline comments.Aug 27 2020, 12:24 AM
llvm/test/CodeGen/AArch64/lrint-conv-fp16-win.ll
7

I think that's correct - it's a leaf function with no actual, prologue/epilogue so unwinding would be just returning to the address in lr. So emitting that sounds right to me, unless we should omit unwind info for leaf functions. (Unwind info for such functions still is needed for unwinding in the event of crashes and such though.)

Curiously though, it never emits .seh_endprologue, only an empty pair of .seh_startepilogue and .seh_endepilogue. So when writing the unwind info, the supposed prologue actually is completely empty (normally it should at least have a terminating UOP_End), so when interpreting the prologue, it implicitly falls through to the epilogue (which consists of one sole UOP_End) which starts at index 0.

mstorsjo updated this revision to Diff 288272.Aug 27 2020, 4:26 AM

Updated the testcase after updated patch 3/5.

Btw, @efriedma - Did you have any opinions on the naming of all the new .seh_* directive that this patch defines? The names of the new directives are pretty much set in stone once we have it in (or at least once third party projects start using them - and I have one usecase lined up).

efriedma added inline comments.Aug 27 2020, 2:04 PM
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
5197

Looking at these names again, maybe we could add underscores in a few places. The directive names should match the opcode names from https://docs.microsoft.com/en-us/cpp/build/arm64-exception-handling .

For the names that aren't directly derived from the opcodes in the spec, it probably makes sense to match x86. Which I think the current names do; that seems fine.

llvm/test/CodeGen/AArch64/lrint-conv-fp16-win.ll
7

Curiously though, it never emits .seh_endprologue, only an empty pair of .seh_startepilogue and .seh_endepilogue

That's sounds like a bug, then

unless we should omit unwind info for leaf functions

The Microsoft docs say "leaf functions that don't require any local storage, and don't need to save/restore non-volatile registers, do not require a .pdata record"

mstorsjo added inline comments.Aug 28 2020, 12:02 AM
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
5197

Ok, will change it that way. That does indeed make it more consistent. The only inconsistency is for the simplest form .seh_save_reg vs x86's .seh_savereg, but that's not much of an issue (the actual register names can't be reused across them anyway), and makes it more consistent and nicer across all of them, from simple .seh_save_reg up to .seh_save_fregp_x.

mstorsjo updated this revision to Diff 288537.Aug 28 2020, 12:04 AM
mstorsjo edited the summary of this revision. (Show Details)

Added underscores in .seh_save_[f]reg[p][_x] and in .seh_set_fp and .seh_add_fp.

This revision was automatically updated to reflect the committed changes.