[PATCH] D13767: [X86] Fix more -Os + EH issues

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Thanks, Reid!


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp:231-233
@@ -230,2 +230,5 @@
     break;
+  case MCCFIInstruction::OpAdjustCfaOffset:
+    OutStreamer->EmitCFIAdjustCfaOffset(Inst.getOffset());
+    break;
   case MCCFIInstruction::OpDefCfa:
----------------
rnk wrote:
> Hah, I guess we have MC support for .cfi_adjust_offset, but never used if from the compiler. :)
Yup.
It's used by the assembler, though. :-)

================
Comment at: lib/Target/X86/X86CallFrameOptimization.cpp:140
@@ -137,1 +139,3 @@
+       (MF.getFunction()->needsUnwindTableEntry() && 
+        !STI.getFrameLowering()->hasFP(MF))))
     return false;
----------------
rnk wrote:
> Isn't `STI.getFrameLowering()` just `TFL`?
Right, thanks!

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2097
@@ -2096,3 +2096,3 @@
     bool HasDwarfEHHandlers =
       !MF.getTarget().getMCAsmInfo()->usesWindowsCFI() &&
       !MF.getMMI().getLandingPads().empty();
----------------
rnk wrote:
> Can we just assert up front that we don't need to produce UNWIND_INFO .xdata for Windows? This call frame optimization stuff only fires for 32-bit x86, which doesn't use UNWIND_INFO.
Sure. Should I just assert on usesWindowsCFI()?

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:2110-2111
@@ +2109,4 @@
+    // we need to set the correct CFA offset at every call site.
+    // TODO: Debug info really needs precise CFA offsets after each stack
+    // adjustment, it's not enough for it to be correct at callsites. 
+    if (MF.getFunction()->needsUnwindTableEntry() && !hasFP(MF))
----------------
rnk wrote:
> I think we should address this now. We know exactly where we're inserting the pushes in `X86CallFrameOptimization::adjustCallSequence`, so all we have to do is:
> - Insert `.cfi_adjust_cfa_offset 4` after each push
> - Insert `.cfi_adjust_cfa_offset InternalAmt` for frame destruction here
> - Insert `.cfi_adjust_cfa_offset Amount` after `BuildStackAdjustment` below
The question is whether we want to always have the precise adjusts or have two separate code paths for the precise version and the call-site-only version.  This depends on whether we care about the eh_frame size for -Os, or not. 

If we want it to always be precise, then what you're suggesting is the right way to go. If we don't, then there should be two separate code paths, the one here, and the one you suggest, and I wanted them to be two separate patches.


http://reviews.llvm.org/D13767