[PATCH] D18046: [X86] Providing correct unwind info in function epilogue

[Violeta Vukobrat via llvm-commits]

violetav added inline comments.

================
Comment at: lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:577
@@ -573,1 +576,3 @@
       }
+      case MCCFIInstruction::OpDefCfa: {
+        break;
----------------
mkuper wrote:
> May also be worthwhile to add someone relevant to this review.
I posted a question to llvm-dev and the response http://lists.llvm.org/pipermail/llvm-dev/2016-March/097044.html made me think about the possible solution to the problem concerning generating compact unwind encoding as well as the problem of impacting eh_frame in the case when -g is passed. The solution consisted of marking CFI instructions with "frame destroy/frame setup" tags and then disregarding the CFI instructions marked with 'frame destroy' when generating compact unwind, or emitting eh_frame. However, the problem with this solution is the case when there are already generated .s files, and then .o files are generated: in that case, none of the CFI instructions are marked with 'frame setup/frame destroy' tags, because information about the tags does not exist in the .s file. Because of this, and because the response on the mailing list said that "The Darwin compact unwinder is never going to learn anything to its benefit from a CFI instruction in the epilogue.", I decided to disable the pass for Darwin.
Another thing that I found out is that when using gas to assemble the .s file generated by llvm, CFI instructions added to the epilogue would also be inserted in the eh_frame.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:103
@@ +102,3 @@
+  ST = &MF.getSubtarget<X86Subtarget>();
+  RI = ST->getRegisterInfo();
+  TFL = ST->getFrameLowering();
----------------
mkuper wrote:
> I don't think there are any plans to do that right now, unfortunately.
> Is there anything preventing you from generating this for .eh_frame?
No, there isn't. The CFI instructions that I'm generating already end up in eh_frame.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:238
@@ +237,3 @@
+      // Set offset that is set by prologue
+      InsertCFIInEpilogue(NewMBBInfo, NewMBBInfo->EndOffset);
+    }
----------------
mkuper wrote:
> This still seems somewhat odd.
> 
> The logic in InsertCFIInEpilogue made sense to me, when it was meant specifically for epilogues. But now, this will affect every block - including non-epilogue blocks that happen to pop / change esp.
>  
> If I understand what's going on correctly, this will add a .cfi_def_cfa_offset at the end of each block that modifies the cfa offset. But doesn't CorrectCFA already do this, regardless? Or am I getting something wrong here? In any case, perhaps it will be easier to understand once the names and comments are updated.
> 
No, InsertCFIInEpilogue() will add appropriate CFI instructions after each instruction that changes the SP (FP). For example, it will add a .cfi_def_cfa_offset with the updated offset after each pop instruction. I added a check to see whether instructions that cause the CFI instructions to be inserted are marked as FrameDestroy. That way, it can be assumed that they are part of the epilogue. On the other hand, CorrectCFA() inserts a CFI instruction at the beginning of a MBB if it's needed. The case when this is needed is when epilogue block (with updated CFI) comes before another MBB that should have the offset that is set by the prologue. Then, a cfi_def_cfa_offset is inserted at the beginning of that MBB so it would override the offset set in the epilogue, and therefore provide correct offset for the MBB in question.


Repository:
  rL LLVM

http://reviews.llvm.org/D18046