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

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Thanks for working on the patch. We count on it to enable libunwind.



================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:80-82
+  /// Contains cfa offset and register values valid at entry and exit of basic
+  /// blocks.
+  SmallVector<struct MBBCFAInfo, 4> MBBVector;
----------------
The size of the vector should be equal or larger than the number of BBs inside of MF, so not quite small . Consider use std::vector?


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:163-164
+  // Determine cfa offset and register set by the block.
+  for (MachineInstr &MI :
+       make_range(MBBInfo.MBB->instr_begin(), MBBInfo.MBB->instr_end())) {
+    if (MI.isCFIInstruction()) {
----------------
equivalent to: for (MachineInstr &MI : *MBBInfo.MBB)


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:168-177
+      if (CFI.getOperation() == MCCFIInstruction::OpDefCfaRegister) {
+        SetRegister = CFI.getRegister();
+      } else if (CFI.getOperation() == MCCFIInstruction::OpDefCfaOffset) {
+        SetOffset = CFI.getOffset();
+      } else if (CFI.getOperation() == MCCFIInstruction::OpAdjustCfaOffset) {
+        SetOffset += CFI.getOffset();
+      } else if (CFI.getOperation() == MCCFIInstruction::OpDefCfa) {
----------------
This is an incomplete list of CFI operations. Theoretically it is possible that OpRememberState/OpRestoreState/... is inserted before CFIInstrInserter pass, then the calculation result here may be incorrect. For those unsupported operations, consider adding an error report and a TODO here? 


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:263
+    const struct MBBCFAInfo& CurrMBBInfo = MBBVector[CurrMBB.getNumber()];
+    for (MachineBasicBlock *Pred : CurrMBB.predecessors()) {
+      const struct MBBCFAInfo& PredMBBInfo = MBBVector[Pred->getNumber()];
----------------
We compare predecessors and successors of each BB, then each pair of BB are compared twice. Is the iteration of predecessors only or successors only enough? 


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:268-274
+        report("The outgoing offset of a predecessor is inconsistent.",
+               CurrMBB);
+        errs() << "Predecessor BB#" << Pred->getNumber()
+               << " has outgoing offset (" << PredMBBInfo.OutgoingCFAOffset
+               << "), while BB#" << CurrMBB.getNumber()
+               << " has incoming offset (" << CurrMBBInfo.IncomingCFAOffset
+               << ").\n";
----------------
The report here and below looks cumbersome. Maybe change CFIInstrInserter::report to:
```void CFIInstrInserter::report(MachineBasicBlock &Pred, MachineBasicBlock &Succ) {
    errs() << "*** " << Inconsistent CFA Reg and Offset between pred and succ << " ***\n"
    errs() << "Pred outgoing CFA Reg:" << Pred.OutgoingCFARegister;
    errs() << "Pred outgoing CFA Offset:" << Pred.OutgoingCFAOffset;
    errs() << "Succ incoming CFA Reg:" << Succ.IncomingCFARegister;
    errs() << "Succ incoming CFA Offset:" << Pred.IncomingCFAOffset;
}```
So all the error reports here and below can share the same function.


Repository:
  rL LLVM

https://reviews.llvm.org/D42848