[Assignment Tracking] Trim assignments for untagged out of bounds stores #66095
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes llvm.org/PR65004
|
@llvm/pr-subscribers-debuginfo ChangesFixes #65004 by trimming assignments from out of bounds stores (out of bounds -- 3 Files Affected:
diff --git a/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp b/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
index 5ef850d09d925f0..cf24bce4b605b53 100644
--- a/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
+++ b/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
@@ -1979,20 +1979,22 @@ static AssignmentTrackingLowering::OverlapMap buildOverlapMapAndRecordDeclares(
I, Fn.getParent()->getDataLayout())) {
// Find markers linked to this alloca.
for (DbgAssignIntrinsic *DAI : at::getAssignmentMarkers(Info->Base)) {
- // Discard the fragment if it covers the entire variable.
- std::optional FragInfo =
- [&Info, DAI]() -> std::optional {
- DIExpression::FragmentInfo F;
- F.OffsetInBits = Info->OffsetInBits;
- F.SizeInBits = Info->SizeInBits;
- if (auto ExistingFrag = DAI->getExpression()->getFragmentInfo())
- F.OffsetInBits += ExistingFrag->OffsetInBits;
- if (auto Sz = DAI->getVariable()->getSizeInBits()) {
- if (F.OffsetInBits == 0 && F.SizeInBits == *Sz)
- return std::nullopt;
- }
- return F;
- }();
+ std::optional FragInfo;
+
+ // Skip this assignment if the affected bits are outside of the
+ // variable fragment.
+ if (!at::calculateFragmentIntersect(
+ I.getModule()->getDataLayout(), Info->Base,
+ Info->OffsetInBits, Info->SizeInBits, DAI, FragInfo) ||
+ (FragInfo && FragInfo->SizeInBits == 0))
+ continue;
+
+ // FragInfo from calculateFragmentIntersect is is nullopt if the
+ // resultant fragment matches DAI's fragment or entire variable - in
+ // that case just copy the fragment from DAI. Now nullopt means "no
+ // fragment info", as per usual.
+ if (!FragInfo)
+ FragInfo = DAI->getExpression()->getFragmentInfo();
DebugVariable DV = DebugVariable(DAI->getVariable(), FragInfo,
DAI->getDebugLoc().getInlinedAt());
diff --git a/llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-assignment-extra-checks.ll b/llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-assignment-extra-checks.ll
new file mode 100644
index 000000000000000..eb820a27d17cb89
--- /dev/null
+++ b/llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-assignment-extra-checks.ll
@@ -0,0 +1,94 @@
+; RUN: llc %s -stop-after=finalize-isel -o - \
+; RUN: | FileCheck %s
+;--implicit-check-not=DBG_
+
+;; Similarly to untagged-store-assignment-outside-variable.ll this test checks
+;; that out of bounds stores that have no DIAssignID are interpreted correctly
+;; (see inline comments and checks). Hand-written IR.
+
+target triple = "x86_64-unknown-linux-gnu"
+
+declare dso_local void @a(i32)
+
+define dso_local void @b() local_unnamed_addr !dbg !14 {
+entry:
+ %c = alloca [4 x i16], align 8, !DIAssignID !24
+ %arrayidx = getelementptr inbounds [4 x i16], ptr %c, i64 0, i64 2
+ call void @llvm.dbg.assign(metadata i1 undef, metadata !18, metadata !DIExpression(DW_OP_LLVM_fragment, 128, 32), metadata !24, metadata ptr %arrayidx, metadata !DIExpression()), !dbg !26
+
+;; Set variable value (use a call to prevent eliminating redundant DBG_VALUEs).
+; CHECK: DBG_VALUE 0, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_fragment, 128, 32)
+ call void @llvm.dbg.assign(metadata i64 0, metadata !18, metadata !DIExpression(DW_OP_LLVM_fragment, 128, 32), metadata !29, metadata ptr %c, metadata !DIExpression()), !dbg !26
+
+;; Trim assignment that leaks outside alloca (upper 32 bits don't fit inside %c alloca).
+; CHECK: DBG_VALUE %stack.0.c, $noreg, ![[#]], !DIExpression(DW_OP_plus_uconst, 4, DW_OP_deref, DW_OP_LLVM_fragment, 128, 32)
+ store i64 1, ptr %arrayidx, align 4
+;; Set variable value (use a call to prevent eliminating redundant DBG_VALUEs).
+; CHECK: DBG_VALUE 10, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_fragment, 128, 32)
+ call void @a(i32 1)
+ call void @llvm.dbg.assign(metadata i64 10, metadata !18, metadata !DIExpression(DW_OP_LLVM_fragment, 128, 32), metadata !29, metadata ptr %c, metadata !DIExpression()), !dbg !26
+
+;; Trim assignment that doesn't align with fragment start and leaks outside
+;; alloca (16 bit offset from fragment start, upper 48 bits don't fit inside %c
+;; alloca).
+; CHECK: DBG_VALUE %stack.0.c, $noreg, ![[#]], !DIExpression(DW_OP_plus_uconst, 6, DW_OP_deref, DW_OP_LLVM_fragment, 144, 16)
+ %arrayidx1 = getelementptr inbounds [4 x i16], ptr %c, i64 0, i64 3
+ store i64 2, ptr %arrayidx1, align 4
+;; Set variable value (use a call to prevent eliminating redundant DBG_VALUEs).
+; CHECK: DBG_VALUE 20, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_fragment, 128, 32)
+ call void @a(i32 2)
+ call void @llvm.dbg.assign(metadata i64 20, metadata !18, metadata !DIExpression(DW_OP_LLVM_fragment, 128, 32), metadata !29, metadata ptr %c, metadata !DIExpression()), !dbg !26
+
+;; Negative accesses are skipped.
+ %arrayidx2 = getelementptr inbounds [4 x i16], ptr %c, i64 0, i64 -1
+ store i128 3, ptr %arrayidx2, align 4
+;; Set variable value (use a call to prevent eliminating redundant DBG_VALUEs).
+; CHECK: DBG_VALUE 30, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_fragment, 128, 32)
+ call void @a(i32 3)
+ call void @llvm.dbg.assign(metadata i64 30, metadata !18, metadata !DIExpression(DW_OP_LLVM_fragment, 128, 32), metadata !29, metadata ptr %c, metadata !DIExpression()), !dbg !26
+
+;; Skip assignment outside base variable fragment.
+ store i32 4, ptr %c, align 4
+;; Set variable value (use a call to prevent eliminating redundant DBG_VALUEs).
+; CHECK: DBG_VALUE 40, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_fragment, 128, 32)
+ call void @a(i32 4)
+ call void @llvm.dbg.assign(metadata i64 40, metadata !18, metadata !DIExpression(DW_OP_LLVM_fragment, 128, 32), metadata !29, metadata ptr %c, metadata !DIExpression()), !dbg !26
+
+;; Trim partial overlap (lower 32 bits of store don't intersect base fragment
+;; and upper 64 bits don't actually fit inside the alloca).
+ store i128 5, ptr %c, align 4
+; CHECK: DBG_VALUE %stack.0.c, $noreg, ![[#]], !DIExpression(DW_OP_deref, DW_OP_LLVM_fragment, 128, 32)
+;; Set variable value (use a call to prevent eliminating redundant DBG_VALUEs).
+; CHECK: DBG_VALUE 50, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_fragment, 128, 32)
+ call void @a(i32 5)
+ call void @llvm.dbg.assign(metadata i64 50, metadata !18, metadata !DIExpression(DW_OP_LLVM_fragment, 128, 32), metadata !29, metadata ptr %c, metadata !DIExpression()), !dbg !26
+
+ ret void
+}
+
+declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata) #2
+declare void @llvm.dbg.value(metadata, metadata, metadata) #3
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!6, !7, !12}
+!llvm.ident = !{!13}
+
+!2 = distinct !DICompileUnit(language: DW_LANG_C11, file: !3, producer: "clang version 17.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !4, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "test.c", directory: "/")
+!4 = !{}
+!5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!6 = !{i32 7, !"Dwarf Version", i32 5}
+!7 = !{i32 2, !"Debug Info Version", i32 3}
+!12 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
+!13 = !{!"clang version 17.0.0"}
+!14 = distinct !DISubprogram(name: "b", scope: !3, file: !3, line: 2, type: !15, scopeLine: 2, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !17)
+!15 = !DISubroutineType(types: !16)
+!16 = !{null}
+!17 = !{!18}
+!18 = !DILocalVariable(name: "c", scope: !14, file: !3, line: 3, type: !19)
+!19 = !DICompositeType(tag: DW_TAG_array_type, baseType: !5, size: 160, elements: !20)
+!20 = !{!21}
+!21 = !DISubrange(count: 2)
+!24 = distinct !DIAssignID()
+!26 = !DILocation(line: 0, scope: !14)
+!29 = distinct !DIAssignID()
diff --git a/llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-assignment-outside-variable.ll b/llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-assignment-outside-variable.ll
new file mode 100644
index 000000000000000..3a7c528d320a738
--- /dev/null
+++ b/llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-assignment-outside-variable.ll
@@ -0,0 +1,97 @@
+; RUN: llc %s -stop-after=finalize-isel -o - \
+; RUN: | FileCheck %s --implicit-check-not=DBG_
+
+;; Generated from following C source that contains UB (read and write to
+;; out of bounds static array element.
+;; int a;
+;; void b() {
+;; int c[2] = {0, 0};
+;; __attribute__((nodebug)) unsigned d = -1;
+;; if (a)
+;; c[a] = c[d] &= a;
+;; b();
+;; }
+;;
+;; $ clang -O1 -g test.c -emit-llvm -S -o -
+;;
+;; Check the assignment c[d] isn't tracked (--implicit-check-not and
+;; no assertion triggered, see llvm.org/PR65004).
+
+; CHECK: bb.1.tailrecurse:
+; CHECK: DBG_VALUE $noreg, $noreg, !18, !DIExpression()
+; CHECK: DBG_VALUE %stack.0.c, $noreg, !18, !DIExpression(DW_OP_deref)
+; CHECK: bb.2.if.then:
+
+target triple = "x86_64-unknown-linux-gnu"
+
+@a = dso_local local_unnamed_addr global i32 0, align 4, !dbg !0
+
+define dso_local void @b() local_unnamed_addr !dbg !14 {
+entry:
+ %c = alloca [2 x i32], align 8, !DIAssignID !22
+ br label %tailrecurse, !dbg !23
+
+tailrecurse: ; preds = %if.end, %entry
+ call void @llvm.dbg.assign(metadata i1 undef, metadata !18, metadata !DIExpression(), metadata !22, metadata ptr %c, metadata !DIExpression()), !dbg !24
+ store i64 0, ptr %c, align 8, !dbg !26, !DIAssignID !27
+ call void @llvm.dbg.assign(metadata i64 0, metadata !18, metadata !DIExpression(), metadata !27, metadata ptr %c, metadata !DIExpression()), !dbg !24
+ %0 = load i32, ptr @a, align 4, !dbg !28
+ %tobool.not = icmp eq i32 %0, 0, !dbg !28
+ br i1 %tobool.not, label %if.end, label %if.then, !dbg !34
+
+if.then: ; preds = %tailrecurse
+ %arrayidx = getelementptr inbounds [2 x i32], ptr %c, i64 0, i64 4294967295, !dbg !35
+ %1 = load i32, ptr %arrayidx, align 4, !dbg !36
+ %and = and i32 %1, %0, !dbg !36
+ store i32 %and, ptr %arrayidx, align 4, !dbg !36
+ %idxprom1 = sext i32 %0 to i64, !dbg !37
+ %arrayidx2 = getelementptr inbounds [2 x i32], ptr %c, i64 0, i64 %idxprom1, !dbg !37
+ store i32 %and, ptr %arrayidx2, align 4, !dbg !38
+ br label %if.end, !dbg !37
+
+if.end: ; preds = %if.then, %tailrecurse
+ br label %tailrecurse, !dbg !23
+}
+
+declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!6, !7, !8, !9, !10, !11, !12}
+!llvm.ident = !{!13}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "a", scope: !2, file: !3, line: 1, type: !5, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C11, file: !3, producer: "clang version 17.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !4, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "test.c", directory: "/")
+!4 = !{!0}
+!5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!6 = !{i32 7, !"Dwarf Version", i32 5}
+!7 = !{i32 2, !"Debug Info Version", i32 3}
+!8 = !{i32 1, !"wchar_size", i32 4}
+!9 = !{i32 8, !"PIC Level", i32 2}
+!10 = !{i32 7, !"PIE Level", i32 2}
+!11 = !{i32 7, !"uwtable", i32 2}
+!12 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
+!13 = !{!"clang version 17.0.0"}
+!14 = distinct !DISubprogram(name: "b", scope: !3, file: !3, line: 2, type: !15, scopeLine: 2, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !17)
+!15 = !DISubroutineType(types: !16)
+!16 = !{null}
+!17 = !{!18}
+!18 = !DILocalVariable(name: "c", scope: !14, file: !3, line: 3, type: !19)
+!19 = !DICompositeType(tag: DW_TAG_array_type, baseType: !5, size: 64, elements: !20)
+!20 = !{!21}
+!21 = !DISubrange(count: 2)
+!22 = distinct !DIAssignID()
+!23 = !DILocation(line: 7, column: 3, scope: !14)
+!24 = !DILocation(line: 0, scope: !14)
+!25 = !DILocation(line: 3, column: 3, scope: !14)
+!26 = !DILocation(line: 3, column: 7, scope: !14)
+!27 = distinct !DIAssignID()
+!28 = !DILocation(line: 5, column: 7, scope: !29)
+!29 = distinct !DILexicalBlock(scope: !14, file: !3, line: 5, column: 7)
+!34 = !DILocation(line: 5, column: 7, scope: !14)
+!35 = !DILocation(line: 6, column: 12, scope: !29)
+!36 = !DILocation(line: 6, column: 17, scope: !29)
+!37 = !DILocation(line: 6, column: 5, scope: !29)
+!38 = !DILocation(line: 6, column: 10, scope: !29)
+!39 = !DILocation(line: 8, column: 1, scope: !14)
|
SLTozer
reviewed
Sep 13, 2023
| (FragInfo && FragInfo->SizeInBits == 0)) | ||
| continue; | ||
|
|
||
| // FragInfo from calculateFragmentIntersect is is nullopt if the |
Comment on lines
1994
to
1995
| // that case just copy the fragment from DAI. Now nullopt means "no | ||
| // fragment info", as per usual. |
There was a problem hiding this comment.
Is this last sentence just restating the above?
There was a problem hiding this comment.
Nope - hopefully the new wording improves clarity?
There was a problem hiding this comment.
It does, that makes more sense!
Comment on lines
1
to
3
| ; RUN: llc %s -stop-after=finalize-isel -o - \ | ||
| ; RUN: | FileCheck %s | ||
| ;--implicit-check-not=DBG_ |
There was a problem hiding this comment.
Looks like like 3 should be part of line 2.
| %arrayidx = getelementptr inbounds [4 x i16], ptr %c, i64 0, i64 2 | ||
| call void @llvm.dbg.assign(metadata i1 undef, metadata !18, metadata !DIExpression(DW_OP_LLVM_fragment, 128, 32), metadata !24, metadata ptr %arrayidx, metadata !DIExpression()), !dbg !26 | ||
|
|
||
| ;; Set variable value (use a call to prevent eliminating redundant DBG_VALUEs). |
There was a problem hiding this comment.
Comment is copied from below, but there's no call on this one; is there meant to be? The debug assign above is definitely redundant with these two being adjacent.
There was a problem hiding this comment.
Ah, no the comment is just copy-pasted. The redundant elimination is fine here as it doesn't impact the behaviour we're testing. The initial dbg.assign is needed to associate a variable (fragment) with the base storage, but we don't need to see a DBG_VALUE proving that.
OCHyams
commented
Sep 13, 2023
Comment on lines
1994
to
1995
| // that case just copy the fragment from DAI. Now nullopt means "no | ||
| // fragment info", as per usual. |
There was a problem hiding this comment.
Nope - hopefully the new wording improves clarity?
Comment on lines
1
to
3
| ; RUN: llc %s -stop-after=finalize-isel -o - \ | ||
| ; RUN: | FileCheck %s | ||
| ;--implicit-check-not=DBG_ |
| %arrayidx = getelementptr inbounds [4 x i16], ptr %c, i64 0, i64 2 | ||
| call void @llvm.dbg.assign(metadata i1 undef, metadata !18, metadata !DIExpression(DW_OP_LLVM_fragment, 128, 32), metadata !24, metadata ptr %arrayidx, metadata !DIExpression()), !dbg !26 | ||
|
|
||
| ;; Set variable value (use a call to prevent eliminating redundant DBG_VALUEs). |
There was a problem hiding this comment.
Ah, no the comment is just copy-pasted. The redundant elimination is fine here as it doesn't impact the behaviour we're testing. The initial dbg.assign is needed to associate a variable (fragment) with the base storage, but we don't need to see a DBG_VALUE proving that.
SLTozer
approved these changes
Sep 14, 2023
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
…res (llvm#66095) Fixes llvm#65004 by trimming assignments from out of bounds stores (out of bounds of either the base variable or the backing alloca). If there's no overlap at all or the out of bounds access starts at a negative offset from the alloca, the assignment is simply skipped.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #65004 by trimming assignments from out of bounds stores (out of bounds
of either the base variable or the backing alloca). If there's no overlap at
all or the out of bounds access starts at a negative offset from the alloca,
the assignment is simply skipped. IMO it's not worth the extra code to
trim the negative-alloca-offset-but-overlaps-variable case.