[COFF, ARM64] Implement support for SEH extensions __try/__except/__finally
Needs ReviewPublic

Authored by mgrang on Mon, Oct 22, 5:33 PM.

Details

Summary

This patch supports MS SEH extensions try/except/__finally. The intrinsics localescape and localrecover are responsible for communicating escaped static allocas from the try block to the handler.

For ARM64 Windows, the fp of the parent function is always passed in x1 to the except/finally funclets. So we do not need to generate llvm.x86.seh.recoverfp intrinsic to recover the parent fp.

We also deviate from X86 SEH behavior in that we do not create an MCSymbol to communicate the fp offsets to the funclets but rather directly pass the value of the symbol.

Finally, we need to preserve frame pointers for SEH. So we create a new function/property HasSEHFunclets similar to HasEHFunclets to handle this.

Diff Detail

mgrang created this revision.Mon, Oct 22, 5:33 PM
rnk added inline comments.Mon, Oct 22, 6:48 PM
lib/Target/AArch64/AArch64ISelLowering.cpp
2724 ↗(On Diff #170537)

It's not EBP for AArch64, surely. I would generalize the text to "FP" instead.

2731–2732 ↗(On Diff #170537)

Yeah, this was a great hack. :)

dmajor added a subscriber: dmajor.Mon, Oct 22, 7:04 PM

I think you got the titles swapped on this and D53541; this one adds the aarch64_seh_recoverfp intrinsic while D53541 uses it to support __try/__except.

mgrang updated this revision to Diff 170698.Tue, Oct 23, 10:50 AM
mgrang edited the summary of this revision. (Show Details)EditedTue, Oct 23, 10:52 AM

I think you got the titles swapped on this and D53541; this one adds the aarch64_seh_recoverfp intrinsic while D53541 uses it to support __try/__except.

D53541 generates aarch64_seh_recoverfp while this patch lowers it. This patch also lowers the localescape/localrecover intrinsics. I have updated the title summary accordingly.

mgrang added inline comments.Tue, Oct 23, 11:02 AM
lib/Target/AArch64/AArch64AsmPrinter.cpp
571 ↗(On Diff #170698)

I see that X86 generates LEA to calculate the address of the .L label. AArch64 does not have a similar instruction. I tried to use ADRP + ADD but that requires either a GlobalSymbol or an ExternalSymbol type. However, the .L label is of type MCSymbol.

So I generate a MOVK x1, #.Lfoo. The problem is that MOVK takes an unsigned immediate. The .L label here represents offset from FP. This works if the offset is positive. But for negative offsets we should generate a MOVN. However, I am not sure how to detect negative immediate without first generating a MOV.

Any suggestions?

lib/Target/AArch64/AArch64ISelLowering.cpp
2731–2732 ↗(On Diff #170537)

I am not really familiar with this piece of code. Could you please comment on whether this should work for AArch64?

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
219 ↗(On Diff #170698)

As I commented above, we assume a non-negative immediate.

rnk added inline comments.Tue, Oct 23, 3:51 PM
lib/Target/AArch64/AArch64AsmPrinter.cpp
571 ↗(On Diff #170698)

I would expect you would want a generic MOVMCSym instruction to be able to materialize any label, not just ones that are known during assembly. This might get selected if we needed to materialize, for example, the address of the LSDA, which is also a private label.

lib/Target/AArch64/AArch64ISelLowering.cpp
2729 ↗(On Diff #170698)

I think this is suppoosed to be ".seh_setframe offset"

2731–2732 ↗(On Diff #170537)

I think it should. Fundamentally, we are trying to transfer information about one function's frame layout into an independent filter function. The idea is that we can write assembly like this:

foo:
.Lfoo = 0x48
  ...

foo$filt:
  mov r0, .Lfoo

Hm, if possible, it might be nice to codegen this whole thing after SD ISel. I wonder if there is some way to pass it through as an intrinsic and lower it later. That will avoid the whole concern of having a generic MCSymbol materialization instruction selection pattern.

We need to have a test case that checks for access to local variables from the finally and except funclets. The parent's frame pointer is the second argument to both the filter functions and termination handlers, fro what I see. We need to make sure that locals variables in funclets are being accessed off x1, and not the stack pointer.

mgrang updated this revision to Diff 171996.Wed, Oct 31, 12:51 PM
mgrang retitled this revision from [COFF, ARM64] Implement support for SEH extensions __try/__except to [COFF, ARM64] Implement support for SEH extensions __try/__except/__finally.
mgrang edited the summary of this revision. (Show Details)
mgrang set the repository for this revision to rL LLVM.

Will push more comprehensive tests for ARM64 SEH soon.

majnemer added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6155

Since you already did an isa, this can be cast<MCConstantExpr>(Expr).

dmajor added inline comments.Thu, Nov 8, 7:57 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6155

I tried applying this patch (plus D53541, D51524, and D54248) and building Firefox, and I got a crash on null Expr here.

Let me know if you're interested in the repro, or if it's too early for me to be playing with this code, in which case sorry for the noise.

mgrang added inline comments.Thu, Nov 8, 11:05 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6155

@dmajor Thanks for testing this out :) If you have a reduced test case to repro the failure that would be great. Although currently I am busy with an internal release I expect to get back to this in Dec.

mgrang added inline comments.Thu, Nov 8, 12:14 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6155

@dmajor Also what patches (if any) to the Firefox code base are needed to build Firefox for arm64-windows. Are those patches publicly available?

dmajor added inline comments.Thu, Nov 8, 12:29 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6155

I don't really recommend it right now, it's a bit of a hassle because you have to apply some hacks to the build system. We're working on making it easier (hopefully within a week or two). In the meantime I can try to get you a repro with creduce.

TomTan added inline comments.Thu, Nov 8, 5:46 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6155

@dmajor, I hit the same null reference issue after cherry-pick changes you listed. Seems FrameAllocSym is not a set to SymContentsVariable here which is expected to happen in AArch64RegisterInfo::eliminateFrameIndex.

dmajor added inline comments.Fri, Nov 9, 7:08 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6155

Here's the repro of the crash from creduce:

class a {
  void b();
  void c();
};
void a::b() {
  int f;
  __try {
    c();
  } __except (f) {
  }
}
rnk added a comment.Tue, Nov 13, 10:46 AM

Sorry, I didn't realize this was waiting on my review. It looks like this is mostly about localescape, so let's change the patch description and code to be about that.

include/llvm/CodeGen/MachineFunction.h
332

IMO this should really live on the AArch64-specific function info, since it's not general to x64/x86 yet.

Also, this isn't SEH specific, this is set to true only by localescape. Maybe you should rename it? localescape was intended to be orthogonal to SEH, potentially usable for non-externally visible lambdas, although the codegen right now is particularly bad.