Skip to content

8358334: C2/Shenandoah: incorrect execution with Unsafe #3770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+128 −37
Open

8358334: C2/Shenandoah: incorrect execution with Unsafe #3770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 51 additions & 36 deletions src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 && (ShenandoahIUBarrier ? (phase->ctrl_or_self(in) == ctrl) : (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
Expand All @@ -653,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) != NULL && 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;
Expand Down Expand Up @@ -1020,7 +1035,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);

Expand All @@ -1031,30 +1059,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 != NULL && 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);
Expand Down Expand Up @@ -1326,7 +1341,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();
Expand Down Expand Up @@ -1423,7 +1438,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;

Expand Down Expand Up @@ -1579,7 +1594,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");
Expand Down
11 changes: 10 additions & 1 deletion src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down
67 changes: 67 additions & 0 deletions test/hotspot/jtreg/gc/shenandoah/compiler/TestLostAntiDependencyAtExpansion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}

}