[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:
> Have you checked this with the Darwin people?
> I don't know enough (=anything) about compact encoding, so can't really say whether these changes make sense or not.
No, I haven't. I will post a question to llvm-dev.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:102
@@ +101,3 @@
+
+  if (!MF.getMMI().hasDebugInfo()) {
+    return false;
----------------
mkuper wrote:
> mkuper wrote:
> > The LLVM convention is to generally frown upon braces for single-line blocks, except when needed for readability. This isn't a hard-and-fast rule (and clang-format does not enforce it), but to be consistent with the rest of the code base, I'd suggest removing them.
> What if we don't have debug info, but have EH?
> On the one hand, I'm not sure whether we care about CFA in the epilogue being correct for EH. On the other hand, I think we've already decided that we don't want -g being passed to cause LLVM to modify .eh_frame. And since we currently can't generate different .eh_frame and .debug_frame, this will happen.
I didn't know about the decision concerning -g and .eh_frame. Yes, these changes would modify .eh_frame now, since there is no support for saying which CFI directives should go in .eh_frame, and which in .debug_frame. Are there any plans for adding the support for generating different .eh_frame and .debug_frame?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:123
@@ +122,3 @@
+
+  MBBInfoList = new struct MBBInfo[MF.size()];
+  BlocksToAnalyze = new std::queue<int>();
----------------
mkuper wrote:
> Why are these two on the heap?
I changed BlocksToAnalyze not to be, as there was no particular reason for it. And as for MBBInfoList, I wanted to create an array and index its elements so I created one on heap because its length is variable (number of MBBs in MF). Should I use SmallVector or something else instead?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:219
@@ +218,3 @@
+                                MBBI->getOpcode() == X86::POP64r)) {
+          FoundEpilogue = true;
+        }
----------------
mkuper wrote:
> This doesn't look right. Why would the first block that has pops be the epilogue?
> 
> More generally, will it make sense to mark the epilogue block while it's emitted? Sure, it can move it around, but do we expect it to be split? If we do, and can't mark it ahead of time, then I think we need a better way to recognize it.
FoundEpilogue is a badly named variable. The thing I am trying to recognize here isn't 'the epilogue', instead, I'm looking for each BB that contains instructions that change the value of SP (or FP). That is what I consider to be 'an epilogue' here. The flag FoundEpilogue would probably be better called something such as ShouldUpdateCFI, because it is set to true when a BB contains instructions that cause the need for adding CFI instructions that update values of offset/register.
As for marking the epilogue block while it's emitted, in addition to it moving around, yes, it can be split, it can also 'disappear' (by merging into the previous block). In my opinion, maintaining info about a BB being an epilogue during all passes would be complicated and error prone.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:354
@@ +353,3 @@
+        ++MBBI;
+        CurrentOffset -= InitialOffset;
+        TFL->BuildCFI(
----------------
mkuper wrote:
> Oh, so you did mean "pop %esp"? If not, then shouldn't this be SlotSize?
> If you did, then is the check for the pop argument missing? And what happens for a pop to a different register?
No, the comment is wrong, I did not mean "pop %esp". InitialOffset is set to SlotSize at the beginning.


Repository:
  rL LLVM

http://reviews.llvm.org/D18046