[DebugInfo] Fix crash when printing malformed DBG machine instructions

[foad]

Summary

MachineVerifier does not check that DBG_VALUE, DBG_VALUE_LIST and
DBG_INSTR_REF have the expected number of operands, so printing them
(e.g. with -print-after-all) should not crash.

[jmorse]

This sounds like a good direction to take. As for why the verifier doesn't check things, IIRC there are some MIR tests out there where we don't bother to feed in any metadata, so that you can put meaningless DBG_VALUE's in MIR tests to test behaviours with little overhead. And which then leads to the behaviour you're patching against here.

Just to confirm my understanding: this is only suppressing printing the operands of debug instructions when there aren't enough operands, yes? It'd be unfortunate if we completely skipped printing an instruction because it was malformed, thus hiding it from developers.

llvm/lib/CodeGen/MachineInstr.cpp

1886–1899

I believe these should be >= operators rather than == . The instructions that can be variadic may have several operands or none, and we want the Variable and DebugLoc printed in those scenarios.

[foad]

Just to confirm my understanding: this is only suppressing printing the operands of debug instructions when there aren't enough operands, yes? It'd be unfortunate if we completely skipped printing an instruction because it was malformed, thus hiding it from developers.

Correct, it only suppresses pretty-printing of the debug variable info as a comment.

As an alternative approach maybe all the accessors like getDebugVariableOp should check getNumOperands and return a failure value if the operand does not exist. But I do not want to get too deeply involved in this, I just wanted to stop llc -print-after-all from crashing.

llvm/lib/CodeGen/MachineInstr.cpp

1886–1899

OK. I was copying from MachineVerifier which does this:

// A fully-formed DBG_VALUE must have a location. Ignore partially formed
// DBG_VALUEs: these are convenient to use in tests, but should never get
// generated.
if (MI->isDebugValue() && MI->getNumOperands() == 4)
  if (!MI->getDebugLoc())
    report("Missing DebugLoc for debug instruction", MI);

But I guess that is slightly wrong for DBG_VALUE (should be >= 4), very wrong for DBG_VALUE_LIST (should be >= 2) and completely misses DBG_INSTR_REF.

[jmorse]

(LGTM with the nit inline fixed)

llvm/lib/CodeGen/MachineInstr.cpp

1886–1899

Interesting, paging @StephenTozer , is the MachineVerifier accidentally skipping verification of DBG_VALUE_LIST?

Even in the verifier is wrong though, IMO this patch should use the greater-than test to ensure variadic info gets printed.

[foad]

Use >=.

llvm/lib/CodeGen/MachineInstr.cpp

if (const DebugLoc &DL = getDebugLoc()) {
     if (!HaveSemi) {
       OS << ';';
       HaveSemi = true;
     }
     OS << ' ';
     DL.print(OS);
   }
 
-  // Print extra comments for DEBUG_VALUE.
-  if (isDebugValueLike() && getDebugVariableOp().isMetadata()) {
+  // Print extra comments for DEBUG_VALUE and friends if they are well-formed.
+  if ((isNonListDebugValue() && getNumOperands() >= 4) ||
+      (isDebugValueList() && getNumOperands() >= 2) ||
+      (isDebugRef() && getNumOperands() >= 3)) {
+    if (getDebugVariableOp().isMetadata()) {
       if (!HaveSemi) {
         OS << ";";
         HaveSemi = true;
       }
       auto *DV = getDebugVariable();
-    OS << " line no:" <<  DV->getLine();
+      OS << " line no:" << DV->getLine();
       if (isIndirectDebugValue())
         OS << " indirect";
     }
+  }
   // TODO: DBG_LABEL
 
   if (AddNewLine)
     OS << '\n';
 }
 
 bool MachineInstr::addRegisterKilled(Register IncomingReg,
                                      const TargetRegisterInfo *RegInfo,

llvm/test/CodeGen/MIR/Generic/dbg-value.mir

+# RUN: llc -run-pass=machineinstr-printer -o - %s | FileCheck %s
+
+---
+name: test
+body: |
+  bb.0:
+    ; CHECK-LABEL: name: test
+    ; CHECK: DBG_VALUE
+    ; CHECK-NEXT: DBG_VALUE_LIST
+    ; CHECK-NEXT: DBG_INSTR_REF
+    DBG_VALUE
+    DBG_VALUE_LIST
+    DBG_INSTR_REF
+...