[mono][interp] Fix broken code attempting to reapply superinstruction optimization#116069
Merged
BrzVlad merged 1 commit intodotnet:mainfrom May 30, 2025
Merged
Conversation
… optimization For each instruction in a basic block we check for patterns. In a certain case, once we replaced the instruction with a new one, we were attempting to do a loop reiteration by setting `ins = ins->prev` so after the loop reincrements with `ins = ins->next` we check super instruction patterns again for the current instruction. This is broken if `ins` was the first instruction in a basic block, aka `ins->prev` is NULL. This used to be impossible before SSA optimizations, since super instruction pass was applying optimizations in a single basic block only.
BrzVlad
requested review from
kotlarmilos,
steveisok and
vitek-karas
as code owners
github-actions
bot
added
the
needs-area-label
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the superinstruction reprocessing logic by eliminating an unsafe pointer dereference when inspecting the previous instruction.
- Removed the use of ins = ins->prev to avoid null pointer access
- Introduced a goto retry_ins label to re-read the opcode of the current instruction
| @@ -3804,9 +3806,7 @@ interp_super_instructions (TransformData *td) | |||
| g_print ("superins: "); | |||
| interp_dump_ins (ins, td->data_items); | |||
| } | |||
There was a problem hiding this comment.
Replacing 'ins = ins->prev' with 'goto retry_ins' avoids a potential null pointer dereference, but consider adding a comment to clarify the control flow and ensure readability for future maintainers.
Suggested change
| } | |
| } | |
| // Retry processing the current instruction after modifying its opcode and operands. |
BrzVlad
added
area-Codegen-Interpreter-mono
and removed
needs-area-label
Contributor
|
Tagging subscribers to this area: @BrzVlad, @kotlarmilos |
kotlarmilos
approved these changes
May 29, 2025
Comment on lines
+3406
to
+3410
| int opcode; | ||
| if (bb->dfs_index >= td->bblocks_count_no_eh || bb->dfs_index == -1 || (ins->flags & INTERP_INST_FLAG_LIVENESS_MARKER)) | ||
| current_liveness.ins_index++; | ||
| retry_ins: | ||
| opcode = ins->opcode; |
Contributor
There was a problem hiding this comment.
Suggested change
| int opcode; | |
| if (bb->dfs_index >= td->bblocks_count_no_eh || bb->dfs_index == -1 || (ins->flags & INTERP_INST_FLAG_LIVENESS_MARKER)) | |
| current_liveness.ins_index++; | |
| retry_ins: | |
| opcode = ins->opcode; | |
| if (bb->dfs_index >= td->bblocks_count_no_eh || bb->dfs_index == -1 || (ins->flags & INTERP_INST_FLAG_LIVENESS_MARKER)) | |
| current_liveness.ins_index++; | |
| retry_ins: | |
| int opcode = ins->opcode; |
Member
Author
There was a problem hiding this comment.
that would lead to compilation failures because opcode needs to be declared before the label.
Contributor
|
so could you backport to release/v9.0 ? |
BrzVlad
added a commit
to BrzVlad/runtime
that referenced
this pull request
Jun 9, 2025
… optimization (dotnet#116069) For each instruction in a basic block we check for patterns. In a certain case, once we replaced the instruction with a new one, we were attempting to do a loop reiteration by setting `ins = ins->prev` so after the loop reincrements with `ins = ins->next` we check super instruction patterns again for the current instruction. This is broken if `ins` was the first instruction in a basic block, aka `ins->prev` is NULL. This used to be impossible before SSA optimizations, since super instruction pass was applying optimizations in a single basic block only.
jozkee
pushed a commit
that referenced
this pull request
Jun 10, 2025
* [mono][interp] Add possibility to configure interp options from env var * [mono][interp] Update var definition when inserting new instructions during cprop (#116179) The definition was not updated, leading to invalid optimizations later on. * [mono][interp] Fix broken code attempting to reapply superinstruction optimization (#116069) For each instruction in a basic block we check for patterns. In a certain case, once we replaced the instruction with a new one, we were attempting to do a loop reiteration by setting `ins = ins->prev` so after the loop reincrements with `ins = ins->next` we check super instruction patterns again for the current instruction. This is broken if `ins` was the first instruction in a basic block, aka `ins->prev` is NULL. This used to be impossible before SSA optimizations, since super instruction pass was applying optimizations in a single basic block only.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
For each instruction in a basic block we check for patterns. In a certain case, once we replaced the instruction with a new one, we were attempting to do a loop reiteration by setting
ins = ins->prevso after the loop reincrements withins = ins->nextwe check super instruction patterns again for the current instruction. This is broken ifinswas the first instruction in a basic block, akains->previs NULL. This used to be impossible before SSA optimizations, since super instruction pass was applying optimizations in a single basic block only.Bug reported in xoofx/markdig#873