[Debugify] Diagnose mis-sized dbg.values

[vsk]

Summary

Report an error in -check-debugify when the size of a dbg.value operand
doesn't match up with the size of the variable it describes.

Eventually this check should be moved into the IR verifier. For the
moment, it's useful to include the check in -check-debugify as a means
of catching regressions and finding existing bugs.

Here are some instances of bugs the new check finds in the -O2 pipeline:

1. A float is used where a double is expected:

ERROR: dbg.value operand has size 32, but its variable has size 64:
call void @llvm.dbg.value(metadata float %expf, metadata !12, metadata
!DIExpression()), !dbg !15

2. An i8 is used where an i32 is expected:

ERROR: dbg.value operand has size 8, but its variable has size 32:
call void @llvm.dbg.value(metadata i8 %t4, metadata !14, metadata
!DIExpression()), !dbg !24

3. A <4 x i32> is used where something twice as large is expected

(perhaps a <4 x i64>, I haven't double-checked):

ERROR: dbg.value operand has size 128, but its variable has size 256:
call void @llvm.dbg.value(metadata <4 x i32> %4, metadata !40, metadata
!DIExpression()), !dbg !95

[vsk]

• Simplify the commit description.
• Use a helper function to clean up the code.
• Remove the exception for integer values which are larger than their described variables, as it's not clear whether debuggers can always truncate to display the right value.

[vsk]

• Mark a variable as missing only if the mis-sized diagnostic fires.

[probinson]

I remember there being a difference of opinion about the "correct" size of an 80-bit float in a 128-bit container. How would that case be handled here?

[vsk]

In D48408#1141148, @probinson wrote:

I remember there being a difference of opinion about the "correct" size of an 80-bit float in a 128-bit container. How would that case be handled here?

That's a good question. Currently, we emit IR that would not trigger the diagnostic this patch introduces (https://godbolt.org/g/7DXyJy):

call void @llvm.dbg.value(metadata x86_fp80 %10, metadata !14, metadata !DIExpression())
!10 = !DIBasicType(name: "long double", size: 128, encoding: DW_ATE_float)
!14 = !DILocalVariable(name: "x", scope: !7, file: !1, line: 2, type: !10)

getSizeInBits() reports back 128 for an x86_fp80.

[probinson]

Hm. DataLayout is willing to report back one of three possible sizes: the size of a value; the size written by a store; and the stride of an array. getTypeAllocSize() returns the third of these, which would include alignment padding as needed.
Is that really the value size we expect from dbg.value?

[vsk]

I think I need to bring back the exception for integers. Adrian pointed out (in https://reviews.llvm.org/D48331) that DI variables with a smaller size than a dbg.value operand implicitly use the lower bits. This works for the integer case.

In D48408#1143487, @probinson wrote:

Hm. DataLayout is willing to report back one of three possible sizes: the size of a value; the size written by a store; and the stride of an array. getTypeAllocSize() returns the third of these, which would include alignment padding as needed.
Is that really the value size we expect from dbg.value?

Oh I see, thanks for pointing this distinction out.

In the example I shared above, at -O0, the stack slot for the x86_fp80 is 128 bits in size (https://godbolt.org/g/B89MwC). LLVM generates a ST_FP80m to convert the fp80 into "double extended-precision floating-point format", which it then stores into this slot. So I think 128 bits makes sense as the size: anything else could confuse the debugger about what's on the stack. I'm not sure how we'd support debugging fp80's which are "in" the FPU, i.e which are not spilled. FWIW, we tend to spill (-O2, https://godbolt.org/g/uwpMYY).

In general then, I think getAllocSizeInBits is the appropriate size to use, because it captures the fact that conversions may be needed to place the value in a stack slot.

[vsk]

• Bring back the exception for an integer dbg.value operand which is wider than the DILocalVariable it's associated with. We're relying on the debugger being able to simply use the low bits of the integer in this scenario in a few different places, and it appears to work.

[probinson]

A couple of naming suggestions but LGTM.

tools/opt/Debugify.cpp

43 ↗

(On Diff #152952)

Given the discussion about sizes, maybe getAllocSizeInBits()?

187 ↗

(On Diff #152952)

Maybe "ValueSizeInBits" and "FragmentSizeInBits" to be more descriptive than judgmental.

llvm/trunk/test/DebugInfo/debugify-bogus-dbg-value.ll

+; RUN: opt -check-debugify < %s 2>&1 | FileCheck %s
+
+define <2 x i64> @test-fun(<2 x i64> %A) !dbg !6 {
+  %and = and <2 x i64> %A, <i64 23, i64 42>, !dbg !14
+
+; CHECK: ERROR: dbg.value operand has size 128, but its variable has size 256
+  call void @llvm.dbg.value(metadata <2 x i64> %and, metadata !12, metadata !DIExpression()), !dbg !15
+
+; CHECK: ERROR: dbg.value operand has size 512, but its variable has size 256
+  call void @llvm.dbg.value(metadata <2 x i256> zeroinitializer, metadata !12, metadata !DIExpression()), !dbg !15
+
+; CHECK: ERROR: dbg.value operand has size 8, but its variable has size 256
+  call void @llvm.dbg.value(metadata i8 0, metadata !12, metadata !DIExpression()), !dbg !15
+
+; Assume the debugger implicitly uses the lower 256 bits.
+; CHECK-NOT: ERROR: dbg.value operand has size 512, but its variable has size 256
+  call void @llvm.dbg.value(metadata i512 0, metadata !12, metadata !DIExpression()), !dbg !15
+
+  ret <2 x i64> %and, !dbg !16
+}
+; CHECK: CheckModuleDebugify: FAIL
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+attributes #0 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!3, !4}
+!llvm.module.flags = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "/Users/vsk/src/llvm.org-master/llvm/test/DebugInfo/debugify-bogus-dbg-value.ll", directory: "/")
+!2 = !{}
+!3 = !{i32 4}
+!4 = !{i32 3}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "test-fun", linkageName: "test-fun", scope: null, file: !1, line: 1, type: !7, isLocal: false, isDefinition: true, scopeLine: 1, isOptimized: true, unit: !0, retainedNodes: !8)
+!7 = !DISubroutineType(types: !2)
+!8 = !{!9, !11, !12}
+!9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 1, type: !10)
+!10 = !DIBasicType(name: "ty64", size: 64, encoding: DW_ATE_unsigned)
+!11 = !DILocalVariable(name: "2", scope: !6, file: !1, line: 2, type: !10)
+!12 = !DILocalVariable(name: "3", scope: !6, file: !1, line: 3, type: !13)
+
+; Set the size here to an incorrect value to check that the size diagnostic
+; triggers.
+!13 = !DIBasicType(name: "ty128", size: 256, encoding: DW_ATE_unsigned)
+
+!14 = !DILocation(line: 2, column: 1, scope: !6)
+!15 = !DILocation(line: 3, column: 1, scope: !6)
+!16 = !DILocation(line: 4, column: 1, scope: !6)

llvm/trunk/tools/opt/Debugify.cpp

 
 namespace {
 
 cl::opt<bool> Quiet("debugify-quiet",
                     cl::desc("Suppress verbose debugify output"));
 
 raw_ostream &dbg() { return Quiet ? nulls() : errs(); }
 
+uint64_t getAllocSizeInBits(Module &M, Type *Ty) {
+  return Ty->isSized() ? M.getDataLayout().getTypeAllocSizeInBits(Ty) : 0;
+}
+
 bool isFunctionSkipped(Function &F) {
   return F.isDeclaration() || !F.hasExactDefinition();
 }
 
 /// Find the basic block's terminating instruction.
 ///
 /// Special care is needed to handle musttail and deopt calls, as these behave
 /// like (but are in fact not) terminators.
bool applyDebugifyMetadata(Module &M,
   }
 
   DIBuilder DIB(M);
   LLVMContext &Ctx = M.getContext();
 
   // Get a DIType which corresponds to Ty.
   DenseMap<uint64_t, DIType *> TypeCache;
   auto getCachedDIType = [&](Type *Ty) -> DIType * {
-    uint64_t Size =
-        Ty->isSized() ? M.getDataLayout().getTypeAllocSizeInBits(Ty) : 0;
+    uint64_t Size = getAllocSizeInBits(M, Ty);
     DIType *&DTy = TypeCache[Size];
     if (!DTy) {
       std::string Name = "ty" + utostr(Size);
       DTy = DIB.createBasicType(Name, Size, dwarf::DW_ATE_unsigned);
     }
     return DTy;
   };
 
bool applyDebugifyMetadata(Module &M,
   // Claim that this synthetic debug info is valid.
   StringRef DIVersionKey = "Debug Info Version";
   if (!M.getModuleFlag(DIVersionKey))
     M.addModuleFlag(Module::Warning, DIVersionKey, DEBUG_METADATA_VERSION);
 
   return true;
 }
 
+/// Return true if a mis-sized diagnostic is issued for \p DVI.
+bool diagnoseMisSizedDbgValue(Module &M, DbgValueInst *DVI) {
+  // The size of a dbg.value's value operand should match the size of the
+  // variable it corresponds to.
+  //
+  // TODO: This, along with a check for non-null value operands, should be
+  // promoted to verifier failures.
+  Value *V = DVI->getValue();
+  if (!V)
+    return false;
+
+  // For now, don't try to interpret anything more complicated than an empty
+  // DIExpression. Eventually we should try to handle OP_deref and fragments.
+  if (DVI->getExpression()->getNumElements())
+    return false;
+
+  Type *Ty = V->getType();
+  uint64_t ValueOperandSize = getAllocSizeInBits(M, Ty);
+  uint64_t DbgVarSize = *DVI->getFragmentSizeInBits();
+  bool HasBadSize = Ty->isIntegerTy() ? (ValueOperandSize < DbgVarSize)
+                                      : (ValueOperandSize != DbgVarSize);
+  if (HasBadSize) {
+    dbg() << "ERROR: dbg.value operand has size " << ValueOperandSize
+          << ", but its variable has size " << DbgVarSize << ": ";
+    DVI->print(dbg());
+    dbg() << "\n";
+  }
+  return HasBadSize;
+}
+
 bool checkDebugifyMetadata(Module &M,
                            iterator_range<Module::iterator> Functions,
                            StringRef NameOfWrappedPass, StringRef Banner,
                            bool Strip) {
   // Skip modules without debugify metadata.
   NamedMDNode *NMD = M.getNamedMetadata("llvm.debugify");
   if (!NMD) {
     dbg() << Banner << "Skipping module without debugify metadata\n";
for (Instruction &I : instructions(F)) {
 
       dbg() << "ERROR: Instruction with empty DebugLoc in function ";
       dbg() << F.getName() << " --";
       I.print(dbg());
       dbg() << "\n";
       HasErrors = true;
     }
 
-    // Find missing variables.
+    // Find missing variables and mis-sized debug values.
     for (Instruction &I : instructions(F)) {
       auto *DVI = dyn_cast<DbgValueInst>(&I);
       if (!DVI)
         continue;
 
       unsigned Var = ~0U;
       (void)to_integer(DVI->getVariable()->getName(), Var, 10);
       assert(Var <= OriginalNumVars && "Unexpected name for DILocalVariable");
+      bool HasBadSize = diagnoseMisSizedDbgValue(M, DVI);
+      if (!HasBadSize)
         MissingVars.reset(Var - 1);
+      HasErrors |= HasBadSize;
     }
   }
 
   // Print the results.
   for (unsigned Idx : MissingLines.set_bits())
     dbg() << "WARNING: Missing line " << Idx + 1 << "\n";
 
   for (unsigned Idx : MissingVars.set_bits())