From 25cba14fc668aca1781f24afb10032121f2f8f94 Mon Sep 17 00:00:00 2001 From: rwestrel Date: Tue, 17 Jun 2025 10:22:50 +0200 Subject: [PATCH 1/2] Backport 46cfc1e1940ff6b91c4f0cb0a9161fd0aef37c38 --- .../gc/shenandoah/c2/shenandoahSupport.cpp | 68 +++++++++++++------ .../gc/shenandoah/c2/shenandoahSupport.hpp | 11 ++- .../TestLostAntiDependencyAtExpansion.java | 67 ++++++++++++++++++ 3 files changed, 125 insertions(+), 21 deletions(-) create mode 100644 test/hotspot/jtreg/gc/shenandoah/compiler/TestLostAntiDependencyAtExpansion.java diff --git a/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp b/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp index 168d8769195..f2b20e9606f 100644 --- a/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp +++ b/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp @@ -639,6 +639,34 @@ void ShenandoahBarrierC2Support::verify(RootNode* root) { } #endif +bool ShenandoahBarrierC2Support::is_anti_dependent_load_at_control(PhaseIdealLoop* phase, Node* maybe_load, Node* store, + Node* control) { + return maybe_load->is_Load() && phase->C->can_alias(store->adr_type(), phase->C->get_alias_index(maybe_load->adr_type())) && + phase->ctrl_or_self(maybe_load) == control; +} + +void ShenandoahBarrierC2Support::maybe_push_anti_dependent_loads(PhaseIdealLoop* phase, Node* maybe_store, Node* control, Unique_Node_List &wq) { + if (!maybe_store->is_Store() && !maybe_store->is_LoadStore()) { + return; + } + Node* mem = maybe_store->in(MemNode::Memory); + for (DUIterator_Fast imax, i = mem->fast_outs(imax); i < imax; i++) { + Node* u = mem->fast_out(i); + if (is_anti_dependent_load_at_control(phase, u, maybe_store, control)) { + wq.push(u); + } + } +} + +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 && phase->has_ctrl(in) && phase->get_ctrl(in) == ctrl) { + wq.push(in); + } + } +} + bool ShenandoahBarrierC2Support::is_dominator_same_ctrl(Node* c, Node* d, Node* n, PhaseIdealLoop* phase) { // That both nodes have the same control is not sufficient to prove // domination, verify that there's no path from d to n @@ -1020,7 +1048,20 @@ void ShenandoahBarrierC2Support::call_lrb_stub(Node*& ctrl, Node*& val, Node* lo phase->register_new_node(val, ctrl); } -void ShenandoahBarrierC2Support::fix_ctrl(Node* barrier, Node* region, const MemoryGraphFixer& fixer, Unique_Node_List& uses, Unique_Node_List& uses_to_ignore, uint last, PhaseIdealLoop* phase) { +void ShenandoahBarrierC2Support::collect_nodes_above_barrier(Unique_Node_List &nodes_above_barrier, PhaseIdealLoop* phase, Node* ctrl, Node* init_raw_mem) { + nodes_above_barrier.clear(); + if (phase->has_ctrl(init_raw_mem) && phase->get_ctrl(init_raw_mem) == ctrl && !init_raw_mem->is_Phi()) { + nodes_above_barrier.push(init_raw_mem); + } + for (uint next = 0; next < nodes_above_barrier.size(); next++) { + Node* n = nodes_above_barrier.at(next); + // Take anti-dependencies into account + maybe_push_anti_dependent_loads(phase, n, ctrl, nodes_above_barrier); + push_data_inputs_at_control(phase, n, ctrl, nodes_above_barrier); + } +} + +void ShenandoahBarrierC2Support::fix_ctrl(Node* barrier, Node* region, const MemoryGraphFixer& fixer, Unique_Node_List& uses, Unique_Node_List& nodes_above_barrier, uint last, PhaseIdealLoop* phase) { Node* ctrl = phase->get_ctrl(barrier); Node* init_raw_mem = fixer.find_mem(ctrl, barrier); @@ -1031,30 +1072,17 @@ void ShenandoahBarrierC2Support::fix_ctrl(Node* barrier, Node* region, const Mem // control will be after the expanded barrier. The raw memory (if // its memory is control dependent on the barrier's input control) // must stay above the barrier. - uses_to_ignore.clear(); - if (phase->has_ctrl(init_raw_mem) && phase->get_ctrl(init_raw_mem) == ctrl && !init_raw_mem->is_Phi()) { - uses_to_ignore.push(init_raw_mem); - } - for (uint next = 0; next < uses_to_ignore.size(); next++) { - Node *n = uses_to_ignore.at(next); - for (uint i = 0; i < n->req(); i++) { - Node* in = n->in(i); - if (in != nullptr && phase->has_ctrl(in) && phase->get_ctrl(in) == ctrl) { - uses_to_ignore.push(in); - } - } - } + collect_nodes_above_barrier(nodes_above_barrier, phase, ctrl, init_raw_mem); for (DUIterator_Fast imax, i = ctrl->fast_outs(imax); i < imax; i++) { Node* u = ctrl->fast_out(i); if (u->_idx < last && u != barrier && !u->depends_only_on_test() && // preserve dependency on test - !uses_to_ignore.member(u) && + !nodes_above_barrier.member(u) && (u->in(0) != ctrl || (!u->is_Region() && !u->is_Phi())) && (ctrl->Opcode() != Op_CatchProj || u->Opcode() != Op_CreateEx)) { Node* old_c = phase->ctrl_or_self(u); - Node* c = old_c; - if (c != ctrl || + if (old_c != ctrl || is_dominator_same_ctrl(old_c, barrier, u, phase) || ShenandoahBarrierSetC2::is_shenandoah_state_load(u)) { phase->igvn().rehash_node_delayed(u); @@ -1334,7 +1362,7 @@ void ShenandoahBarrierC2Support::pin_and_expand(PhaseIdealLoop* phase) { // Expand load-reference-barriers MemoryGraphFixer fixer(Compile::AliasIdxRaw, true, phase); - Unique_Node_List uses_to_ignore; + Unique_Node_List nodes_above_barriers; for (int i = state->load_reference_barriers_count() - 1; i >= 0; i--) { ShenandoahLoadReferenceBarrierNode* lrb = state->load_reference_barrier(i); uint last = phase->C->unique(); @@ -1429,7 +1457,7 @@ void ShenandoahBarrierC2Support::pin_and_expand(PhaseIdealLoop* phase) { Node* out_val = val_phi; phase->register_new_node(val_phi, region); - fix_ctrl(lrb, region, fixer, uses, uses_to_ignore, last, phase); + fix_ctrl(lrb, region, fixer, uses, nodes_above_barriers, last, phase); ctrl = orig_ctrl; @@ -1585,7 +1613,7 @@ void ShenandoahBarrierC2Support::pin_and_expand(PhaseIdealLoop* phase) { phase->register_control(region, loop, heap_stable_ctrl->in(0)); phase->register_new_node(phi, region); - fix_ctrl(barrier, region, fixer, uses, uses_to_ignore, last, phase); + fix_ctrl(barrier, region, fixer, uses, nodes_above_barriers, last, phase); for(uint next = 0; next < uses.size(); next++ ) { Node *n = uses.at(next); assert(phase->get_ctrl(n) == init_ctrl, "bad control"); diff --git a/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.hpp b/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.hpp index 7a6ed74f563..8db929037db 100644 --- a/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.hpp +++ b/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.hpp @@ -62,12 +62,16 @@ class ShenandoahBarrierC2Support : public AllStatic { PhaseIdealLoop* phase, int flags); static void call_lrb_stub(Node*& ctrl, Node*& val, Node* load_addr, DecoratorSet decorators, PhaseIdealLoop* phase); + + static void collect_nodes_above_barrier(Unique_Node_List &nodes_above_barrier, PhaseIdealLoop* phase, Node* ctrl, + Node* init_raw_mem); + static void test_in_cset(Node*& ctrl, Node*& not_cset_ctrl, Node* val, Node* raw_mem, PhaseIdealLoop* phase); static void move_gc_state_test_out_of_loop(IfNode* iff, PhaseIdealLoop* phase); static void merge_back_to_back_tests(Node* n, PhaseIdealLoop* phase); static bool merge_point_safe(Node* region); static bool identical_backtoback_ifs(Node *n, PhaseIdealLoop* phase); - static void fix_ctrl(Node* barrier, Node* region, const MemoryGraphFixer& fixer, Unique_Node_List& uses, Unique_Node_List& uses_to_ignore, uint last, PhaseIdealLoop* phase); + static void fix_ctrl(Node* barrier, Node* region, const MemoryGraphFixer& fixer, Unique_Node_List& uses, Unique_Node_List& nodes_above_barrier, uint last, PhaseIdealLoop* phase); static IfNode* find_unswitching_candidate(const IdealLoopTree *loop, PhaseIdealLoop* phase); static Node* get_load_addr(PhaseIdealLoop* phase, VectorSet& visited, Node* lrb); @@ -82,6 +86,11 @@ class ShenandoahBarrierC2Support : public AllStatic { static void pin_and_expand(PhaseIdealLoop* phase); static void optimize_after_expansion(VectorSet& visited, Node_Stack& nstack, Node_List& old_new, PhaseIdealLoop* phase); + static void push_data_inputs_at_control(PhaseIdealLoop* phase, Node* n, Node* ctrl, + Unique_Node_List &wq); + static bool is_anti_dependent_load_at_control(PhaseIdealLoop* phase, Node* maybe_load, Node* store, Node* control); + + static void maybe_push_anti_dependent_loads(PhaseIdealLoop* phase, Node* maybe_store, Node* control, Unique_Node_List &wq); #ifdef ASSERT static void verify(RootNode* root); #endif diff --git a/test/hotspot/jtreg/gc/shenandoah/compiler/TestLostAntiDependencyAtExpansion.java b/test/hotspot/jtreg/gc/shenandoah/compiler/TestLostAntiDependencyAtExpansion.java new file mode 100644 index 00000000000..bcdf964a1fa --- /dev/null +++ b/test/hotspot/jtreg/gc/shenandoah/compiler/TestLostAntiDependencyAtExpansion.java @@ -0,0 +1,67 @@ +/* + * Copyright (c) 2025, Red Hat, Inc. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/** + * @test + * @bug 8358334 + * @summary C2/Shenandoah: incorrect execution with Unsafe + * @requires vm.gc.Shenandoah + * @modules java.base/jdk.internal.misc:+open + * + * @run main/othervm -XX:-UseOnStackReplacement -XX:-BackgroundCompilation -XX:-TieredCompilation -XX:+UseShenandoahGC + * TestLostAntiDependencyAtExpansion + * + * + */ + +import jdk.internal.misc.Unsafe; + +public class TestLostAntiDependencyAtExpansion { + static final jdk.internal.misc.Unsafe UNSAFE = Unsafe.getUnsafe(); + + public static void main(String[] args) { + long addr = UNSAFE.allocateMemory(8); + for (int i = 0; i < 20_000; i++) { + UNSAFE.putLong(addr, 42L); + long res = test1(addr); + if (res != 42L) { + throw new RuntimeException("Incorrect result: " + res); + } + } + } + + static class A { + long field; + } + + static A a = new A(); + + private static long test1(long addr) { + long tmp = UNSAFE.getLong(addr); + + UNSAFE.putLong(addr, 0L); + + return tmp + a.field; + } + +} From 63c627cf09131b9234c35795a74cc4b6a6f393a5 Mon Sep 17 00:00:00 2001 From: rwestrel Date: Mon, 7 Jul 2025 10:02:42 +0200 Subject: [PATCH 2/2] review --- .../gc/shenandoah/c2/shenandoahSupport.cpp | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp b/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp index f2b20e9606f..8939279fd25 100644 --- a/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp +++ b/src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp @@ -661,7 +661,7 @@ void ShenandoahBarrierC2Support::maybe_push_anti_dependent_loads(PhaseIdealLoop* 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 && phase->has_ctrl(in) && phase->get_ctrl(in) == ctrl) { + if (in != nullptr && (ShenandoahIUBarrier ? (phase->ctrl_or_self(in) == ctrl) : (phase->has_ctrl(in) && phase->get_ctrl(in) == ctrl))) { wq.push(in); } } @@ -681,22 +681,9 @@ bool ShenandoahBarrierC2Support::is_dominator_same_ctrl(Node* c, Node* d, Node* if (m->is_Phi() && m->in(0)->is_Loop()) { assert(phase->ctrl_or_self(m->in(LoopNode::EntryControl)) != c, "following loop entry should lead to new control"); } else { - if (m->is_Store() || m->is_LoadStore()) { - // Take anti-dependencies into account - Node* mem = m->in(MemNode::Memory); - for (DUIterator_Fast imax, i = mem->fast_outs(imax); i < imax; i++) { - Node* u = mem->fast_out(i); - if (u->is_Load() && phase->C->can_alias(m->adr_type(), phase->C->get_alias_index(u->adr_type())) && - phase->ctrl_or_self(u) == c) { - wq.push(u); - } - } - } - for (uint i = 0; i < m->req(); i++) { - if (m->in(i) != nullptr && phase->ctrl_or_self(m->in(i)) == c) { - wq.push(m->in(i)); - } - } + // Take anti-dependencies into account + maybe_push_anti_dependent_loads(phase, m, c, wq); + push_data_inputs_at_control(phase, m, c, wq); } } return true;