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

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added a comment.
Herald added a reviewer: javed.absar.

Thanks for working on this again!



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


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:408
     NI = skipDebugInstructionsForward(NI, MBB.end());
+    NI = skipCFIInstructionsForward(NI, MBB.end());
+  }
----------------
violetav wrote:
> 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.
All right this could work for now. Can you document that somewhere? Can you put this explanation in a comment below with a TODO/FIXME?


================
Comment at: test/CodeGen/X86/GlobalISel/add-scalar.ll:23
 ; X32-NEXT:    popl %ebp
+; X32-NEXT:    .cfi_def_cfa %esp, 4
 ; X32-NEXT:    retl
----------------
I think this change made sense before. What happened?


================
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
----------------
violetav wrote:
> 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.
Ideally this would be part of the AsmPrinter since it's pretty much "fixing" the CFI based on the final layout but doesn't modify the code. I wonder what was meant to be added in `preEmitPass2` instead of the retpoline pass, but I think this is pretty much OK to add it here. If not, just making sure that this runs after the outliner (or whatever pass deals with CFI last) should also be fine.


================
Comment at: test/CodeGen/X86/avx512-regcall-Mask.ll:197
 ; LINUXOSX64-NEXT:    addq $24, %rsp
-; LINUXOSX64-NEXT:    .cfi_adjust_cfa_offset -16
+; LINUXOSX64-NEXT:    .cfi_adjust_cfa_offset -24
 ; LINUXOSX64-NEXT:    popq %r12
----------------
Hmm, I still don't really understand this change.


Repository:
  rL LLVM

https://reviews.llvm.org/D42848