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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a subscriber: hans.
rnk added a comment.

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 https://reviews.llvm.org/D50166 mostly in lib/Target/AArch64



================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:238-239
 
+  bool isAArch64 = Asm->MF->getTarget().getTargetTriple().getArch() ==
+                   Triple::ArchType::aarch64;
+
----------------
This variable is used frequently enough that I'd make it a member of WinException.


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


================
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");
----------------
Please abstract this into a generic `getLabel` method. It can internally check a member `IsAArch64` boolean and add one or not.


================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:882
 
+  bool isAArch64 = Asm->MF->getTarget().getTargetTriple().getArch() ==
+                   Triple::ArchType::aarch64;
----------------
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.


================
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
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1889
+  // emitPrologue if it gets called and emits CFI.
+  MF.setHasWinCFI(false);
+
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D50288