- Notifications
You must be signed in to change notification settings - Fork 15.4k
[LLVM-Reduce] - Null pointer handling during distinct metadata reduction #117570
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
Conversation
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.
| @arsenm Hey, found a small issue with the metadata node reduction algorithm, could you please take a look? |
arsenm left a comment
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.
Seems like an API flaw but I know nothing about debug info
| @llvm/pr-subscribers-debuginfo Author: Robert Barinov (rbintel) ChangesSome 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:
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); } } |
michalpaszkowski left a comment
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.
@rbintel Thanks for the patch! LGTM!
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.