Page MenuHomePhabricator

[ARM SEH 3] [ARM] [MC] Add support for writing ARM WinEH unwind info
ClosedPublic

Authored by mstorsjo on May 15 2022, 2:33 PM.

Details

Summary

This includes .seh_* directives for generating it from assembly.
It is designed fairly similarly to the ARM64 handling.

For .seh_handler directives, such as
".seh_handler __C_specific_handler, @except" (which is supported
on x86_64 and aarch64 so far), the "@except" bit doesn't work in
ARM assembly, as '@' is used as a comment character (on all current
platforms).

Allow using '%' instead of '@' for this purpose. This convention
is used by GAS in similar contexts already,
e.g. [1]:

Note on targets where the @ character is the start of a comment
(eg ARM) then another character is used instead. For example the
ARM port uses the % character.

In practice, this unfortunately means that all such .seh_handler
directives will need ifdefs for ARM.

Contrary to ARM64, on ARM, it's quite common that we can't evaluate
e.g. the function length at this point, due to instructions whose
length is finalized later. (Also, inline jump tables end with
a ".p2align 1".)

If unable to to evaluate the function length immediately, emit
it as an MCExpr instead. If we'd implement splitting the unwind
info for a function (which isn't implemented for ARM64 yet either),
we wouldn't know whether we need to split it though.

TODO: We probably should emit this with an ARM specific fixup, so
that we at least could implement range checking before writing the
final value after evaluating the MCExpr.

We maybe should rename MCWin64EH, as this is used on a 32 bit
architecture too.

[1] https://sourceware.org/binutils/docs/as/Section.html#Section

Diff Detail

Event Timeline

mstorsjo created this revision.May 15 2022, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 2:33 PM
mstorsjo requested review of this revision.May 15 2022, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 2:33 PM

It looks like this doesn't include any directive to set the "F" bit in xdata, or the "Condition" field in epilogue scopes. Is the omission intentional? (I assume your LLVM patch won't emit sequences that would require either of those, but that might change, and people writing assembly might want them.)

llvm/include/llvm/Support/Win64EH.h
79

Not sure I like calling this operation UOP_SetFP, given it can set an arbitrary register.

It looks like this doesn't include any directive to set the "F" bit in xdata,

That's true. If we'd have unwind info splitting implemented, that bit would be set on the follow-up fragments, implicitly. Maybe there's a usecase for manually creating such unwind data though? I think that one can be added without much troubles later, if there's a concrete demand for it though...

(On that note, when rereading the docs about the "F" bit, I'm left wondering how that's supposed to work. If we have a huge function, and describe a prologue-less fragment in the middle of the function - how will unwinding from it work, when there's no prologue opcodes in that fragment to use for unwinding, if unwinding starts from outside of any potential epilogue in it, as there also can be fragments without epilogues?)

or the "Condition" field in epilogue scopes. Is the omission intentional? (I assume your LLVM patch won't emit sequences that would require either of those, but that might change, and people writing assembly might want them.)

Right, that's probably a bit bigger omission. Yeah it's not needed by our generated code, but it would probably be good to be able to express it if we wanted to. Maybe .seh_startepilogue_cond <cond> or something like that? (We'd probably want to have the default .seh_startepilogue as is - I think.)

llvm/include/llvm/Support/Win64EH.h
79

Oh, right, I guess I brought over the design a bit too much from the aarch64 case here, while it's notably different here.

Should we call it UOP_SaveSP and .seh_save_sp then?

It looks like this doesn't include any directive to set the "F" bit in xdata,

That's true. If we'd have unwind info splitting implemented, that bit would be set on the follow-up fragments, implicitly. Maybe there's a usecase for manually creating such unwind data though? I think that one can be added without much troubles later, if there's a concrete demand for it though...

We would want this to split a function across multiple section (e.g. for hot/cold splitting).

(On that note, when rereading the docs about the "F" bit, I'm left wondering how that's supposed to work. If we have a huge function, and describe a prologue-less fragment in the middle of the function - how will unwinding from it work, when there's no prologue opcodes in that fragment to use for unwinding, if unwinding starts from outside of any potential epilogue in it, as there also can be fragments without epilogues?)

I'm pretty sure the idea is that you still emit the prologue opcodes from the original prologue. The "F" bit just means that the prologue is not part of the current fragment. (For AArch64, this idea was generalized with the "c_end" opcode.)

or the "Condition" field in epilogue scopes. Is the omission intentional? (I assume your LLVM patch won't emit sequences that would require either of those, but that might change, and people writing assembly might want them.)

Right, that's probably a bit bigger omission. Yeah it's not needed by our generated code, but it would probably be good to be able to express it if we wanted to. Maybe .seh_startepilogue_cond <cond> or something like that? (We'd probably want to have the default .seh_startepilogue as is - I think.)

Sure.

llvm/include/llvm/Support/Win64EH.h
79

That sounds reasonable.

It looks like this doesn't include any directive to set the "F" bit in xdata,

That's true. If we'd have unwind info splitting implemented, that bit would be set on the follow-up fragments, implicitly. Maybe there's a usecase for manually creating such unwind data though? I think that one can be added without much troubles later, if there's a concrete demand for it though...

We would want this to split a function across multiple section (e.g. for hot/cold splitting).

Oh, right, true.

(On that note, when rereading the docs about the "F" bit, I'm left wondering how that's supposed to work. If we have a huge function, and describe a prologue-less fragment in the middle of the function - how will unwinding from it work, when there's no prologue opcodes in that fragment to use for unwinding, if unwinding starts from outside of any potential epilogue in it, as there also can be fragments without epilogues?)

I'm pretty sure the idea is that you still emit the prologue opcodes from the original prologue. The "F" bit just means that the prologue is not part of the current fragment. (For AArch64, this idea was generalized with the "c_end" opcode.)

Ah, yes. (I just rechecked this with my empirical tests with RtlVirtualUnwind too.)

Would this look like a reasonable way of handling it?

myfragment:
    .seh_proc myfragment
    .seh_save_regs {r4-r7,lr}
    .seh_stackalloc 1234
    .seh_endprologue_fragment
    // fragment code
    .seh_startepilogue
    // regular epilogue

Thus we'd pile up the phantom prologue opcodes at the start, and end it with an . seh_endprologue_fragment (which also implies that the opcodes aren't expected to exactly match the range between .seh_proc and .seh_endprologue_fragment).

.seh_endprologue_fragment looks fine.

mstorsjo updated this revision to Diff 430178.May 17 2022, 1:45 PM

Renamed the opcode for mov rX, sp to UOP_SaveSP and .seh_save_sp, added .seh_endprologue_fragment and .seh_startepilogue_cond.

mstorsjo added inline comments.May 17 2022, 1:48 PM
llvm/include/llvm/MC/MCWinEH.h
60

I guess it'd be much preferrable here, to change this into a map containing a struct instead, e.g.:

struct Epilog {
    std::vector<Instruction> Instructions;
    unsigned Condition;
};
MapVector<MCSymbol *, Epilog> EpilogMap;

And if doing that, I guess it'd be best to do such a refactoring (without the Condition field) as a separate preceding commit. But posting it in this form for now, for potential comments on the rest of the changes.

llvm/lib/MC/MCWin64EH.cpp
1441

How do I make this into an ARM specific fixup, which could do range checking for FuncLengthExpr and error out if the function is too long?

efriedma added inline comments.May 17 2022, 4:52 PM
llvm/lib/MC/MCWin64EH.cpp
1441

See llvm/lib/Target/ARM/MCTargetDesc/ARMMCExpr.h.

I'd prefer not to put any substantial effort into infrastructure we won't need if we implement splitting, though. Zhaoshi is currently looking into that.

mstorsjo added inline comments.May 18 2022, 12:38 AM
llvm/lib/MC/MCAsmStreamer.cpp
2079

What do you think about this use of % instead of @ here, does it seem reasonable?

The alternative would be to switch to a different comment character, but I'm afraid that would break lots of handwritten assembly out there - while there's very little code that uses these explicit .seh_handler directives, so I guess those will just have to cope with some ifdefs.

llvm/lib/MC/MCWin64EH.cpp
1441

Oh, ok. Sure, I'll stop worrying about that aspect then! (As I've never hit that limit in practice on ARM64 so far, I'm not very worried about it for ARM either. But for implementing splitting, the fact that ARM functions often won't have their length fixed at this point might be problematic...)

mstorsjo updated this revision to Diff 430338.May 18 2022, 5:10 AM

Cleaner implementation of conditional epilogues, using the preparatory refactoring from D125879.

efriedma added inline comments.May 18 2022, 2:54 PM
llvm/lib/MC/MCAsmStreamer.cpp
2079

Switching the character seems fine.

llvm/lib/MC/MCWin64EH.cpp
1441

If we need to, we can put off emitting the unwind info until the layout is fixed. See the giant FIXME near the beginning of ARM64EmitUnwindInfo.

mstorsjo added inline comments.May 19 2022, 12:31 AM
llvm/lib/MC/MCWin64EH.cpp
1441

Ah, I see! (I've read it a couple times before but never really confidently understood all the alternatives - but now I think I get the part about a relaxable fragment too).

Anyway, the current code should be fine for practical cases here, and whenever we get support for delaying emitting the unwind info for ARM64, we can apply the same to ARM too.

Any further comments on patches 3-5? AFAIK there are no outstanding issues that have been brought up on them. (I'm still looking into fixing the frame pointer placement for patches 6-7.)

Did you consider instead of adding .seh_endepilogue_nop, just require writing .seh_nop; .seh_endepilogue? Then we can automatically merge a tailing nop with the "end" opcode.

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
12106

Do we really need the pc -> lr remapping, as opposed to just forbidding users from specifying pc? Seems a little weird to have multiple ways to write the same unwind code.

Did you consider instead of adding .seh_endepilogue_nop, just require writing .seh_nop; .seh_endepilogue? Then we can automatically merge a tailing nop with the "end" opcode.

Hmm, I didn't consider that - as they're separate opcodes in the unwind info, I was kinda led into this format. But that sounds like a good idea - I'll try it out and see if it seems better or not!

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
12106

I guess it's not strictly necessary, but both for consistency with the preceding instruction, you may want to use both; pop {pc} and pop {lr}; bx lr both would use the same .seh_save_regs {lr} opcode, while I think you'd might want to write .seh_save_regs {pc} in the former case.

Did you consider instead of adding .seh_endepilogue_nop, just require writing .seh_nop; .seh_endepilogue? Then we can automatically merge a tailing nop with the "end" opcode.

Hmm, I didn't consider that - as they're separate opcodes in the unwind info, I was kinda led into this format. But that sounds like a good idea - I'll try it out and see if it seems better or not!

Yep, overall that seems to make things even cleaner and looks like a good direction - thanks!

However, doing that exposes a different underlying issue in the handling of the SEH_* machine instructions used in frame lowering that hadn't been a problem so far - I'll follow up in D125648 with that issue.

mstorsjo updated this revision to Diff 431268.May 22 2022, 2:38 PM

Remove the .seh_endepilogue_nop variants and use explicit .seh_nop instructions instead.

Any more comments on this set? I’m generally happy with patches 3-6, and patch 7 seems to work even if it’s not very pretty.

efriedma added inline comments.May 25 2022, 12:35 PM
llvm/lib/CodeGen/AsmPrinter/WinException.cpp
764

I assume this is related somehow, but not sure how, exactly.

llvm/lib/Target/ARM/MCTargetDesc/ARMWinCOFFStreamer.cpp
114

This comment is out-of-date?

mstorsjo added inline comments.May 25 2022, 12:45 PM
llvm/lib/CodeGen/AsmPrinter/WinException.cpp
764

Ah, hmm. I think this might be leftovers from initial attempts at making things work for MSVC mode (for either C++ unwinding or SEH __try). I left that out of scope for now (itanium unwinding for mingw mode works fine without it though), so I guess I should leave it out here too.

llvm/lib/Target/ARM/MCTargetDesc/ARMWinCOFFStreamer.cpp
114

Oh, oops. I guess I'll leave out the specifics of the opcodes here as I don't think it's relevant to elaborate on them here.

mstorsjo added inline comments.May 25 2022, 12:52 PM
llvm/lib/CodeGen/AsmPrinter/WinException.cpp
764

Ah, no, now I remember. We have an existing testcase, test/CodeGen/ARM/Windows/wineh-basic.ll, which tests some C++ exception handling constructs.

Since we enable WinEHEncodingType = WinEH::EncodingType::Itanium; in this commit, we end up trying to do more things than we've done before, and that testcase triggers a failed assert, as we try to call UnwindHelpOffset = getFrameIndexOffset(FuncInfo.UnwindHelpFrameIdx, FuncInfo) when FuncInfo.UnwindHelpFrameIdx is unset (left at its default value of std::numeric_limits<int>::max()).

So these changes could be removed later when MSVC C++ exception handling is properly implemented for ARM, but for now, this avoids breaking the existing testcase.

mstorsjo updated this revision to Diff 432096.May 25 2022, 12:54 PM

Removed leftover copypaste bits from AArch64 in a comment.

mstorsjo updated this revision to Diff 432232.May 26 2022, 3:34 AM

Rebased after superclass methods have been renamed.

Ping - any more on this? If you want to, I could add comments at the changes in AsmPrinter/WinException.cpp that they can be removed if the MS C++ unwinding is fully implemented for ARM (and/or remove the wineh-basic.ll testcase?).

efriedma accepted this revision.Tue, May 31, 5:30 PM

I think I'd like some sort of comment on the WinException.cpp, yes.

Otherwise LGTM

This revision is now accepted and ready to land.Tue, May 31, 5:30 PM
This revision was landed with ongoing or failed builds.Wed, Jun 1, 1:26 AM
This revision was automatically updated to reflect the committed changes.