[PATCH] D53540: [COFF, ARM64] Implement support for SEH extensions __try/__except/__finally

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.

If you're okay with trying to resolve any remaining bugs as a followup, that's fine with me; LGTM.



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:2744
+    else // This function handles the SP or FP case.
+      Reg = RegInfo->getFrameRegister(MF);
+    return DAG.getCopyFromReg(DAG.getEntryNode(), dl, Reg,
----------------
rnk wrote:
> smeenai wrote:
> > rnk wrote:
> > > efriedma wrote:
> > > > Are you sure this does the right thing in general?  The link between the choice of register here, and the register used for the getFrameIndexReference call in WinException::emitEHRegistrationOffsetLabel , seems fragile at best.
> > > It is fragile, but it's what was done for x86. Unfortunately, it's actually really hard to know which physical register is going to be used to address locals during ISel, so I just did this brittle thing and moved on. See these comments from AArch64FrameLowering::hasFP for some of the phase ordering problems:
> > >   // With large callframes around we may need to use FP to access the scavenging
> > >   // emergency spillslot.
> > >   //
> > >   // Unfortunately some calls to hasFP() like machine verifier ->
> > >   // getReservedReg() -> hasFP in the middle of global isel are too early
> > >   // to know the max call frame size. Hopefully conservatively returning "true"
> > >   // in those cases is fine.
> > >   // DefaultSafeSPDisplacement is fine as we only emergency spill GP regs.
> > >   if (!MFI.isMaxCallFrameSizeComputed() ||
> > >       MFI.getMaxCallFrameSize() > DefaultSafeSPDisplacement)
> > >     return true;
> > > 
> > > I think we are mostly safe because when using EH funclets, an FP is always needed, so it's just a decision between "base" and "frame" pointer registers. That is currently entirely determined by the existence of user allocas with alignments above the standard ABI stack alignment.
> > > 
> > > However, you can imagine that some later pass might try to create more highly aligned stack objects, which would require a base pointer. For example, the register allocator might spill vector registers that require more stack alignment than the ABI provides. Today, for x86 at least, LLVM punts on this problem, and simply uses unaligned instructions for the spill and reload. I don't know how to get into the analogous situation for AArch64 or what LLVM would do, though.
> > Is this at all related to https://bugs.llvm.org/show_bug.cgi?id=37169?
> I had forgotten about that issue, but that's exactly the case I was describing. I proposed this solution there:
> 
> > I think localaddress should really lower down to a LEA of a zero-sized stack object. It's really trying to say, give me the address of the local variable area. The localrecover label assignments would instead compute the difference between this particular zero-sized stack object and their stack object.
> 
> For AArch64, LEA would really be ADC, or whatever is generated to materialize the address of an alloca. Does something like that make sense?
> 
> It occurs to me that this localescape system is pretty flawed, since not all local variables are static allocas. This source crashes clang today:
> 
> ```
> struct Foo {
>   int x, y, z;
> };
> int filter(int);
> void may_throw();
> void seh_byval(Foo o) {
>   __try {
>     may_throw();
>   } __except(filter(o.x)) {
>   }
> }
> ```
> 
> Oops. =/
> That is currently entirely determined by the existence of user allocas with alignments above the standard ABI stack alignment.

This is not true on AArch64.  There's a second condition: whether the size of the local frame is greater than 256 bytes.  This condition isn't strictly required, but avoids materializing offsets in some situations.  (AArch64 addressing modes support very limited immediate negative offsets.)

That doesn't really have any impact on its own, but AArch64FrameLowering::getFrameIndexReference actually chooses between FP-relative and BP-relative addressing based on the offsets, in general.  So we probably need to do something here, to force the frame index to be resolved the same way as localaddress.

I don't think we can hit an issue similar to bug 37169 on AArch64; the natural stack alignment is 16, and there currently isn't anything that requires higher alignment.

> It occurs to me that this localescape system is pretty flawed, since not all local variables are static allocas. 

Yes, we need special handling for byval/inalloca arguments at the IR level; in general, it's impossible to compute an offset to an argument relative to the base pointer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53540/new/

https://reviews.llvm.org/D53540