Fix null-deref when inlining an If branch that outputs an initializer#314
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a crash in the eliminate_if_with_const_cond optimization pass when inlining a constant-condition If whose taken-branch output is not produced by a node (e.g., forwarded initializer / forwarded captured value). This improves robustness when the optimizer is run after constant-folding/extraction steps that turn Constant nodes into initializers.
Changes:
- Resolves subgraph outputs via
value_dictwhen possible, and adds explicit handling forkCapturedandkParamoutputs when the output has no producing node. - Copies subgraph initializers into the parent graph for
kParamoutputs to avoid inserting nulls into the value map and segfaulting onreplaceAllUsesWith.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+122
to
+124
| auto *captured_node = parent_graph.create(kCaptured, 1); | ||
| captured_node->output()->setUniqueName(unique_name); | ||
| new_output = captured_node->output(); |
Comment on lines
+128
to
+132
| } else if (output_in_subgraph->node()->kind() == kParam) { | ||
| // The taken branch forwards a constant initializer straight to its | ||
| // output. This happens, for example, when constant folding (as done by | ||
| // onnx-simplifier) has already turned the branch's Constant node into a | ||
| // subgraph initializer, so there is no node producing this output and |
Signed-off-by: take-cheeze <takechi101010@gmail.com>
eliminate_if_with_const_cond inlines the taken branch of an If whose condition is a compile-time constant by re-creating the branch's nodes in the parent graph and mapping each subgraph output to its newly created parent value via value_dict. value_dict is populated only from the outputs of the branch's *nodes*. When a branch output is produced by a constant initializer (kParam) or is a value captured from an outer scope (kCaptured) rather than by a node, its unique name is absent from value_dict. The output loop then did `value_dict[name]`, which default-inserts a null Value*, and `replaceAllUsesWith(nullptr)` dereferenced it and crashed (segfault). This is easy to trigger together with constant folding: onnx-simplifier's extract_constant_to_initializer turns a branch's `Constant` node into a subgraph initializer, after which that branch's output is a kParam value with no producing node. Handle the kParam and kCaptured cases in the output loop the same way the node-input loop already does: copy a kParam initializer into the parent graph, and resolve a kCaptured value from the parent graph (creating a captured node when it comes from a further-out scope). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: take-cheeze <takechi101010@gmail.com>
- Insert the newly created kCaptured node into the parent graph so that downstream nodes made to depend on it via replaceAllUsesWith reference an attached node instead of a floating one, keeping the graph valid. - Add a regression test covering the output-only case where the taken If-branch forwards a subgraph initializer straight to its output. It runs extract_constant_to_initializer before eliminate_if_with_const_cond to reproduce the onnxsim/onnxsim#452 segfault, which the pass now handles without dereferencing a null Value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JZiQnQnUQS3UCA9xzsocn1 Signed-off-by: take-cheeze <takechi101010@gmail.com>
take-cheeze
force-pushed
the
fix-if-const-cond-null-deref
branch
from
July 22, 2026 04:24
3c79809 to
ccefa42
Compare
justinchuby
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to onnxsim/onnxsim#452
eliminate_if_with_const_cond inlines the taken branch of an If whose condition is a compile-time constant by re-creating the branch's nodes in the parent graph and mapping each subgraph output to its newly created parent value via value_dict.
value_dict is populated only from the outputs of the branch's nodes. When a branch output is produced by a constant initializer (kParam) or is a value captured from an outer scope (kCaptured) rather than by a node, its unique name is absent from value_dict. The output loop then did
value_dict[name], which default-inserts a null Value*, andreplaceAllUsesWith(nullptr)dereferenced it and crashed (segfault).This is easy to trigger together with constant folding: onnx-simplifier's extract_constant_to_initializer turns a branch's
Constantnode into a subgraph initializer, after which that branch's output is a kParam value with no producing node.Handle the kParam and kCaptured cases in the output loop the same way the node-input loop already does: copy a kParam initializer into the parent graph, and resolve a kCaptured value from the parent graph (creating a captured node when it comes from a further-out scope).