[PATCH] D156226: [DebugInfo] Fix crash when printing malformed DBG machine instructions

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

(LGTM with the nit inline fixed)



================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:1886-1889
+  // Print extra comments for DEBUG_VALUE and friends if they are well-formed.
+  if ((isNonListDebugValue() && getNumOperands() == 4) ||
+      (isDebugValueList() && getNumOperands() == 2) ||
+      (isDebugRef() && getNumOperands() == 3)) {
----------------
foad wrote:
> jmorse wrote:
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156226/new/

https://reviews.llvm.org/D156226