[PATCH] D50288: [ARM64] [Windows] Exception handling, part #2

Fri Aug 24 18:19:22 PDT 2018

ssijaric added inline comments.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:238-239

+  bool isAArch64 = Asm->MF->getTarget().getTargetTriple().getArch() ==
+                   Triple::ArchType::aarch64;
+
----------------
rnk wrote:
> This variable is used frequently enough that I'd make it a member of WinException.
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.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:252
+      Asm->OutStreamer->SwitchSection(CurrentFuncletTextSection);
+      Asm->OutStreamer->EmitWinCFIFuncletOrFuncEnd();
+      MCSection *XData = Asm->OutStreamer->getAssociatedXDataSection(
----------------
rnk wrote:
> How is this different from the existing `.seh_endproc` directive emitted below?
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.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:636-637
     AddComment("LabelStart");
-    OS.EmitValue(getLabelPlusOne(BeginLabel), 4);
+    OS.EmitValue(isAArch64 ? getLabel(BeginLabel)
+                           : getLabelPlusOne(BeginLabel), 4);
     AddComment("LabelEnd");
----------------
rnk wrote:
> Please abstract this into a generic `getLabel` method. It can internally check a member `IsAArch64` boolean and add one or not.
WinException is now moved to the MCLayer patch, D50166, and this is addressed there.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:882

+  bool isAArch64 = Asm->MF->getTarget().getTargetTriple().getArch() ==
+                   Triple::ArchType::aarch64;
----------------
rnk wrote:
> 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.
I will update the MCLayer patch, which now includes WinException, to exclude this part. 

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:184
+// A small RAII class to handle insertion of SEH codes on multiple paths.
+class InsertCFI {
+  MachineBasicBlock &MBB;
----------------
thegameg wrote:
> 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.
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.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:446

+static unsigned getRegNumX(unsigned Reg) {
+  if (Reg == AArch64::FP)
----------------
thegameg wrote:
> Can you use `MCRegisterInfo::getDwarfRegNum` for this?
Will do in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:614
+    const DebugLoc &DL, const TargetInstrInfo *TII, int CSStackSizeInc,
+    bool NeedsWinCFI = false, bool InProlog = true) {
   // Ignore instructions that do not operate on SP, i.e. shadow call stack
----------------
rnk wrote:
> 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.
Will fix this in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:732
+  if (NeedsWinCFI) {
+    auto SEH_MI = std::next(MachineBasicBlock::iterator(MI));
+    auto &MBB = *(MI.getParent());
----------------
thegameg wrote:
> Can you move this into a separate function?
Will do in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:818

+  // The very laset FrameSetup instruction  indicates the end of prologue.  Emit
+  // a SEH opcode indicating the prologue end.
----------------
thegameg wrote:
> Typo (`laset`) and extra spaces here.
Will fix in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:825
+      ++LastFrameSetupI;
+    DebugLoc DL_CFI =
+      (MBB.end() == LastFrameSetupI) ? DL : LastFrameSetupI->getDebugLoc();
----------------
thegameg wrote:
> `DL_CFI` does not match the LLVM naming conventions.
Will fix in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:879
     NumBytes = 0;
-  } else if (PrologueSaveSize != 0) {
+  } else if (AFI->getCalleeSavedStackSize() != 0) {
     MBBI = convertCalleeSaveRestoreToSPPrePostIncDec(MBB, MBBI, DL, TII,
----------------
thegameg wrote:
> This does not seem equivalent to what `PrologueSaveSize` was.
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.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:899
+  // The code below is not applicable to funclets.  We have emitted all the
+  // SEH opcodes that we needed to emit.  The FP and BP belong to the
+  // containing function.
----------------
thegameg wrote:
> Extra spaces in the comments.
Will fix in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1196
   auto PrologueSaveSize = AFI->getCalleeSavedStackSize() + FixedObject;
+  AFI->setLocalStackSize(NumBytes - PrologueSaveSize);
   bool CombineSPBump = shouldCombineCSRLocalStackBump(MF, NumBytes);
----------------
thegameg wrote:
> This was already set in `emitPrologue`. Why is it needed here?
I will document this better.  It was needed because it would get reset in the presence of funclets.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1200

-  if (!CombineSPBump && PrologueSaveSize != 0) {
+  if (!CombineSPBump && AFI->getCalleeSavedStackSize() != 0) {
     MachineBasicBlock::iterator Pop = std::prev(MBB.getFirstTerminator());
----------------
thegameg wrote:
> Same here, it doesn't seem equivalent to `PrologueSaveSize`.
Please see the reply to the previous comment. 

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1502
+          (!RPI.IsGPR && AArch64::FPR64RegClass.contains(NextReg))) &&
+          (!NeedsWinCFI || (NextReg == RPI.Reg1 + 1)))
         RPI.Reg2 = NextReg;
----------------
thegameg wrote:
> Can we make this windows specific check a separate function and move the comment there?
Yes, will fix in the updated patch.

Repository:
  rL LLVM

https://reviews.llvm.org/D50288