[PATCH] X86: In debug build, emit CFI data in X86FrameLowering and X86CallFrameOptimization

[Reid Kleckner]

Thanks for looking into this, I agree this is important functionality. I also agree this code is fairly duplicated and needs cleanup.

I have a feeling that emitting asynch unwind tables by default is going to break some tools that consume our unwind tables. I believe ld64 processes our .eh_frame data to produce it's own compact CFI format, and it may choke on these kinds of cfa updates. We're going to need a way to turn off these fine-grained updates if something breaks. The simplest way to flag this would be to add a bool to TargetOptions and default it to true. If it causes someone problems, we can flip the default (or tweak it based on platform) instead of reverting the patch. Ultimately, we can wire this up to -fasynchronous-unwind-tables in clang, and (hopefully) keep that on by default.

This also needs much more comprehensive testing, new tests in addition to the ones already present.


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:226-231
@@ -225,1 +225,8 @@
 
+  MachineFunction &MF = *MBB.getParent();
+  const Function *Fn = MF.getFunction();
+  MachineModuleInfo &MMI = MF.getMMI();
+  bool IsWinEH = MF.getTarget().getMCAsmInfo()->usesWindowsCFI();
+  bool NeedsDwarfCFI =
+      !IsWinEH && (MMI.hasDebugInfo() || Fn->needsUnwindTableEntry());
+
----------------
I won't ask you to refactor this goo, I'll see if I can do it myself at some point.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:258-261
@@ +257,6 @@
+        if (NeedsDwarfCFI) {
+          unsigned CFIIndex = MMI.addFrameInst(
+              MCCFIInstruction::createAdjustCfaOffset(nullptr, -Offset));
+          BuildMI(MBB, MBBI, DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
+              .addCFIIndex(CFIIndex);
+        }
----------------
Can you maybe wrap this all up in a static helper like BuildAdjustCFAMI? Anything would be good.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:285-286
@@ +284,4 @@
+        if (NeedsDwarfCFI) {
+          unsigned CFIIndex = MMI.addFrameInst(
+              MCCFIInstruction::createAdjustCfaOffset(nullptr, -ThisVal));
+          BuildMI(MBB, MBBI, DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
----------------
This doesn't appear to handle the sub case?

http://reviews.llvm.org/D10149

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/