Skip to content

Conversation

@rbintel
Copy link
Contributor

@rbintel rbintel commented Nov 25, 2024

Some distinct metadata nodes, e.g DICompileUnit, have implicit nullptrs inside them. Iterating over them with dyn_cast leads to a crash, change the behavior so that the nullptr operands are skipped.

Add the test distinct-metadata-nullptr.ll which will crash if null pointers are not handled correctly.

inside them. Iterating over them with dyn_cast leads to a crash, change
the behavior so that the nullptr operands are skipped.

Add the test distinct-metadata-nullptr.ll which will crash if null
pointers are not handled correctly.
@rbintel
Copy link
Contributor Author

rbintel commented Nov 25, 2024

@arsenm Hey, found a small issue with the metadata node reduction algorithm, could you please take a look?

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

Seems like an API flaw but I know nothing about debug info

@llvmbot
Copy link
Member

llvmbot commented Nov 25, 2024

@llvm/pr-subscribers-debuginfo

Author: Robert Barinov (rbintel)

Changes

Some distinct metadata nodes, e.g DICompileUnit, have implicit nullptrs inside them. Iterating over them with dyn_cast leads to a crash, change the behavior so that the nullptr operands are skipped.

Add the test distinct-metadata-nullptr.ll which will crash if null pointers are not handled correctly.


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

2 Files Affected:

  • (added) llvm/test/tools/llvm-reduce/distinct-dimetadata-nullptr.ll (+17)
  • (modified) llvm/tools/llvm-reduce/deltas/ReduceDistinctMetadata.cpp (+5-4)
diff --git a/llvm/test/tools/llvm-reduce/distinct-dimetadata-nullptr.ll b/llvm/test/tools/llvm-reduce/distinct-dimetadata-nullptr.ll
new file mode 100644
index 00000000000000..5861adc65b83c5
--- /dev/null
+++ b/llvm/test/tools/llvm-reduce/distinct-dimetadata-nullptr.ll
@@ -0,0 +1,17 @@
+; Test checking that distinct metadata reduction pass handles null pointers properly.
+; This test will lead to a crash if nullptrs inside distinct metadata are not handled correctly, in this case inside DICompileUnit
+
+; RUN: llvm-reduce --delta-passes=distinct-metadata --aggressive-named-md-reduction --test FileCheck --test-arg %s --test-arg --input-file %s -o %t
+; CHECK: {{.*}}distinct !DICompileUnit{{.*}}
+
+
+!llvm.module.flags = !{!0, !1, !6}
+!llvm.dbg.cu = !{!4}
+
+!0 = !{i32 7, !"Dwarf Version", i32 4}
+!1 = !{i32 2, !"Source Lang Literal", !2}
+!2 = !{!3}
+!3 = !{!4, i32 33}
+!4 = distinct !DICompileUnit(language: DW_LANG_OpenCL, file: !5, producer: "foobar", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
+!5 = !DIFile(filename: "main.cpp", directory: "foodir")
+!6 = !{i32 2, !"Debug Info Version", i32 3}
diff --git a/llvm/tools/llvm-reduce/deltas/ReduceDistinctMetadata.cpp b/llvm/tools/llvm-reduce/deltas/ReduceDistinctMetadata.cpp
index 02129263f6af44..0f46409977a336 100644
--- a/llvm/tools/llvm-reduce/deltas/ReduceDistinctMetadata.cpp
+++ b/llvm/tools/llvm-reduce/deltas/ReduceDistinctMetadata.cpp
@@ -39,7 +39,7 @@ reduceNodes(MDNode *Root,
     // Mark the nodes for removal
     for (unsigned int I = 0; I < CurrentNode->getNumOperands(); ++I) {
       if (MDNode *Operand =
-              dyn_cast<MDNode>(CurrentNode->getOperand(I).get())) {
+              dyn_cast_or_null<MDNode>(CurrentNode->getOperand(I).get())) {
         // Check whether node has been visited
         if (VisitedNodes.insert(Operand))
           NodesToTraverse.push(Operand);
@@ -71,7 +71,7 @@ static void cleanUpTemporaries(NamedMDNode &NamedNode, MDTuple *TemporaryTuple,
   for (auto I = NamedNode.op_begin(); I != NamedNode.op_end(); ++I) {
     // If the node hasn't been traversed yet, add it to the queue of nodes to
     // traverse.
-    if (MDTuple *TupleI = dyn_cast<MDTuple>((*I))) {
+    if (MDTuple *TupleI = dyn_cast_or_null<MDTuple>((*I))) {
       if (VisitedNodes.insert(TupleI))
         NodesToTraverse.push(TupleI);
     }
@@ -108,7 +108,8 @@ static void cleanUpTemporaries(NamedMDNode &NamedNode, MDTuple *TemporaryTuple,
 
     // Push the remaining nodes into the queue
     for (unsigned int I = 0; I < CurrentTuple->getNumOperands(); ++I) {
-      MDTuple *Operand = dyn_cast<MDTuple>(CurrentTuple->getOperand(I).get());
+      MDTuple *Operand =
+          dyn_cast_or_null<MDTuple>(CurrentTuple->getOperand(I).get());
       if (Operand && VisitedNodes.insert(Operand))
         // If the node hasn't been traversed yet, add it to the queue of nodes
         // to traverse.
@@ -127,7 +128,7 @@ static void extractDistinctMetadataFromModule(Oracle &O,
        Program.named_metadata()) { // Iterate over the named nodes
     for (unsigned int I = 0; I < NamedNode.getNumOperands();
          ++I) { // Iterate over first level unnamed nodes..
-      if (MDTuple *Operand = dyn_cast<MDTuple>(NamedNode.getOperand(I)))
+      if (MDTuple *Operand = dyn_cast_or_null<MDTuple>(NamedNode.getOperand(I)))
         reduceNodes(Operand, NodesToDelete, TemporaryTuple, O, Program);
     }
   }

Copy link
Member

@michalpaszkowski michalpaszkowski left a comment

Choose a reason for hiding this comment

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

@rbintel Thanks for the patch! LGTM!

@michalpaszkowski michalpaszkowski merged commit 0988bf8 into llvm:main Nov 25, 2024
10 checks passed
@rbintel rbintel deleted the fixdistinct branch November 25, 2024 22:22
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.

4 participants