[mono][interp] Fix miscompile of self-assignment via newobj with alised byref args. - #131586
Conversation
…sed byref args
The interpreter's optimized tier (INTERP_OPT_SUPER_INSTRUCTIONS) miscompiled
a self-reassignment through a constructor whose `in` (byref) parameters
alias the destination, e.g.:
a = new GEJ(a.x, a.y, a.z, a.infinity);
Per ECMA-335, `newobj` must construct into a temporary and only then copy the
result to `a`, so the constructor observes the old value of `a` through the
`in` pointers. The `interp_super_instructions` "forward dreg" pass was
retargeting the constructed value's store directly into the address-taken
local `a` (`def->dreg = dreg`), eliminating the intermediate move. This made
the constructor read the very storage it was simultaneously writing, zeroing
the fields once the method tiered up to the optimized tier (observed on Android
after ~1000 iterations, thats when the interpreter tiering kicks in).
Add an address-taken guard (`var_has_indirects`) before the retarget, bailing
out when either the source or destination local has had its address taken. This
mirrors the existing guard in `interp_cprop` and the Mono JIT `vreg_is_volatile`
discipline in local-propagation.c.
Add a regression test under JIT/Regression/JitBlue/Runtime_122237. It fails
before the fix and passes after, verified under both the default (auto) and
forced interpreter tiering modes.
Fixes dotnet#122237
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @vitek-karas, @BrzVlad, @kotlarmilos |
There was a problem hiding this comment.
Pull request overview
Fixes a Mono interpreter optimized-tier miscompile by preventing a copy-elimination/forwarding optimization from retargeting stores involving address-taken locals, and adds a regression test to cover the self-assignment + newobj + aliased in-byref scenario.
Changes:
- Add an indirect-local (address-taken) guard in
interp_super_instructionsbefore forwarding a definition’sdregacross amov. - Add a new JIT regression test reproducing the self-reassignment-through-aliased-
in-args pattern. - Wire the new regression test into the existing
Regression_ro_2test project.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/mono/mono/mini/interp/transform-opt.c | Skips the “forward dreg” retargeting optimization when either source or destination local is indirect/address-taken. |
| src/tests/JIT/Regression/Regression_ro_2.csproj | Adds the new Runtime_122237 test file to the compile items. |
| src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.cs | New regression test validating that repeated a = new GEJ(a.x, a.y, a.z, a.infinity) doesn’t corrupt a after tiering/optimization. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/mono/mono/mini/interp/transform-opt.c:3855
- The new indirect-local guard uses an empty
if (...) { /*comment*/ } else if (...)pattern. It works, but the empty block makes the control flow easy to misread and is inconsistent with nearby patterns (e.g.,interp_cpropuses a single combined condition). Consider folding!var_has_indirectsinto the main condition and dropping the empty block.
int dreg = ins->dreg;
if (var_has_indirects (td, dreg)) {
// Don't bother with indirect locals
}
// if var is not ssa or it is a renamed fixed, then we can't replace the dreg
|
can you backport to release/10.0 branch? |
|
/backport to release/10.0 |
|
Started backporting to |
|
@lateralusX backporting to git am output$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: [mono][interp] Fix miscompile of self-assignment via newobj with aliased byref args
Using index info to reconstruct a base tree...
A src/tests/JIT/Regression/Regression_ro_2.csproj
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): src/tests/JIT/Regression/Regression_ro_2.csproj deleted in HEAD and modified in [mono][interp] Fix miscompile of self-assignment via newobj with aliased byref args. Version [mono][interp] Fix miscompile of self-assignment via newobj with aliased byref args of src/tests/JIT/Regression/Regression_ro_2.csproj left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 [mono][interp] Fix miscompile of self-assignment via newobj with aliased byref args
Error: The process '/usr/bin/git' failed with exit code 128 |
|
|
Will do manual backport during the week. |
The interpreter's optimized tier (INTERP_OPT_SUPER_INSTRUCTIONS) miscompiled a self-reassignment through a constructor whose
in(byref) parameters alias the destination, e.g.:Per ECMA-335,
newobjmust construct into a temporary and only then copy the result toa, so the constructor observes the old value ofathrough theinpointers. Theinterp_super_instructions"forward dreg" pass was retargeting the constructed value's store directly into the address-taken locala(def->dreg = dreg), eliminating the intermediate move. This made the constructor read the very storage it was simultaneously writing, zeroing the fields once the method tiered up to the optimized tier (observed on Android after ~1000 iterations, thats when the interpreter tiering kicks in).Add an address-taken guard (
var_has_indirects) before the retarget, bailing out when either the source or destination local has had its address taken. This mirrors the existing guard ininterp_cpropand the Mono JITvreg_is_volatilediscipline in local-propagation.c.Add a regression test under JIT/Regression/JitBlue/Runtime_122237. It fails before the fix and passes after, verified under both the default (auto) and forced interpreter tiering modes.
Fixes #122237