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

[Violeta Vukobrat via Phabricator via llvm-commits]

violetav added inline comments.


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:182
+        // TODO: Add support for handling cfi_remember_state.
+#ifndef NDEBUG
+        report_fatal_error(
----------------
thegameg wrote:
> 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?
> 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?

Yes.


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

I don't think it's unreasonable, I just thought that if verifier doesn't report errors in release builds, neither should these. Maybe there could be an option, to emit warnings, erorrs, or to ignore inconsistent CFA values altogether. Different build types could have different default values set for this option.




================
Comment at: lib/Target/X86/X86FrameLowering.cpp:408
     NI = skipDebugInstructionsForward(NI, MBB.end());
+    NI = skipCFIInstructionsForward(NI, MBB.end());
+  }
----------------
thegameg wrote:
> 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`.
I changed the code in mergeSPUpdates(), so that it supports the case when ADD/SUB/LEA is succeded by one CFI instruction, and that there are no DBG_VALUE or other instructions between ADD/SUB/LEA and its corresponding CFI instruction. The case where there are multiple CFI instructions below the ADD/SUB/LEA, e.g.:
...
add
cfi_def_cfa_offset
cfi_offset
...
is not currently supported.


================
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
----------------
thegameg wrote:
> 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?
I moved CFIInstrInserter pass from preEmitPass() (runs before MachineOutliner) to preEmitPass2() (runs after outliner).
I added a test that checks that CFIInstrInserter runs after passes that add CFI instructions.


Repository:
  rL LLVM

https://reviews.llvm.org/D42848