[Utils] Fix incorrect dbg.declare store conversion

[loladiro]

Summary

The dbg.declare -> dbg.value conversion did not check which operand of the store instruction the alloca was passed to. As a result code that stored the address of an alloca, rather than storing to the alloca, would still trigger the conversion routine, leading to the insertion of an incorrect dbg.value intrinsic.

[aprantl]

Good catch! Looks pretty obvious to me. Some minor comments inline.

lib/Transforms/Utils/Local.cpp

1133 ↗

(On Diff #44820)

Please add more/full context to diffs uploaded to phabricator; it makes reviewing easier.

1138 ↗

(On Diff #44820)

Does turning this if cascade into a switch() make it any more readable?

test/Transforms/Util/store-first-op.ll

11 ↗

(On Diff #44820)

pass pass

13 ↗

(On Diff #44820)

I would still add a positive check — this way we'll be notified when the testcase needs to be updated.

[loladiro]

lib/Transforms/Utils/Local.cpp

1138 ↗

(On Diff #44820)

I think that would introduce a lot of extra casting, which would probably make it less readable (assuming you were thinking of a cast on ->getOperand()), if there's a better syntax, do let me know.

[loladiro]

lib/Transforms/Utils/Local.cpp

1138 ↗

(On Diff #44820)

Sorry ->getOpcode of course.

[loladiro]

test/Transforms/Util/store-first-op.ll

13 ↗

(On Diff #44820)

What kind of positive test are you imagining. In the corrected version, no dbg.* intrinsic is inserted.

[aprantl]

lib/Transforms/Utils/Local.cpp

1138 ↗

(On Diff #44820)

Yes, that's what I meant. If it doesn't improve the readability (speed doesn't matter with three cases) let's leave it as is.

test/Transforms/Util/store-first-op.ll

13 ↗

(On Diff #44820)

My thought was that if no dbg.value is inserted the dbg.declare should still be there but that isn't actually the case. In this testcase the alloca is dead because there is no store to it or load, so not having any intrinsics makes sense.

Hm. This brings up an interesting point: Shouldn't LowerDbgDeclare() replace the alloca in the dbg.declare instruction with an %undef or alternatively leave the dbg.declare in there if the dbg.declare was not lowered? Otherwise we're going to loose function arguments and local variables.

[loladiro]

test/Transforms/Util/store-first-op.ll

13 ↗

(On Diff #44820)

Yes, I think that would be reasonable (but not for this patch - will go ahead and commit).

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

for (auto &I : Dbgs) {
     AllocaInst *AI = dyn_cast_or_null<AllocaInst>(DDI->getAddress());
     // If this is an alloca for a scalar variable, insert a dbg.value
     // at each load and store to the alloca and erase the dbg.declare.
     // The dbg.values allow tracking a variable even if it is not
     // stored on the stack, while the dbg.declare can only describe
     // the stack slot (and at a lexical-scope granularity). Later
     // passes will attempt to elide the stack slot.
     if (AI && !isArray(AI)) {
-      for (User *U : AI->users())
-        if (StoreInst *SI = dyn_cast<StoreInst>(U))
+      for (auto &AIUse : AI->uses()) {
+        User *U = AIUse.getUser();
+        if (StoreInst *SI = dyn_cast<StoreInst>(U)) {
+          if (AIUse.getOperandNo() == 1)
             ConvertDebugDeclareToDebugValue(DDI, SI, DIB);
-        else if (LoadInst *LI = dyn_cast<LoadInst>(U))
+        } else if (LoadInst *LI = dyn_cast<LoadInst>(U)) {
           ConvertDebugDeclareToDebugValue(DDI, LI, DIB);
-        else if (CallInst *CI = dyn_cast<CallInst>(U)) {
+        } else if (CallInst *CI = dyn_cast<CallInst>(U)) {
           // This is a call by-value or some other instruction that
           // takes a pointer to the variable. Insert a *value*
           // intrinsic that describes the alloca.
           SmallVector<uint64_t, 1> NewDIExpr;
           auto *DIExpr = DDI->getExpression();
           NewDIExpr.push_back(dwarf::DW_OP_deref);
           NewDIExpr.append(DIExpr->elements_begin(), DIExpr->elements_end());
           DIB.insertDbgValueIntrinsic(AI, 0, DDI->getVariable(),
                                       DIB.createExpression(NewDIExpr),
                                       DDI->getDebugLoc(), CI);
         }
+      }
       DDI->eraseFromParent();
     }
   }
   return true;
 }
 
 /// FindAllocaDbgDeclare - Finds the llvm.dbg.declare intrinsic describing the
 /// alloca 'V', if any.

llvm/trunk/test/Transforms/Util/store-first-op.ll

+; RUN: opt -instcombine -S %s | FileCheck %s
+
+%foo = type { i8 }
+
+; Function Attrs: nounwind uwtable
+define void @_ZN4llvm13ScaledNumbers10multiply64Emm() {
+entry:
+  %getU = alloca %foo, align 1
+; This is supposed to make sure that the declare conversion, does not accidentally think the store OF
+; %getU is a store TO %getU. There are valid reasons to have an llvm.dbg.value here, but if the pass
+; is changed to emit such, a more specific check should be added to make sure that any llvm.dbg.value
+; is correct.
+; CHECK-NOT: @llvm.dbg.value(metadata %foo* %getU
+  call void @llvm.dbg.declare(metadata %foo* %getU, metadata !3, metadata !6), !dbg !7
+  store %foo* %getU, %foo** undef, align 8, !tbaa !8
+  unreachable
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #1 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.8.0 (https://github.com/llvm-mirror/clang 89dda3855cda574f355e6defa1d77bdae5053994) (llvm/trunk 257597)", isOptimized: true, runtimeVersion: 0, emissionKind: 1)
+!1 = !DIFile(filename: "none", directory: ".")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocalVariable(name: "getU", scope: !4, file: !1, line: 25, type: !5)
+!4 = distinct !DISubprogram(name: "multiply64", linkageName: "_ZN4llvm13ScaledNumbers10multiply64Emm", scope: null, file: !1, line: 22, isLocal: false, isDefinition: true, scopeLine: 23, flags: DIFlagPrototyped, isOptimized: true)
+!5 = !DICompositeType(tag: DW_TAG_class_type, scope: !4, file: !1, line: 25, size: 8, align: 8)
+!6 = !DIExpression()
+!7 = !DILocation(line: 25, column: 8, scope: !4)
+!8 = !{!9, !9, i64 0}
+!9 = !{i64 0}