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

Tue Oct 3 11:50:41 PDT 2017

MatzeB added a comment.

Thanks @violetav this looks a lot better as a self-contained pass!

================
Comment at: include/llvm/CodeGen/MachineInstr.h:795

+  bool isDirective() const { return isDebugValue() || isCFIInstruction(); }
   bool isPHI() const {
----------------
This name is unintuitive to me. And it feels like this isn't really a category of machine instructions but rather just a combination that `BranchFolding` is able to deal with after this patch.

Maybe it would be better to move this into BranchFolding.cpp then in the form of
`static bool isDirective(const MachineInstr &MI) { return MI.isDebugValue() || MI.isCFIInstruction(); }`.

================
Comment at: include/llvm/CodeGen/Passes.h:420-424
+  /// This pass verifies that outgoing cfa offset and register of predecessor
+  /// blocks match incoming cfa offset and register of their successors. Then it
+  /// checks if blocks have correct CFA calculation rule set and inserts
+  /// required CFI instruction at their beginnings if they don't (due to
+  /// non-linear block layout).
----------------
Even though there is a lot of precedent in this file, I'm starting to wonder how useful it is to have a description of a pass at the beginning of the passes implementation file and another one in front of the corresponding function in Passes.h. Maybe it would be best to shorten this to:
```
/// Creates CFI Instruction Inserter pass. \see CFIInstrInserter.cpp
```
so we only have to maintain one description of the passes details.

================
Comment at: include/llvm/Target/TargetFrameLowering.h:345-347
+  // Return initial CFA offset value i.e. the one valid at the beginning of the
+  // function (before any stack operations).
+  virtual int getInitialCFAOffset(MachineFunction &MF) const;
----------------
- Use doxygen `///` comments.
- I guess `const MachineFunction &MF` should work as well?

================
Comment at: include/llvm/Target/TargetFrameLowering.h:349-351
+  // Return initial CFA register value i.e. the one valid at the beginning of
+  // the function (before any stack operations).
+  virtual unsigned getInitialCFARegister(MachineFunction &MF) const;
----------------
ditto

================
Comment at: include/llvm/Target/TargetMachine.h:268-272
+
+  /// Check whether CFA info is maintained for this target. If not, it shouldn't
+  /// be verified for consistency and additional CFI instructions should not be
+  /// inserted to correct CFA calculation rule.
+  virtual bool maintainsCFAInfo() const { return false; }
----------------
I think we should rather add a TargetPassConfig method instead of another TargetMachine callback (see below).

================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:10-18
+// This pass verifies incoming and outgoing CFA information of basic blocks. CFA
+// information is information about offset and register set by CFI directives,
+// valid at the start and end of a basic block. This pass checks that outgoing
+// information of predecessors matches incoming information of their successors.
+// Then it checks if blocks have correct CFA calculation rule set and inserts
+// additional CFI instruction at their beginnings if they don't. CFI
+// instructions are inserted if basic blocks have incorrect offset or register
----------------
Use doxygen comments:
```
/// \file This pass verifies ...
```

================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:35
+  static char ID;
+  int ErrorNum;
+
----------------
This feels slightly out of place as a member variable. Why not make it a return value of `verify()`?

================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:53
+    calculateCFAInfo(MF);
+    verify(MF);
+    if (ErrorNum)
----------------
thegameg wrote:
> Should this be guarded by the `EXPENSIVE_CHECKS` macro?
I wouldn't go as far as EXPENSIVE_CHECKS. But I think `#ifndef NDEBUG` would be good to skip the verification in release builds.

================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:77-79
+  // Contains cfa offset and register values valid at entry and exit of basic
+  // blocks.
+  std::map<unsigned, MBBCFAInfo> MBBMap;
----------------
MachineBasicBlocks have a dense numbering, so you can use a more efficient std::vector of length `MachineFunction::getNumBlockIDs()` instead. (See MachineTraceMetrics::BlockInfo for an example)

================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:114
+char CFIInstrInserter::ID = 0;
+INITIALIZE_PASS(CFIInstrInserter, "CFIInstrInserter",
+                "Check CFA info and insert CFI instructions if needed", false,
----------------
We tend to use lowercase pass names here. So maybe `"cfi-instr-inserter"`?

================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:130
+  // Initialize MBBMap.
+  for (auto &MBB : MF) {
+    struct MBBCFAInfo MBBInfo;
----------------
Please avoid `auto` in cases where the actual type is only slightly longer. I think that is friendlier to readers of the code. (same for a number of other loops below).

================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:164-165
+  // Determine cfa offset and register set by the block.
+  for (MachineBasicBlock::instr_iterator MI = MBBInfo->MBB->instr_begin();
+       MI != MBBInfo->MBB->instr_end(); ++MI) {
+    if (MI->isCFIInstruction()) {
----------------
You could use `for (MachineInstr &MI : make_range(MBBInfo->MBB->instr_begin(), MBBInfo->MBB->instr_end()) { ... }` here.

================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:205
+  MBBCFAInfo PrevMBBInfo = MBBMap[MF.front().getNumber()];
+  MBBCFAInfo MBBInfo;
+  const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
----------------
You could move this declaration down to the use (and combine it with the first assignment).

================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:254
+void CFIInstrInserter::report(const char *msg, MachineBasicBlock &MBB) {
+  assert(&MBB);
+  ErrorNum++;
----------------
thegameg wrote:
> I think you can assume the reference is always non-null here, right?
References must be non-null otherwise it's undefined behavior. This means the compiler will most likely optimize this assert away and it won't do anything even if the reference happened to illegally be nullptr.

================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:266
+  ErrorNum = 0;
+  MBBCFAInfo CurrMBBInfo, PredMBBInfo, SuccMBBInfo;
+  for (auto &CurrMBB : MF) {
----------------
The declarations could be move downwards to their first use.

================
Comment at: lib/CodeGen/MachineInstr.cpp:331-347
+    if (op != OtherInst.getOperation()) return false;
+    if (op == MCCFIInstruction::OpDefCfa || op == MCCFIInstruction::OpOffset ||
+        op == MCCFIInstruction::OpRestore ||
+        op == MCCFIInstruction::OpUndefined ||
+        op == MCCFIInstruction::OpSameValue ||
+        op == MCCFIInstruction::OpDefCfaRegister ||
+        op == MCCFIInstruction::OpRelOffset ||
----------------
This looks like a good candidate for a `switch()`.

================
Comment at: lib/CodeGen/TargetFrameLoweringImpl.cpp:109-112
+  assert(!MF.getTarget().maintainsCFAInfo() &&
+         "Must implement getInitialCFAOffset() if target maintains CFA "
+         "information!");
+  return -1;
----------------
I'd simply use `llvm_unreachable("getInitialCFAOffset not implemented!");` and don't even bother checking the flag. Same in the next function.

================
Comment at: lib/CodeGen/TargetPassConfig.cpp:877-881
+  // Verify basic block incoming and outgoing cfa offset and register values and
+  // correct CFA calculation rule where needed by inserting appropriate CFI
+  // instructions.
+  if (TM->maintainsCFAInfo())
+    addPass(createCFIInstrInserter(), false);
----------------
- I'd vote to remove the `maintainsCFAInfo()` flag and simply let the targets that need it add the pass as part of `addPreEmitPass()`.
- Is the `, false` really necessary? I wouldn't expect the pass to produce anything that fails the machine verifier.

Repository:
  rL LLVM

https://reviews.llvm.org/D35844