-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DebugInfo] Fix faulty DIExpression::appendToStack assert #85255
Conversation
The appendToStack() function asserts that no DW_OP_stack_value or DW_OP_LLVM_fragment operations are present in the operations to be appended. The function did that by iterating over all elements in the array rather than just the operations, leading it to falsely asserting on the following input produced by getExt(), since 159 (0x9f) is the DWARF code for DW_OP_stack_value: {dwarf::DW_OP_LLVM_convert, 159, dwarf::DW_ATE_signed} Fix this by using expr_op iterators.
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-ir Author: David Stenberg (dstenb) ChangesThe appendToStack() function asserts that no DW_OP_stack_value or {dwarf::DW_OP_LLVM_convert, 159, dwarf::DW_ATE_signed} Fix this by using expr_op iterators. Full diff: https://github.com/llvm/llvm-project/pull/85255.diff 2 Files Affected:
diff --git a/llvm/lib/IR/DebugInfoMetadata.cpp b/llvm/lib/IR/DebugInfoMetadata.cpp
index 28f96653d815b5..f37add2120f13b 100644
--- a/llvm/lib/IR/DebugInfoMetadata.cpp
+++ b/llvm/lib/IR/DebugInfoMetadata.cpp
@@ -1880,11 +1880,12 @@ DIExpression *DIExpression::append(const DIExpression *Expr,
DIExpression *DIExpression::appendToStack(const DIExpression *Expr,
ArrayRef<uint64_t> Ops) {
assert(Expr && !Ops.empty() && "Can't append ops to this expression");
- assert(none_of(Ops,
- [](uint64_t Op) {
- return Op == dwarf::DW_OP_stack_value ||
- Op == dwarf::DW_OP_LLVM_fragment;
- }) &&
+ assert(std::none_of(expr_op_iterator(Ops.begin()),
+ expr_op_iterator(Ops.end()),
+ [](auto Op) {
+ return Op.getOp() == dwarf::DW_OP_stack_value ||
+ Op.getOp() == dwarf::DW_OP_LLVM_fragment;
+ }) &&
"Can't append this op");
// Append a DW_OP_deref after Expr's current op list if it's non-empty and
diff --git a/llvm/test/DebugInfo/Generic/append-to-stack-assert.ll b/llvm/test/DebugInfo/Generic/append-to-stack-assert.ll
new file mode 100644
index 00000000000000..0c0677f782aa64
--- /dev/null
+++ b/llvm/test/DebugInfo/Generic/append-to-stack-assert.ll
@@ -0,0 +1,49 @@
+; RUN: opt -passes=instcombine -S < %s | FileCheck %s
+
+@f = dso_local local_unnamed_addr global i16 0, align 1, !dbg !0
+@e = dso_local local_unnamed_addr global i159 0, align 1, !dbg !5
+
+; Function Attrs: nounwind
+define dso_local void @g() local_unnamed_addr #0 !dbg !14 {
+; CHECK: [[TMP0:%.*]] = load i16, ptr @f, align 1
+; CHECK: [[CONV1:%.*]] = sext i16 [[TMP0]] to i159
+; CHECK: tail call void @llvm.dbg.value(metadata i159 [[CONV1]], metadata {{![0-9]+}}, metadata !DIExpression(DW_OP_LLVM_convert, 159, DW_ATE_signed, DW_OP_LLVM_convert, 256, DW_ATE_signed, DW_OP_stack_value))
+entry:
+ %0 = load i16, ptr @f, align 1, !dbg !21
+ %conv = sext i16 %0 to i256, !dbg !21
+ tail call void @llvm.dbg.value(metadata i256 %conv, metadata !18, metadata !DIExpression()), !dbg !22
+ %conv1 = trunc i256 %conv to i159, !dbg !23
+ store i159 %conv1, ptr @e, align 1, !dbg !23
+ ret void, !dbg !23
+}
+
+attributes #0 = { nounwind }
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!9, !10, !11, !12}
+!llvm.ident = !{!13}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "f", scope: !2, file: !3, line: 7, type: !8, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C11, file: !3, producer: "clang version 19.0.0git", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !4, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "bbi-93380.c", directory: "/")
+!4 = !{!0, !5}
+!5 = !DIGlobalVariableExpression(var: !6, expr: !DIExpression())
+!6 = distinct !DIGlobalVariable(name: "e", scope: !2, file: !3, line: 8, type: !7, isLocal: false, isDefinition: true)
+!7 = !DIBasicType(name: "_BitInt", size: 160, encoding: DW_ATE_signed)
+!8 = !DIBasicType(name: "int", size: 16, encoding: DW_ATE_signed)
+!9 = !{i32 7, !"Dwarf Version", i32 4}
+!10 = !{i32 2, !"Debug Info Version", i32 3}
+!11 = !{i32 1, !"wchar_size", i32 1}
+!12 = !{i32 7, !"frame-pointer", i32 2}
+!13 = !{!"clang version 19.0.0git"}
+!14 = distinct !DISubprogram(name: "g", scope: !3, file: !3, line: 9, type: !15, scopeLine: 9, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !17)
+!15 = !DISubroutineType(types: !16)
+!16 = !{null}
+!17 = !{!18}
+!18 = !DILocalVariable(name: "d", scope: !19, file: !3, line: 9, type: !20)
+!19 = distinct !DILexicalBlock(scope: !14, file: !3, line: 9)
+!20 = !DIBasicType(name: "_BitInt", size: 256, encoding: DW_ATE_signed)
+!21 = !DILocation(line: 9, scope: !19)
+!22 = !DILocation(line: 0, scope: !19)
+!23 = !DILocation(line: 9, scope: !14)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice find. LGTM, thanks.
@@ -0,0 +1,49 @@ | |||
; RUN: opt -passes=instcombine -S < %s | FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An instcombine test in order to test a change to DIEExpression::append operation feels like the wrong granularity. Not to mention this test could start failing for other reasons in the future.
Suggestion: could you have a look at unittests and check if you can build a DIEExpression manually there, and then call appendToStack
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, seems reasonable to explore putting this in a unittest instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I have changed this to a unit test now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for improving the test
Thanks for the reviews! |
The appendToStack() function asserts that no DW_OP_stack_value or
DW_OP_LLVM_fragment operations are present in the operations to be
appended. The function did that by iterating over all elements in the
array rather than just the operations, leading it to falsely asserting
on the following input produced by getExt(), since 159 (0x9f) is the
DWARF code for DW_OP_stack_value:
{dwarf::DW_OP_LLVM_convert, 159, dwarf::DW_ATE_signed}
Fix this by using expr_op iterators.