[jdk21u-dev] RFR: 8358334: C2/Shenandoah: incorrect execution with Unsafe
Aleksey Shipilev
shade at openjdk.org
Thu Jul 3 14:06:41 UTC 2025
On Wed, 18 Jun 2025 13:48:14 GMT, Roland Westrelin <roland at openjdk.org> wrote:
> Patch doesn't apply cleanly:
>
> - jdk21u still has IU barriers: context for some of the changes is
> different.
>
> - Because of IU barriers, there's an extra call to `fix_ctrl()` for
> which the renaming to `nodes_above_barriers` must be applied.
>
> - The initial patch makes a subtle change that doesn't affect jdk
> 25/26 in the code that was
> refactored. `ShenandoahBarrierC2Support::push_data_inputs_at_control()`
> is introduced with logic:
>
> if (in != nullptr && phase->has_ctrl(in) && phase->get_ctrl(in) == ctrl) {
>
>
> which replaces the same logic in
> `ShenandoahBarrierC2Support::fix_ctrl()` but also a slightly
> different logic in
> `ShenandoahBarrierC2Support::is_dominator_same_ctrl()`:
>
>
> if (m->in(i) != nullptr && phase->ctrl_or_self(m->in(i)) == c) {
>
>
> that is, it uses `ctrl_or_self()` which works for both data nodes
> and control nodes but the new method uses `has_ctrl(in) &&
> get_ctrl(in)` which can only be true for data nodes. That change
> causes failures in jdk 21 again because of IU barriers that produce
> a new memory state when expanded and need the logic from
> `MemoryGraphFixer`. What I propose in this backport to be on the
> safe side is to leave
> `ShenandoahBarrierC2Support::is_dominator_same_ctrl()` alone (not
> apply that part of the refactoring).
>
> Tested with hotspot_gc_shenandoah + tier1 with -XX:+UseShenandoahGC
I took a deeper look at this, and I have a concern about the divergence from mainline. It might be that some subsequent backport that applies to `maybe_push_anti_dependent_loads` or `push_data_inputs_at_control` would miss the need to update `is_dominator_same_ctrl`.
IU mode is also experimental, so risking the default (SATB) mode this way to avoid the breakage if IU seems doubly awkward.
So, seeing that the trouble is in `push_data_inputs_at_control`, I propose/suggest we pick up all the hunks from mainline, but change `push_data_inputs_at_control` to clearly demarcate the difference for IU mode. E.g.:
void ShenandoahBarrierC2Support::push_data_inputs_at_control(PhaseIdealLoop* phase, Node* n, Node* ctrl, Unique_Node_List &wq) {
for (uint i = 0; i < n->req(); i++) {
Node* in = n->in(i);
if (in != nullptr && (ShenandoahIUBarrier ? (phase->ctrl_or_self(in)) == ctrl) : (phase->has_ctrl(in) && phase->get_ctrl(in) == ctrl))) {
wq.push(in);
}
}
}
Would that work, @rwestrel?
-------------
PR Review: https://git.openjdk.org/jdk21u-dev/pull/1901#pullrequestreview-2983331511
More information about the jdk-updates-dev
mailing list