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