From 2bc4f64c56ebc844d494a4ce8ba72a25643d4075 Mon Sep 17 00:00:00 2001 From: Marc Chevalier Date: Thu, 20 Mar 2025 15:50:23 +0000 Subject: [PATCH] 8335708: C2: Compile::verify_graph_edges must start at root and safepoints, just like CCP traversal Reviewed-by: chagedorn, epeter --- src/hotspot/share/opto/compile.cpp | 28 ++++++-- src/hotspot/share/opto/compile.hpp | 28 ++++++-- src/hotspot/share/opto/node.hpp | 2 +- src/hotspot/share/opto/phaseX.cpp | 2 +- ...hEdgesWithDeadCodeCheckFromSafepoints.java | 70 +++++++++++++++++++ 5 files changed, 114 insertions(+), 16 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/loopopts/VerifyGraphEdgesWithDeadCodeCheckFromSafepoints.java diff --git a/src/hotspot/share/opto/compile.cpp b/src/hotspot/share/opto/compile.cpp index 5294315fe74..56f57793fbb 100644 --- a/src/hotspot/share/opto/compile.cpp +++ b/src/hotspot/share/opto/compile.cpp @@ -421,7 +421,7 @@ void Compile::remove_useless_node(Node* dead) { } // Disconnect all useless nodes by disconnecting those at the boundary. -void Compile::disconnect_useless_nodes(Unique_Node_List& useful, Unique_Node_List& worklist) { +void Compile::disconnect_useless_nodes(Unique_Node_List& useful, Unique_Node_List& worklist, const Unique_Node_List* root_and_safepoints) { uint next = 0; while (next < useful.size()) { Node *n = useful.at(next++); @@ -474,7 +474,7 @@ void Compile::disconnect_useless_nodes(Unique_Node_List& useful, Unique_Node_Lis remove_useless_late_inlines( &_string_late_inlines, useful); remove_useless_late_inlines( &_boxing_late_inlines, useful); remove_useless_late_inlines(&_vector_reboxing_late_inlines, useful); - debug_only(verify_graph_edges(true/*check for no_dead_code*/);) + debug_only(verify_graph_edges(true /*check for no_dead_code*/, root_and_safepoints);) } // ============================================================================ @@ -4253,11 +4253,25 @@ bool Compile::needs_clinit_barrier(ciInstanceKlass* holder, ciMethod* accessing_ //------------------------------verify_bidirectional_edges--------------------- // For each input edge to a node (ie - for each Use-Def edge), verify that // there is a corresponding Def-Use edge. -void Compile::verify_bidirectional_edges(Unique_Node_List &visited) { +void Compile::verify_bidirectional_edges(Unique_Node_List& visited, const Unique_Node_List* root_and_safepoints) const { // Allocate stack of size C->live_nodes()/16 to avoid frequent realloc uint stack_size = live_nodes() >> 4; - Node_List nstack(MAX2(stack_size, (uint)OptoNodeListSize)); - nstack.push(_root); + Node_List nstack(MAX2(stack_size, (uint) OptoNodeListSize)); + if (root_and_safepoints != nullptr) { + assert(root_and_safepoints->member(_root), "root is not in root_and_safepoints"); + for (uint i = 0, limit = root_and_safepoints->size(); i < limit; i++) { + Node* root_or_safepoint = root_and_safepoints->at(i); + // If the node is a safepoint, let's check if it still has a control input + // Lack of control input signifies that this node was killed by CCP or + // recursively by remove_globally_dead_node and it shouldn't be a starting + // point. + if (!root_or_safepoint->is_SafePoint() || root_or_safepoint->in(0) != nullptr) { + nstack.push(root_or_safepoint); + } + } + } else { + nstack.push(_root); + } while (nstack.size() > 0) { Node* n = nstack.pop(); @@ -4307,12 +4321,12 @@ void Compile::verify_bidirectional_edges(Unique_Node_List &visited) { //------------------------------verify_graph_edges--------------------------- // Walk the Graph and verify that there is a one-to-one correspondence // between Use-Def edges and Def-Use edges in the graph. -void Compile::verify_graph_edges(bool no_dead_code) { +void Compile::verify_graph_edges(bool no_dead_code, const Unique_Node_List* root_and_safepoints) const { if (VerifyGraphEdges) { Unique_Node_List visited; // Call graph walk to check edges - verify_bidirectional_edges(visited); + verify_bidirectional_edges(visited, root_and_safepoints); if (no_dead_code) { // Now make sure that no visited node is used by an unvisited node. bool dead_nodes = false; diff --git a/src/hotspot/share/opto/compile.hpp b/src/hotspot/share/opto/compile.hpp index 4c97b26bb25..bd998194ff7 100644 --- a/src/hotspot/share/opto/compile.hpp +++ b/src/hotspot/share/opto/compile.hpp @@ -1035,7 +1035,7 @@ public: void identify_useful_nodes(Unique_Node_List &useful); void update_dead_node_list(Unique_Node_List &useful); - void disconnect_useless_nodes(Unique_Node_List& useful, Unique_Node_List& worklist); + void disconnect_useless_nodes(Unique_Node_List& useful, Unique_Node_List& worklist, const Unique_Node_List* root_and_safepoints = nullptr); void remove_useless_node(Node* dead); @@ -1242,14 +1242,28 @@ public: static void print_intrinsic_statistics() PRODUCT_RETURN; // Graph verification code - // Walk the node list, verifying that there is a one-to-one - // correspondence between Use-Def edges and Def-Use edges - // The option no_dead_code enables stronger checks that the - // graph is strongly connected from root in both directions. - void verify_graph_edges(bool no_dead_code = false) PRODUCT_RETURN; + // Walk the node list, verifying that there is a one-to-one correspondence + // between Use-Def edges and Def-Use edges. The option no_dead_code enables + // stronger checks that the graph is strongly connected from starting points + // in both directions. + // root_and_safepoints is used to give the starting points for the traversal. + // If not supplied, only root is used. When this check is called after CCP, + // we need to start traversal from Root and safepoints, just like CCP does its + // own traversal (see PhaseCCP::transform for reasons). + // + // To call this function, there are 2 ways to go: + // - give root_and_safepoints to start traversal everywhere needed (like after CCP) + // - if the whole graph is assumed to be reachable from Root's input, + // root_and_safepoints is not needed (like in PhaseRemoveUseless). + // + // Failure to specify root_and_safepoints in case the graph is not fully + // reachable from Root's input make this check unsound (can miss inconsistencies) + // and even incomplete (can make up non-existing problems) if no_dead_code is + // true. + void verify_graph_edges(bool no_dead_code = false, const Unique_Node_List* root_and_safepoints = nullptr) const PRODUCT_RETURN; // Verify bi-directional correspondence of edges - void verify_bidirectional_edges(Unique_Node_List &visited); + void verify_bidirectional_edges(Unique_Node_List& visited, const Unique_Node_List* root_and_safepoints = nullptr) const; // End-of-run dumps. static void print_statistics() PRODUCT_RETURN; diff --git a/src/hotspot/share/opto/node.hpp b/src/hotspot/share/opto/node.hpp index 32478961ba1..3efe4ca0825 100644 --- a/src/hotspot/share/opto/node.hpp +++ b/src/hotspot/share/opto/node.hpp @@ -1745,7 +1745,7 @@ public: Unique_Node_List(Unique_Node_List&&) = default; void remove( Node *n ); - bool member( Node *n ) { return _in_worklist.test(n->_idx) != 0; } + bool member(const Node* n) const { return _in_worklist.test(n->_idx) != 0; } VectorSet& member_set(){ return _in_worklist; } void push(Node* b) { diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index 397b1e52e93..0369143c7b0 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -2110,7 +2110,7 @@ Node *PhaseCCP::transform( Node *n ) { C->update_dead_node_list(useful); remove_useless_nodes(useful.member_set()); _worklist.remove_useless_nodes(useful.member_set()); - C->disconnect_useless_nodes(useful, _worklist); + C->disconnect_useless_nodes(useful, _worklist, &_root_and_safepoints); Node* new_root = node_map[n->_idx]; assert(new_root->is_Root(), "transformed root node must be a root node"); diff --git a/test/hotspot/jtreg/compiler/loopopts/VerifyGraphEdgesWithDeadCodeCheckFromSafepoints.java b/test/hotspot/jtreg/compiler/loopopts/VerifyGraphEdgesWithDeadCodeCheckFromSafepoints.java new file mode 100644 index 00000000000..c731ae6f770 --- /dev/null +++ b/test/hotspot/jtreg/compiler/loopopts/VerifyGraphEdgesWithDeadCodeCheckFromSafepoints.java @@ -0,0 +1,70 @@ +/* + * Copyright (c) 2025, Oracle and/or its affiliates. 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 8335708 + * @summary Crash Compile::verify_graph_edges with dead code check when safepoints are reachable but not connected back to Root's inputs + * @library /test/lib + * + * @run driver compiler.loopopts.VerifyGraphEdgesWithDeadCodeCheckFromSafepoints + * @run main/othervm + * -XX:+IgnoreUnrecognizedVMOptions + * -XX:-TieredCompilation -XX:+VerifyGraphEdges + * -XX:+StressIGVN -Xcomp + * -XX:CompileCommand=compileonly,compiler.loopopts.VerifyGraphEdgesWithDeadCodeCheckFromSafepoints::mainTest + * compiler.loopopts.VerifyGraphEdgesWithDeadCodeCheckFromSafepoints + * + */ + +package compiler.loopopts; + +import jdk.test.lib.Utils; + +public class VerifyGraphEdgesWithDeadCodeCheckFromSafepoints { + public static void main(String[] args) throws Exception { + Thread thread = new Thread() { + public void run() { + VerifyGraphEdgesWithDeadCodeCheckFromSafepoints instance = new VerifyGraphEdgesWithDeadCodeCheckFromSafepoints(); + byte[] a = new byte[997]; + for (int i = 0; i < 100; ++i) { + instance.mainTest(a, a); + } + } + }; + // Give thread some time to trigger compilation + thread.setDaemon(true); + thread.start(); + Thread.sleep(Utils.adjustTimeout(500)); + } + + public void mainTest(byte[] a, byte[] b) { + int i = 0; + while (i < (a.length - 4)) { + a[i] = b[i]; + } + while (true) { + a[i] = b[i]; + } + } +}