This is an archive of the discontinued LLVM Phabricator instance.

[ARM64] [Windows] Exception handling support in frame lowering
ClosedPublic

Authored by ssijaric on Aug 3 2018, 5:17 PM.

Details

Summary

This is a follow-up patch to D50166. It modifies frame lowering to emit pseudo instructions corresponding to unwind codes (eg. SEH_SaveReg), that the patch in D50166 (MCLayer part) will emit into the .xdata section. This includes pairing of only consecutive registers in prologue and epilogue, and saving/restoring fp,lr instead of lr,fp on Windows when unwinding data needs to be emitted.

Changes for funclets and EH tables where they differ from X86 will be upstreamed in separate patches.

Diff Detail

Repository
rL LLVM

Event Timeline

ssijaric created this revision.Aug 3 2018, 5:17 PM
ssijaric updated this revision to Diff 159158.Aug 3 2018, 7:43 PM
ssijaric edited the summary of this revision. (Show Details)

Remove a couple of unused functions from the initial patch. Fix formatting.

ssijaric edited the summary of this revision. (Show Details)Aug 3 2018, 8:40 PM
thegameg added inline comments.
lib/Target/AArch64/AArch64FrameLowering.cpp
185

From what I've seen in the way it's used, I'm not sure this class is actually useful. It seems to be always constructed at the end of a scope, where a BuildMI would have been much more readable.

409

Can you use MCRegisterInfo::getDwarfRegNum for this?

727

Can you move this into a separate function?

792

Typo (laset) and extra spaces here.

799

DL_CFI does not match the LLVM naming conventions.

848

This does not seem equivalent to what PrologueSaveSize was.

867

Extra spaces in the comments.

1202

This was already set in emitPrologue. Why is it needed here?

1205

Same here, it doesn't seem equivalent to PrologueSaveSize.

1530

Can we make this windows specific check a separate function and move the comment there?

rnk added a subscriber: hans.Aug 6 2018, 11:59 AM

The main points are:

  1. Let's split funclet EH stuff out into a separate patch and just focus on CFI in this
  2. I would prioritize implementing D50166 mostly in lib/Target/AArch64
lib/CodeGen/AsmPrinter/WinException.cpp
238–239 ↗(On Diff #159158)

This variable is used frequently enough that I'd make it a member of WinException.

252 ↗(On Diff #159158)

How is this different from the existing .seh_endproc directive emitted below?

636–637 ↗(On Diff #159158)

Please abstract this into a generic getLabel method. It can internally check a member IsAArch64 boolean and add one or not.

882 ↗(On Diff #159158)

None of the changes so far appear to be necessary for plain unwind info, no C++ exception handling. I'd prefer to split this patch into changes necessary for generating correct unwind info, and changes necessary to make C++ exception handling work.

lib/Target/AArch64/AArch64FrameLowering.cpp
587

Let's not have two boolean parameters with default values next to each other. It makes it easy to confuse their meaning at the call site.

1959

Hm, looks like I added this for X86. It seems more confusing than just defaulting MachineFunction::HasWinCFI to false. @hans, would you be OK if I made that Optional<bool> into a plain bool initialized to false? It seems intuitive enough that if emitPrologue isn't called (for naked functions), the function doesn't have CFI.

hans added inline comments.Aug 6 2018, 11:51 PM
lib/Target/AArch64/AArch64FrameLowering.cpp
1959

Sounds good to me.

ssijaric added inline comments.Aug 24 2018, 6:19 PM
lib/CodeGen/AsmPrinter/WinException.cpp
238–239 ↗(On Diff #159158)

WinException is now part of the MCLayer patch (D50166) and includes this change. I moved WinException to D50166 to get the test case to work as I check for the validity of unwind codes (and other information) in the .xdata section.

252 ↗(On Diff #159158)

This is now documented in the updated patch. The .seh_endproc below is not emitted until after the .xdata section is populated. We use the "FuncletOrFuncEnd" directive to calculate sizes of funclets and functions that .xdata requires.

636–637 ↗(On Diff #159158)

WinException is now moved to the MCLayer patch, D50166, and this is addressed there.

882 ↗(On Diff #159158)

I will update the MCLayer patch, which now includes WinException, to exclude this part.

lib/Target/AArch64/AArch64FrameLowering.cpp
185

I used it to avoid making sure that I inserted SEH_PrologEnd or SEH_EpilogEnd on all possible exits from emitEpilogue or emitPrologue. I will undo it.

409

Will do in the updated patch.

587

Will fix this in the updated patch.

727

Will do in the updated patch.

792

Will fix in the updated patch.

799

Will fix in the updated patch.

848

I ran into a bug where convertCalleeSaveRestoreToSPPrePostIncDec was being called when the CalleeSavedStackSize was zero, but FixedObject was of non-zero size (due to varargs on Windows). I cannot reproduce this any longer, so I will restore the original check. If I run into the problem again, I will post a separate patch.

867

Will fix in the updated patch.

1202

I will document this better. It was needed because it would get reset in the presence of funclets.

1205

Please see the reply to the previous comment.

1530

Yes, will fix in the updated patch.

ssijaric updated this revision to Diff 163267.Aug 29 2018, 11:33 PM
ssijaric edited the summary of this revision. (Show Details)

Addressed the comments and reduced the patch to handle only the frame lowering portion. Changes for funclets and EH tables will go in separate patches.

thegameg added inline comments.Sep 3 2018, 6:43 AM
lib/Target/AArch64/AArch64FrameLowering.cpp
159

Should this be in AArch64InstrInfo?

459

How about:

case AArch64::STPDpost:
  Imm = -Imm;
  LLVM_FALLTHROUGH
case AArch64::STPDpre:
  [...]

Same applied to the other cases below.

466

Is Reg0 - AArch64::D0 the same as MRI->getDwarfRegNum?

Same applied to the other cases below.

483

Why the extra { / }?

1455

Indentation seems off.

1692

I would rather just special-case the whole block since it seems to be having a pretty different behaviour than the non-wincfi path, but it's just a personal preference.

1779

Same here.

Thanks for reviewing, Francis. I'll upload an updated patch.

lib/Target/AArch64/AArch64FrameLowering.cpp
159

Will move it in the updated patch.

459

Will change it in the updated patch.

466

getDwarfRegNum doesn't work for floating point registers, as the Dwarf register numbers start at 64 for those:

e.g.

def B0 : AArch64Reg<0, "b0">, DwarfRegNum<[64]>;

I looked around for an existing API to retrieve the number corresponding to the register, but didn't see one.

483

Will remove in the updated patch.

1455

Will fix in the updated patch.

1692

I'll special case the whole block in the updated patch.

ssijaric updated this revision to Diff 164457.Sep 7 2018, 10:31 AM
ssijaric retitled this revision from [ARM64] [Windows] Exception handling, part #2 to [ARM64] [Windows] Exception handling support in frame lowering.
ssijaric added a reviewer: thegameg.

Addressed Francis' review feedback, and ran clang-format through some parts of the changes in AArch64FrameLowering.cpp. Changed the title to better reflect the intent of the patch.

thegameg added inline comments.Sep 13 2018, 9:05 AM
lib/Target/AArch64/AArch64FrameLowering.cpp
460

We often use use LLVM_FALLTHROUGH; in this case.

dmajor added a subscriber: dmajor.Oct 22 2018, 2:48 PM
ssijaric updated this revision to Diff 171584.Oct 29 2018, 3:06 PM

Add test cases. Use getSEHReg to retrieve SEH register numbers. Clean up the logic in a few places.

Test coverage seems reasonable to me.

lib/Target/AArch64/AArch64FrameLowering.cpp
567

llvm_unreachable.

884

MOVi64imm is a pseudo-instruction that expands to up to four instructions. (Here, probably not more than two, given that the unwind opcodes don't allow allocating more than 256MB of stack at a time anyway.) So I think you might be short one NOP in cases that involve allocating more than 1MB of stack.

Probably the simplest solution is to explicitly write out the relevant instructions in frame lowering, so you know how many nops you will need. (See AArch64ExpandPseudo::expandMOVImmSimple.)

1527

Not sure why you're deleting the comment here.

1584

FixupDone = NeedsWinCFI; is technically correct, I guess, but it seems simpler to write the equivalent FixupDone = true;.

It might be worth changing this logic further at some point, to rearrange spills to avoid ldp/stp operations that aren't 16-byte-aligned on Windows. (They're not wrong, but they might be slower.) But maybe it's rare enough that it doesn't matter.

ssijaric updated this revision to Diff 171642.Oct 29 2018, 11:44 PM

Address Eli's feedback.

This revision is now accepted and ready to land.Oct 30 2018, 11:25 AM
rnk accepted this revision.Oct 30 2018, 4:30 PM

Looks good, thanks!

lib/Target/AArch64/AArch64FrameLowering.cpp
456

Please use llvm_unreachable instead of assert(false).

This revision was automatically updated to reflect the committed changes.