[PATCH] D42848: Correct dwarf unwind information in function epilogue

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added a reviewer: craig.topper.
thegameg added a subscriber: craig.topper.
thegameg added a comment.

Added @craig.topper for the `X86FrameLowering::mergeSPUpdates` changes.



================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:182
+        // TODO: Add support for handling cfi_remember_state.
+#ifndef NDEBUG
+        report_fatal_error(
----------------
violetav wrote:
> thegameg wrote:
> > Why are the `report_fatal_error`s guarded by `NDEBUG` here?
> I thought that possible wrong value of CFA should not be reported and abort compilation in release builds. Do you think that these should be reported as errors in release builds as well?
I'm not sure, but from what I see an `OpRemember/RestoreState` CFI_INSTRUCTION can only be created through MIR. I think if at some point we add code that generates one of these, we would want this pass to be updated as well, right?

Do you think reporting this error in release builds is unreasonable?


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:408
     NI = skipDebugInstructionsForward(NI, MBB.end());
+    NI = skipCFIInstructionsForward(NI, MBB.end());
+  }
----------------
The famous question: should this be `skipMetaInstructions(Backward|Forward)` or something similar (like you did in `countsAsInstruction`)?

I would at least merge the two functions (Debug + CFI) since this assumes we won't have a mix like: `DBG_VALUE, CFI_INSTRUCTION, DBG_VALUE, CFI_INSTRUCTION`.


================
Comment at: test/CodeGen/X86/O0-pipeline.ll:54
 ; CHECK-NEXT:       X86 vzeroupper inserter
+; CHECK-NEXT:       Check CFA info and insert CFI instructions if needed
 ; CHECK-NEXT:       Contiguously Lay Out Funclets
----------------
After [[ https://reviews.llvm.org/rL327917 | r327917 ]], the MachineOutliner will add some CFIs. Can you add a test where we make sure this pass runs afterwards?


================
Comment at: test/CodeGen/X86/throws-cfi-fp.ll:86
+attributes #0 = { "no-frame-pointer-elim"="true" }
+
+!llvm.dbg.cu = !{!2}
----------------
Is all this metadata necessary?


================
Comment at: test/CodeGen/X86/throws-cfi-no-fp.ll:85
+declare i32 @puts(i8* nocapture readonly)
+
+!llvm.dbg.cu = !{!2}
----------------
Same here, necessary?


Repository:
  rL LLVM

https://reviews.llvm.org/D42848