[PATCH] D48024: [DebugInfo] Check size of variable in ConvertDebugDeclareToDebugValue

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope created this revision.
bjope added reviewers: dblaikie, aprantl, rnk.
bjope added a project: debug-info.
Herald added a subscriber: JDevlieghere.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D48024

Files:
  lib/Transforms/Utils/Local.cpp
  test/Transforms/InstCombine/debuginfo.ll


Index: test/Transforms/InstCombine/debuginfo.ll
===================================================================
--- test/Transforms/InstCombine/debuginfo.ll
+++ test/Transforms/InstCombine/debuginfo.ll
@@ -71,9 +71,11 @@
 ; 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)
 
Index: lib/Transforms/Utils/Local.cpp
===================================================================
--- lib/Transforms/Utils/Local.cpp
+++ lib/Transforms/Utils/Local.cpp
@@ -1228,6 +1228,25 @@
   return false;
 }
 
+/// Get the number of bits that are described by a dbg.declare.
+static uint64_t DescribedVariablesSizeInBits(DbgInfoIntrinsic *DII) {
+  if (auto Fragment = DII->getExpression()->getFragmentInfo())
+    return Fragment->SizeInBits;
+  if (auto VarSize = DII->getVariable()->getSizeInBits())
+    return *VarSize;
+  llvm_unreachable("Unkonwn size of the variable described by DII");
+}
+
+/// Check if the size of the variable (or fragment of the variable) described by
+/// \p DII isn't greater than the size of \p Ty.
+static bool IsVariableCoveredByType(DbgInfoIntrinsic *DII, Type *Ty) {
+  const DataLayout &DL = DII->getModule()->getDataLayout();
+  uint64_t TypeSize = DL.getTypeSizeInBits(Ty);
+  uint64_t DescribedSize = DescribedVariablesSizeInBits(DII);
+  return TypeSize >= DescribedSize;
+}
+
+
 /// 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,
@@ -1238,6 +1257,21 @@
   auto *DIExpr = DII->getExpression();
   Value *DV = SI->getOperand(0);
 
+  if (!IsVariableCoveredByType(DII, SI->getValueOperand()->getType())) {
+    // 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 variables 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;
@@ -1281,6 +1315,9 @@
   if (LdStHasDebugValue(DIVar, DIExpr, LI))
     return;
 
+  assert(IsVariableCoveredByType(DII, LI->getType()) &&
+         "Assuming that the load is loading the full variable.");
+
   // 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
@@ -1301,6 +1338,9 @@
   if (PhiHasDebugValue(DIVar, DIExpr, APN))
     return;
 
+  assert(IsVariableCoveredByType(DII, APN->getType()) &&
+         "Assuming that the load is loading the full variable.");
+
   BasicBlock *BB = APN->getParent();
   auto InsertionPt = BB->getFirstInsertionPt();
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D48024.150744.patch
Type: text/x-patch
Size: 4035 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180611/3e12d6a5/attachment.bin>