[DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue

[bjope]

Summary

Do not convert a DbgDeclare to DbgValue if the store
instruction only refer to a fragment of the variable
described by the DbgDeclare.

Problem was seen when for example having an alloca for an
array or struct, and there were stores to individual elements.
In the past we inserted a DbgValue intrinsics for each store,
just as if the store wrote the whole variable.

When handling store instructions we insert a DbgValue that
indicates that the variable is "undefined", as we do not know
which part of the variable that is updated by the store.

When ConvertDebugDeclareToDebugValue is used with a load/phi
instruction we assert that the referenced value is large enough
to cover the whole variable. Afaict this should be true for all
scenarios where those methods are used on trunk. If the assert
blows in the future I guess we could simply skip to insert a
dbg.value instruction.

In the future I think we should examine which part of the variable
that is accessed, and add a DbgValue instrinsic with an appropriate
DW_OP_LLVM_fragment expression.

[aprantl]

lib/Transforms/Utils/Local.cpp

1231 ↗

(On Diff #150744)

... by a llvm.dbg intrinsic.

1232 ↗

(On Diff #150744)

I think that getSizeOfFragment() would be more a more accurate name?

1237 ↗

(On Diff #150744)

Unknown

Is this really unreachable, i.e.: would the Verifier reject a variable with a size of zero? In C (but not C++) I believe the size of struct Empty {}; is zero. In Swift you can also have zero-sized types.

1242 ↗

(On Diff #150744)

How about: fragmentCoversEntireVariable()

1272 ↗

(On Diff #150744)

This part seems reasonable (though sort of unsatisfying :-).

[bjope]

lib/Transforms/Utils/Local.cpp

1237 ↗

(On Diff #150744)

We only end up in the unreachable if getSizeInBits failed to determine the size (it returns an Optional<uint64_t>). So *VarSize could still be zero.

But maybe we need to handle the situation more gracefully. I should probably let this method return an Optional<uint64_t> as well, and then deal with it somehow in IsVariableCoveredByType.

1242 ↗

(On Diff #150744)

In my world the variable and the fragment is both related to the same thing.
But I can change it to: valueCoversEntireFragment(Type *Ty, DbgInfoIntrinsic *DII)

[aprantl]

lib/Transforms/Utils/Local.cpp

1237 ↗

(On Diff #150744)

Oh, I didn't realize that this was an Optional! That said, the new code must not throw an unreachable for any input that is accepted by the Verifier. Both making the new code more permissive or tightening the Verifier are acceptable solutions.

1242 ↗

(On Diff #150744)

That name makes sense to me, perhaps the doxygen comment can be improved, too, by removing the negation.

[bjope]

Updated after comments from aprantl.

Also added the new getFragmentSize() method to class DbgInfoIntrinsic.

[bjope]

Additional cleanups.

[aprantl]

include/llvm/IR/IntrinsicInst.h

98 ↗

(On Diff #150784)

getFragmentSizeInBits() ?

I'm still curious whether it makes sense to support the case where this returns None, or if we could add a Verifier check that is effectively assert_di(getFragmentSize()). Doesn't need to be in this commit though.

lib/Transforms/Utils/Local.cpp

1233 ↗

(On Diff #150784)

Are all functions in this file starting with an upper case letter? Otherwise it would be better to follow the LLVM naming conventions. Perhaps also just doing this in a later NFC commit.

1310 ↗

(On Diff #150784)

I think we usually spell out the error, not the assertion: load is not loading the full variable fragment

[bjope]

More updates after review.

[aprantl]

Few more small nitpicks inline but otherwise LGTM.

include/llvm/IR/IntrinsicInst.h

96 ↗

(On Diff #150931)

I think the last comma is not necessary in this sentence.

lib/IR/IntrinsicInst.cpp

54 ↗

(On Diff #150931)

Why not just
return getVariable()->getSizeInBits() ?

lib/Transforms/Utils/Local.cpp

1258 ↗

(On Diff #150931)

variable's

[bjope]

After some more testing I discovered a case when the added assert in

void llvm::ConvertDebugDeclareToDebugValue(DbgInfoIntrinsic *DII,
                                           PHINode *APN, DIBuilder &Builder)

failed.

I think it is more correct to use getTypeStoreSizeInBits() instead of getTypeSizeInBits() in valueCoversEntireFragment(), as the store size tells the number of bits that would be overwritten if writing the value to memory (and the dbg.declare that we want to replace describes memory).

So this update is adding a test case that would trigger the assert, and an update in valueCoversEntireFragment() to use the store size.

@adrian: Is this still good?

[bjope]

include/llvm/IR/IntrinsicInst.h

98 ↗

(On Diff #150784)

I don't know if we can do that check in the verifier. I guess it is bad if we can't determine the size of the variable (as we would lose debug info).

All I know is that there is a comment about "broken types" in the implementation of DIVariable::getSizeInBits():

Optional<uint64_t> DIVariable::getSizeInBits() const {
  // This is used by the Verifier so be mindful of broken types.
  ...

I have no idea if those "broken types" only exist in handwritten / poorly stripped IR, or if they are common in real use cases.

[aprantl]

Still works for me, but please add a comment explaining why getStoreSize is used here and what that means.

include/llvm/IR/IntrinsicInst.h

98 ↗

(On Diff #150784)

That "broken types" comment refers to the need for the Verifier to be robust against malformed debug metadata. The Verifier should not crash in the presence of malformed input, and it also shouldn't accept malformed input. Given the many embedded uses of LLVM both of these situations could be conceivably security vulnerabilities.

[morehouse]

An assertion introduced by this patch is failing on a build bot: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/25013

Please take a look.

[bjope]

In D48024#1132496, @morehouse wrote:

An assertion introduced by this patch is failing on a build bot: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/25013

Please take a look.

Ok, I'll revert and then try to reproduce the fault. Thanks for the notification!

llvm/trunk/include/llvm/IR/IntrinsicInst.h

public:
     Metadata *getRawVariable() const {
       return cast<MetadataAsValue>(getArgOperand(1))->getMetadata();
     }
 
     Metadata *getRawExpression() const {
       return cast<MetadataAsValue>(getArgOperand(2))->getMetadata();
     }
 
+    /// Get the size (in bits) of the variable, or fragment of the variable that
+    /// is described.
+    Optional<uint64_t> getFragmentSizeInBits() const;
+
     /// \name Casting methods
     /// @{
     static bool classof(const IntrinsicInst *I) {
       switch (I->getIntrinsicID()) {
       case Intrinsic::dbg_declare:
       case Intrinsic::dbg_value:
       case Intrinsic::dbg_addr:
       case Intrinsic::dbg_label:

llvm/trunk/lib/IR/IntrinsicInst.cpp

 // type pointer to empty struct { }*.  To access the real item of interest the
 // cast instruction needs to be stripped away.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
 /// DbgInfoIntrinsic - This is the common base class for debug info intrinsics
 ///
 
 Value *DbgInfoIntrinsic::getVariableLocation(bool AllowNullOp) const {
   Value *Op = getArgOperand(0);
   if (AllowNullOp && !Op)
     return nullptr;
 
   auto *MD = cast<MetadataAsValue>(Op)->getMetadata();
   if (auto *V = dyn_cast<ValueAsMetadata>(MD))
     return V->getValue();
 
   // When the value goes to null, it gets replaced by an empty MDNode.
   assert(!cast<MDNode>(MD)->getNumOperands() && "Expected an empty MDNode");
   return nullptr;
 }
 
+Optional<uint64_t> DbgInfoIntrinsic::getFragmentSizeInBits() const {
+  if (auto Fragment = getExpression()->getFragmentInfo())
+    return Fragment->SizeInBits;
+  return getVariable()->getSizeInBits();
+}
+
 int llvm::Intrinsic::lookupLLVMIntrinsicByName(ArrayRef<const char *> NameTable,
                                                StringRef Name) {
   assert(Name.startswith("llvm."));
 
   // Do successive binary searches of the dotted name components. For
   // "llvm.gc.experimental.statepoint.p1i8.p1i32", we will find the range of
   // intrinsics starting with "llvm.gc", then "llvm.gc.experimental", then
   // "llvm.gc.experimental.statepoint", and then we will stop as the range is

llvm/trunk/lib/Transforms/Utils/Local.cpp

static bool PhiHasDebugValue(DILocalVariable *DIVar,
   for (auto *DVI : DbgValues) {
     assert(DVI->getValue() == APN);
     if ((DVI->getVariable() == DIVar) && (DVI->getExpression() == DIExpr))
       return true;
   }
   return false;
 }
 
+/// Check if the store size of \p ValTy is large enough to cover the variable
+/// (or fragment of the variable) described by \p DII.
+///
+/// This is primarily intended as a helper for the different
+/// ConvertDebugDeclareToDebugValue functions. The dbg.declare/dbg.addr that is
+/// converted describes an alloca'd variable, so we need to use the
+/// store size of the value when doing the comparison. E.g. an i1 value will be
+/// identified as covering an n-bit fragment, if the store size of i1 is at
+/// least n bits.
+static bool valueCoversEntireFragment(Type *ValTy, DbgInfoIntrinsic *DII) {
+  const DataLayout &DL = DII->getModule()->getDataLayout();
+  uint64_t ValueSize = DL.getTypeStoreSizeInBits(ValTy);
+  if (auto FragmentSize = DII->getFragmentSizeInBits())
+    return ValueSize >= *FragmentSize;
+  return false;
+}
+
 /// Inserts a llvm.dbg.value intrinsic before a store to an alloca'd value
 /// that has an associated llvm.dbg.declare or llvm.dbg.addr intrinsic.
 void llvm::ConvertDebugDeclareToDebugValue(DbgInfoIntrinsic *DII,
                                            StoreInst *SI, DIBuilder &Builder) {
   assert(DII->isAddressOfVariable());
   auto *DIVar = DII->getVariable();
   assert(DIVar && "Missing variable");
   auto *DIExpr = DII->getExpression();
   Value *DV = SI->getOperand(0);
 
+  if (!valueCoversEntireFragment(SI->getValueOperand()->getType(), DII)) {
+    // FIXME: If storing to a part of the variable described by the dbg.declare,
+    // then we want to insert a dbg.value for the corresponding fragment.
+    LLVM_DEBUG(dbgs() << "Failed to convert dbg.declare to dbg.value: "
+                      << *DII << '\n');
+    // For now, when there is a store to parts of the variable (but we do not
+    // know which part) we insert an dbg.value instrinsic to indicate that we
+    // know nothing about the variable's content.
+    DV = UndefValue::get(DV->getType());
+    if (!LdStHasDebugValue(DIVar, DIExpr, SI))
+      Builder.insertDbgValueIntrinsic(DV, DIVar, DIExpr, DII->getDebugLoc(),
+                                      SI);
+    return;
+  }
+
   // If an argument is zero extended then use argument directly. The ZExt
   // may be zapped by an optimization pass in future.
   Argument *ExtendedArg = nullptr;
   if (ZExtInst *ZExt = dyn_cast<ZExtInst>(SI->getOperand(0)))
     ExtendedArg = dyn_cast<Argument>(ZExt->getOperand(0));
   if (SExtInst *SExt = dyn_cast<SExtInst>(SI->getOperand(0)))
     ExtendedArg = dyn_cast<Argument>(SExt->getOperand(0));
   if (ExtendedArg) {
void llvm::ConvertDebugDeclareToDebugValue(DbgInfoIntrinsic *DII,
                                            LoadInst *LI, DIBuilder &Builder) {
   auto *DIVar = DII->getVariable();
   auto *DIExpr = DII->getExpression();
   assert(DIVar && "Missing variable");
 
   if (LdStHasDebugValue(DIVar, DIExpr, LI))
     return;
 
+  assert(valueCoversEntireFragment(LI->getType(), DII) &&
+         "Load is not loading the full variable fragment.");
+
   // We are now tracking the loaded value instead of the address. In the
   // future if multi-location support is added to the IR, it might be
   // preferable to keep tracking both the loaded value and the original
   // address in case the alloca can not be elided.
   Instruction *DbgValue = Builder.insertDbgValueIntrinsic(
       LI, DIVar, DIExpr, DII->getDebugLoc(), (Instruction *)nullptr);
   DbgValue->insertAfter(LI);
 }
 
 /// Inserts a llvm.dbg.value intrinsic after a phi that has an associated
 /// llvm.dbg.declare or llvm.dbg.addr intrinsic.
 void llvm::ConvertDebugDeclareToDebugValue(DbgInfoIntrinsic *DII,
                                            PHINode *APN, DIBuilder &Builder) {
   auto *DIVar = DII->getVariable();
   auto *DIExpr = DII->getExpression();
   assert(DIVar && "Missing variable");
 
   if (PhiHasDebugValue(DIVar, DIExpr, APN))
     return;
 
+  assert(valueCoversEntireFragment(APN->getType(), DII) &&
+         "PHI node is not describing the full variable.");
+
   BasicBlock *BB = APN->getParent();
   auto InsertionPt = BB->getFirstInsertionPt();
 
   // The block may be a catchswitch block, which does not have a valid
   // insertion point.
   // FIXME: Insert dbg.value markers in the successors when appropriate.
   if (InsertionPt != BB->end())
     Builder.insertDbgValueIntrinsic(APN, DIVar, DIExpr, DII->getDebugLoc(),

llvm/trunk/test/Transforms/InstCombine/debuginfo.ll

entry:
   call void @tworegs_callee(i64 %5, i64 %7), !dbg !33
   ret void, !dbg !33
 }
 
 ; NOLOWER-LABEL: define void @tworegs(i64 %o.coerce0, i64 %o.coerce1)
 ; NOLOWER-NOT: alloca
 ; NOLOWER-NOT: store
 ; NOLOWER-NOT: call void @llvm.dbg.declare
-; NOLOWER: call void @llvm.dbg.value(metadata i64 %o.coerce0, {{.*}})
+; Here we want to find:  call void @llvm.dbg.value(metadata i64 %o.coerce0, metadata [[VARIABLE_O]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 64))
+; NOLOWER: call void @llvm.dbg.value(metadata i64 undef, {{.*}})
 ; NOLOWER-NOT: store
-; NOLOWER: call void @llvm.dbg.value(metadata i64 %o.coerce1, {{.*}})
+; Here we want to find:  call void @llvm.dbg.value(metadata i64 %o.coerce1, metadata [[VARIABLE_O]], metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64))
+; NOLOWER: call void @llvm.dbg.value(metadata i64 undef, {{.*}})
 ; NOLOWER-NOT: store
 ; NOLOWER: call void @tworegs_callee(i64 %o.coerce0, i64 %o.coerce1)
 
 
 !llvm.dbg.cu = !{!3}
 !llvm.module.flags = !{!30}
 
 !0 = !DILocalVariable(name: "a", line: 78, arg: 1, scope: !1, file: !2, type: !6)

llvm/trunk/test/Transforms/Mem2Reg/debug-alloca-phi-2.ll

+; RUN: opt < %s -mem2reg -S | FileCheck %s
+source_filename = "bugpoint-output.bc"
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.12.0"
+
+define void @scan() #0 !dbg !12 {
+entry:
+  %entry1 = alloca i1, align 8
+  call void @llvm.dbg.declare(metadata i1* %entry1, metadata !18, metadata !19), !dbg !20
+  store i1 0, i1* %entry1, align 8, !dbg !20
+  br label %for.cond, !dbg !20
+
+for.cond:
+; CHECK: %[[PHI:.*]] = phi i1 [ false, %entry ], [ %0, %for.cond ]
+  %entryN = load i1, i1* %entry1, align 8, !dbg !20
+; CHECK: call void @llvm.dbg.value(metadata i1 %[[PHI]],
+; CHECK-SAME:                      metadata !DIExpression())
+  %0 = add i1 %entryN, 1
+; CHECK: %0 = add i1 %[[PHI]], true
+; CHECK: call void @llvm.dbg.value(metadata i1 %0,
+; CHECK-SAME:                      metadata !DIExpression())
+  store i1 %0, i1* %entry1, align 8, !dbg !20
+  br label %for.cond, !dbg !20
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { nounwind ssp uwtable }
+attributes #1 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!10, !11}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "adrian", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "<stdin>", directory: "/")
+!2 = !{}
+!4 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!10 = !{i32 2, !"Debug Info Version", i32 3}
+!11 = !{i32 1, !"PIC Level", i32 2}
+!12 = distinct !DISubprogram(name: "scan", scope: !1, file: !1, line: 4, type: !13, isLocal: false, isDefinition: true, scopeLine: 5, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !15)
+!13 = !DISubroutineType(types: !14)
+!14 = !{null, !4, !4}
+!15 = !{!18}
+!18 = !DILocalVariable(name: "entry", scope: !12, file: !1, line: 6, type: !4)
+!19 = !DIExpression()
+!20 = !DILocation(line: 6, scope: !12)