[Debuginfo][Instcombiner] Do not clone dbg.declare in TryToSinkInstruction()

[avl]

Summary

TryToSinkInstruction() has a bug: While updating debug info for sunk instruction, it could clone dbg.declare intrinsic. That is wrong. There could be only one dbg.declare. The fix is to not clone dbg.declare intrinsic and to update it`s arguments, to not to point to sunk instruction.

before instcombine :

entry:

%result = alloca i64, align 8
%tmpcast = bitcast i64* %result to %struct.S1*
%0 = bitcast i64* %result to i8*, !dbg !24
call void @llvm.dbg.declare(metadata %struct.S1* %tmpcast, metadata !12, metadata !DIExpression()), !dbg !24

%p1 = bitcast i64* %result to i32*, !dbg !35
store i32 2, i32* %p1, align 8, !dbg !35

after instcombine:

%result = alloca i64, align 8
%0 = bitcast i64* %result to i8*, !dbg !24
call void @llvm.dbg.declare(metadata i64* %result, metadata !12, metadata !DIExpression()), !dbg !24

%tmpcast = bitcast i64* %result to %struct.S1*
call void @llvm.dbg.declare(metadata %struct.S1* %tmpcast, metadata !12, metadata !DIExpression()), !dbg !24
^^^^^^^^^^^^^^^^^^^^^^^^^^^
cloned llvm.dbg.declare

[jmorse]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3169

"should" rather than "could"? Also, IMO "be left at original" should be "be left in the original"

3173–3175

As I understand it, this works right now because the only sinkable instructions that dbg.declares point at are casts of other things (like arguments). This is probably fine -- but if dbg.declares operand rules ever change or someone writes IR that doesn't match that expectation, getOperand/setOperand could fail and it won't be clear why.

IMO it'd be good to add something like

if (!isa<Cast>(I))
  continue; // dbg.declare points at something it shouldn't

so that the failure mode is a debug use-before-def (which I don't believe is an error) rather than a crash.

[avl]

addressed comments.

[aprantl]

This is a strict improvement and as such is fine, but there's some general cleanups that are uncovered by this that we should also be doing.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3169

How about:

A dbg.declare instruction should not be cloned, since there can only be one per variable fragment.

3170

space before (

3173–3175

We could probably simplify the model by enforcing that dbg.declares always point at allocas or function arguments directly, That would make code like this one look less murky.

3174

This should really be a Verifier error.

[aprantl]

llvm/test/Transforms/InstCombine/do-not-clone-dbg-declare.ll

150

You could probably use this as a !dbg atachement in most instructions to simplify the testcase.

[avl]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3173–3175

@aprantl how should this be done in a procedural manner? I mean - Could I create a patch for verifier, which would check "that dbg.declares always point at allocas or function arguments directly" and then fix all places ? Or that question should be previously discussed somehow?

[aprantl]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3173–3175

It may be best to proceed even slower than that. Changing what we accept inside of dbg.declare potentially means that all sorts of producers of LLVM need to be updated, so we need to be mindful of that. I think it would be reasonable to only let dbg.declare point inside an alloca or inside a (stack-allocated) function argument/sret value. But it might be too strict to demand that there is a 1-1 correspondence between allocas and dbg.declares, I think it might be reasonable to also allow bitcast and getelementptr operations to support clients like ASAN — as long as it points into a stack slot.

So I'd start by writing a patch that

• updates LangRef.rst / SourceLevelDebugging.rst
• verifies that the argument of a dbg.declare fullfills these rules
• see if we have any failures (running the llvm testsuite, compiling clang at -O0, -O3, -O1+asan):
• - if they are obvious mistakes, fix them
• - otherwise come back here to discuss what we found
• from there we can gradually tighten the requirements.

[avl]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3173–3175

Ok, I would follow that plan. Thank you.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {
   // get debug use before the def. Attempt to salvage debug uses first, to
   // maximise the range variables have location for. If we cannot salvage, then
   // mark the location undef: we know it was supposed to receive a new location
   // here, but that computation has been sunk.
   SmallVector<DbgVariableIntrinsic *, 2> DbgUsers;
   findDbgUsers(DbgUsers, I);
   for (auto *DII : reverse(DbgUsers)) {
     if (DII->getParent() == SrcBlock) {
+
+      if (isa<DbgDeclareInst>(DII)) {
+        // dbg.declare instruction could not be cloned. It should be left at
+        // original place since sunk instruction is not an alloca(otherwise we
+        // could not be here). But we need to update arguments of dbg.declare
+        // instruction, so that it would not point into sunk instruction.
+        DII->setOperand(
+            0, MetadataAsValue::get(I->getContext(),
+                                    ValueAsMetadata::get(I->getOperand(0))));
+        continue;
+      }
+
       // dbg.value is in the same basic block as the sunk inst, see if we can
       // salvage it. Clone a new copy of the instruction: on success we need
       // both salvaged and unsalvaged copies.
       SmallVector<DbgVariableIntrinsic *, 1> TmpUser{
           cast<DbgVariableIntrinsic>(DII->clone())};
 
       if (!salvageDebugInfoForDbgValues(*I, TmpUser)) {
         // We are unable to salvage: sink the cloned dbg.value, and mark the

llvm/test/Transforms/InstCombine/do-not-clone-dbg-declare.ll

+
+; RUN: opt %s -instcombine -verify -S -o - | FileCheck %s
+
+; Hand-reduced from this example.
+; -g -O -mllvm -disable-llvm-optzns -gno-column-info
+; plus opt -sroa -instcombine -inline
+
+; #include <stdio.h>
+;
+; struct S1 {
+;     int p1;
+;     int p2;
+;
+;     bool IsNull (  ) {
+;         return p1 == 0;
+;     }
+; };
+;
+; S1 foo ( void );
+;
+; int bar (  ) {
+;
+;     S1 result = foo();
+;
+;     if ( result.IsNull() )
+;         return 0;
+;
+;     result.p1 = 2;
+;     result.p2 = 3;
+;
+;     int* ptr = &result.p1;
+;
+;     printf("%d", *ptr);
+;     printf("%d", *(ptr+1));
+;
+;     return result.p1 + 1;
+; }
+
+; CHECK: _Z3barv
+; CHECK: llvm.dbg.declare(metadata i64* %{{.*}}, metadata [[METADATA_IDX1:![0-9]+]]
+; CHECK-NOT: llvm.dbg.declare(metadata %struct.S1* %{{.*}}, metadata [[METADATA_IDX1]]
+; CHECK: ret
+; CHECK: DICompileUnit
+; CHECK: [[METADATA_IDX1]] = !DILocalVariable(name: "result"
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%struct.S1 = type { i32, i32 }
+
+@.str = private unnamed_addr constant [3 x i8] c"%d\00", align 1
+
+define dso_local i32 @_Z3barv() !dbg !7 {
+entry:
+  %result = alloca i64, align 8
+  %tmpcast = bitcast i64* %result to %struct.S1*
+  %0 = bitcast i64* %result to i8*, !dbg !24
+  call void @llvm.lifetime.start.p0i8(i64 8, i8* nonnull %0) #4, !dbg !24
+  call void @llvm.dbg.declare(metadata %struct.S1* %tmpcast, metadata !12, metadata !DIExpression()), !dbg !24
+  %call = call i64 @_Z3foov(), !dbg !24
+  store i64 %call, i64* %result, align 8, !dbg !24
+  call void @llvm.dbg.value(metadata %struct.S1* %tmpcast, metadata !25, metadata !DIExpression()), !dbg !29
+  %p1.i = getelementptr inbounds %struct.S1, %struct.S1* %tmpcast, i64 0, i32 0, !dbg !32
+  %1 = load i32, i32* %p1.i, align 4, !dbg !32
+  %cmp.i = icmp eq i32 %1, 0, !dbg !32
+  br i1 %cmp.i, label %if.then, label %if.end, !dbg !38
+
+if.then:                                          ; preds = %entry
+  br label %cleanup, !dbg !39
+
+if.end:                                           ; preds = %entry
+
+  %p1 = bitcast i64* %result to i32*, !dbg !40
+  store i32 2, i32* %p1, align 8, !dbg !40
+  %p2 = getelementptr inbounds %struct.S1, %struct.S1* %tmpcast, i64 0, i32 1, !dbg !41
+  store i32 3, i32* %p2, align 4, !dbg !41
+  %p12 = bitcast i64* %result to i32*, !dbg !43
+  call void @llvm.dbg.value(metadata i32* %p12, metadata !22, metadata !DIExpression()), !dbg !44
+  %call3 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i64 0, i64 0), i32 2), !dbg !45
+  %add.ptr = getelementptr inbounds i32, i32* %p12, i64 1, !dbg !46
+  %2 = load i32, i32* %add.ptr, align 4, !dbg !46
+  %call4 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i64 0, i64 0), i32 %2), !dbg !46
+  %p15 = bitcast i64* %result to i32*, !dbg !48
+  %3 = load i32, i32* %p15, align 8, !dbg !48
+  %add = add nsw i32 %3, 1, !dbg !48
+  br label %cleanup
+
+cleanup:                                          ; preds = %if.end, %if.then
+  %retval.0 = phi i32 [ 0, %if.then ], [ %add, %if.end ], !dbg !44
+  %4 = bitcast i64* %result to i8*, !dbg !49
+  call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %4) #4, !dbg !49
+  ret i32 %retval.0, !dbg !49
+}
+
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture) #1
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #2
+
+declare dso_local i64 @_Z3foov() #3
+
+declare dso_local i32 @printf(i8*, ...) #3
+
+declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture) #1
+
+declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang"}
+!7 = distinct !DISubprogram(name: "bar", linkageName: "_Z3barv", scope: !1, file: !1, line: 15, type: !8, scopeLine: 15, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!12, !22}
+!12 = !DILocalVariable(name: "result", scope: !7, file: !1, line: 17, type: !13)
+!13 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "S1", file: !1, line: 4, size: 64, flags: DIFlagTypePassByValue, elements: !14, identifier: "_ZTS2S1")
+!14 = !{!15, !16, !17}
+!15 = !DIDerivedType(tag: DW_TAG_member, name: "p1", scope: !13, file: !1, line: 5, baseType: !10, size: 32)
+!16 = !DIDerivedType(tag: DW_TAG_member, name: "p2", scope: !13, file: !1, line: 6, baseType: !10, size: 32, offset: 32)
+!17 = !DISubprogram(name: "IsNull", linkageName: "_ZN2S16IsNullEv", scope: !13, file: !1, line: 8, type: !18, scopeLine: 8, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+!18 = !DISubroutineType(types: !19)
+!19 = !{!20, !21}
+!20 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!21 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64, flags: DIFlagArtificial | DIFlagObjectPointer)
+!22 = !DILocalVariable(name: "ptr", scope: !7, file: !1, line: 25, type: !23)
+!23 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !10, size: 64)
+!24 = !DILocation(line: 17, scope: !7)
+!25 = !DILocalVariable(name: "this", arg: 1, scope: !26, type: !28, flags: DIFlagArtificial | DIFlagObjectPointer)
+!26 = distinct !DISubprogram(name: "IsNull", linkageName: "_ZN2S16IsNullEv", scope: !13, file: !1, line: 8, type: !18, scopeLine: 8, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, declaration: !17, retainedNodes: !27)
+!27 = !{!25}
+!28 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64)
+!29 = !DILocation(line: 0, scope: !26, inlinedAt: !30)
+!30 = distinct !DILocation(line: 19, scope: !31)
+!31 = distinct !DILexicalBlock(scope: !7, file: !1, line: 19)
+!32 = !DILocation(line: 9, scope: !26, inlinedAt: !30)
+!38 = !DILocation(line: 19, scope: !7)
+!39 = !DILocation(line: 20, scope: !31)
+!40 = !DILocation(line: 22, scope: !7)
+!41 = !DILocation(line: 23, scope: !7)
+!43 = !DILocation(line: 25, scope: !7)
+!44 = !DILocation(line: 0, scope: !7)
+!45 = !DILocation(line: 27, scope: !7)
+!46 = !DILocation(line: 28, scope: !7)
+!48 = !DILocation(line: 30, scope: !7)
+!49 = !DILocation(line: 31, scope: !7)