Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
184 ↗(On Diff #159158)

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.

446 ↗(On Diff #159158)

Can you use MCRegisterInfo::getDwarfRegNum for this?

732 ↗(On Diff #159158)

Can you move this into a separate function?

818 ↗(On Diff #159158)

Typo (laset) and extra spaces here.

825 ↗(On Diff #159158)

DL_CFI does not match the LLVM naming conventions.

879 ↗(On Diff #159158)

This does not seem equivalent to what PrologueSaveSize was.

899 ↗(On Diff #159158)

Extra spaces in the comments.

1196 ↗(On Diff #159158)

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

1200 ↗(On Diff #159158)

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

1502 ↗(On Diff #159158)

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
614 ↗(On Diff #159158)

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.

1889 ↗(On Diff #159158)

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
1889 ↗(On Diff #159158)

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
184 ↗(On Diff #159158)

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.

446 ↗(On Diff #159158)

Will do in the updated patch.

614 ↗(On Diff #159158)

Will fix this in the updated patch.

732 ↗(On Diff #159158)

Will do in the updated patch.

818 ↗(On Diff #159158)

Will fix in the updated patch.

825 ↗(On Diff #159158)

Will fix in the updated patch.

879 ↗(On Diff #159158)

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.

899 ↗(On Diff #159158)

Will fix in the updated patch.

1196 ↗(On Diff #159158)

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

1200 ↗(On Diff #159158)

Please see the reply to the previous comment.

1502 ↗(On Diff #159158)

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 ↗(On Diff #163267)

Should this be in AArch64InstrInfo?

484 ↗(On Diff #163267)

How about:

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

Same applied to the other cases below.

491 ↗(On Diff #163267)

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

Same applied to the other cases below.

508 ↗(On Diff #163267)

Why the extra { / }?

1440 ↗(On Diff #163267)

Indentation seems off.

1629 ↗(On Diff #163267)

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.

1706 ↗(On Diff #163267)

Same here.

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

lib/Target/AArch64/AArch64FrameLowering.cpp
159 ↗(On Diff #163267)

Will move it in the updated patch.

484 ↗(On Diff #163267)

Will change it in the updated patch.

491 ↗(On Diff #163267)

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.

508 ↗(On Diff #163267)

Will remove in the updated patch.

1440 ↗(On Diff #163267)

Will fix in the updated patch.

1629 ↗(On Diff #163267)

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 ↗(On Diff #164457)

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 ↗(On Diff #171584)

llvm_unreachable.

884 ↗(On Diff #171584)

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 ↗(On Diff #171584)

Not sure why you're deleting the comment here.

1584 ↗(On Diff #171584)

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 ↗(On Diff #171642)

Please use llvm_unreachable instead of assert(false).

This revision was automatically updated to reflect the committed changes.