Skip to content

Conversation

jmorse
Copy link
Member

@jmorse jmorse commented Oct 31, 2023

The base-type field of a DICompositeType is optional in the IR syntax, however it makes no sense to have an array of an unspecified type. Such debug-info would be meaningless, and the added test crashes otherwise (see #70787). Thus, add a verifier check to reject such ill-formed debug info metadata. Test produced by Christian Ulmann.

fixes #70787

The base-type field of a DICompositeType is optional in the IR syntax,
however it makes no sense to have an array of an unspecified type. Such
debug-info would be meaningless, and the added test crashes otherwise.
Thus, add a verifier check to reject such ill-formed debug info metadata.
Test produced by Christian Ulmann.

fixes llvm#70787
@llvmbot
Copy link
Member

llvmbot commented Oct 31, 2023

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Changes

The base-type field of a DICompositeType is optional in the IR syntax, however it makes no sense to have an array of an unspecified type. Such debug-info would be meaningless, and the added test crashes otherwise (see #70787). Thus, add a verifier check to reject such ill-formed debug info metadata. Test produced by Christian Ulmann.

fixes #70787


Full diff: https://github.com/llvm/llvm-project/pull/70803.diff

2 Files Affected:

  • (modified) llvm/lib/IR/Verifier.cpp (+5)
  • (added) llvm/test/DebugInfo/Generic/arrays-need-types.ll (+27)
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 396af600b8dab29..45e49a9741f5943 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -1287,6 +1287,11 @@ void Verifier::visitDICompositeType(const DICompositeType &N) {
     CheckDI(N.getTag() == dwarf::DW_TAG_array_type,
             "rank can only appear in array type");
   }
+
+  if (N.getTag() == dwarf::DW_TAG_array_type) {
+    CheckDI(N.getRawBaseType(),
+            "array types must have a base type", &N);
+  }
 }
 
 void Verifier::visitDISubroutineType(const DISubroutineType &N) {
diff --git a/llvm/test/DebugInfo/Generic/arrays-need-types.ll b/llvm/test/DebugInfo/Generic/arrays-need-types.ll
new file mode 100644
index 000000000000000..a1b7c963d384433
--- /dev/null
+++ b/llvm/test/DebugInfo/Generic/arrays-need-types.ll
@@ -0,0 +1,27 @@
+; RUN: opt %s -o - -S --passes=verify 2>&1 | FileCheck %s
+
+; CHECK:      array types must have a base type
+; CHECK-NEXT: !DICompositeType(tag: DW_TAG_array_type,
+; CHECK-NEXT: warning: ignoring invalid debug info
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+define i32 @func(ptr %0) !dbg !3 {
+  call void @llvm.dbg.value(metadata ptr %0, metadata !6, metadata !DIExpression()), !dbg !10
+  ret i32 0
+}
+
+!llvm.module.flags = !{!0}
+!llvm.dbg.cu = !{!1}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DICompileUnit(language: DW_LANG_C11, file: !2, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!2 = !DIFile(filename: "file.c", directory: "/")
+!3 = distinct !DISubprogram(name: "func", scope: !2, file: !2, line: 46, type: !4, scopeLine: 48, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1)
+!4 = distinct !DISubroutineType(types: !5)
+!5 = !{}
+!6 = !DILocalVariable(name: "op", arg: 5, scope: !3, file: !2, line: 47, type: !7)
+!7 = !DICompositeType(tag: DW_TAG_array_type, size: 2624, elements: !8)
+!8 = !{!9}
+!9 = !DISubrange(count: 41)
+!10 = !DILocation(line: 0, scope: !3)

Copy link
Contributor

@Dinistro Dinistro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this 😄
Looks reasonable to me, but I suggest to wait for someone else's review.

Copy link

github-actions bot commented Oct 31, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds OK to me - though probably run it on some real codebases first, just in case there's some clang bugs we need to fix before we can take this live?

@jmorse
Copy link
Member Author

jmorse commented Nov 1, 2023

Yeah, I've done a stage2 clang build and it didn't hit any problems, I'll give it a shot with the test suite too first.

@Dinistro
Copy link
Contributor

Dinistro commented Nov 6, 2023

Did the tests pass, if so, could you address the formatting issues?

@jmorse jmorse merged commit 4a8b0ea into llvm:main Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LLVM][DebugInfo] Strengthen DI metadata verifiers
4 participants