RFR: 8335708: C2: assert(!dead_nodes) failed: using nodes must be reachable from root

[Emanuel Peter]

On Tue, 11 Mar 2025 09:12:44 GMT, Marc Chevalier <duke at openjdk.org> wrote:

> In CCP, we transform the nodes going up (toward inputs) starting from root and safepoints because infinite loops can be reachable from the root, but not co-reachable from the root, that is one can follow def-use from root to the loop, but not the use-def from root to loop. For more details, see:
> https://github.com/openjdk/jdk/blob/4cf63160ad575d49dbe70f128cd36aba22b8f2ff/src/hotspot/share/opto/phaseX.cpp#L2063-L2070
> 
> Since we are specifically marking nodes as useful if they are above a safepoint, the check that no dead nodes must be there anymore must also consider nodes above a safepoint as alive: the same criterion must apply. We should nevertheless not start from a safepoint killed by CCP.
> 
> About the test, I use this trick found in `TestInfiniteLoopCCP` because I indeed need a really infinite loop, but I want a terminating test. The crash is not deterministic, as it needs StressIGVN, so I did a bit of stats. Using a little helper script, on 100 runs, 69 runs fail as in the JBS ticket and 31 are successful (so 0 fail in another way). After the fix, I find 100 successes.
> 
> And thanks to @eme64  who extracted such a concise reproducer.

Nice work, and thanks again for the offline discussion :)

I left a few comments / suggestions below.
Ah, and you could also consider changing the PR name. Maybe something like `graph verification must start at root and safepoints, just like CCP traversal`. Maybe you have an even better idea ;)

src/hotspot/share/opto/compile.cpp line 4206:

> 4204:   uint stack_size = live_nodes() >> 4;
> 4205:   Node_List nstack(MAX2(stack_size, (uint) OptoNodeListSize));
> 4206:   if (root_and_safepoints != nullptr) {

Can you say in which cases we don't have `root_and_safepoints`? Why is it ok not to also start at SafePoint in those cases?

test/hotspot/jtreg/compiler/loopopts/Test8335708.java line 1:

> 1: /*

Can you rename the test file? I think the new common practice is to give it a descriptive name, rather than just the bug number which is already tracked under `@bug 8335708` anyway ;)

test/hotspot/jtreg/compiler/loopopts/Test8335708.java line 28:

> 26:  * @bug 8335708
> 27:  * @summary Crash Compile::verify_graph_edges
> 28:  * @requires vm.debug == true & vm.flavor == "server"

Can we find a way not to have this restriction? It could make sense to still execute this in product, or with other compilers.

If the issues is with vm flags, then you can always use `-XX:+IgnoreUnrecognizedVMOptions`.

test/hotspot/jtreg/compiler/loopopts/Test8335708.java line 29:

> 27:  * @summary Crash Compile::verify_graph_edges
> 28:  * @requires vm.debug == true & vm.flavor == "server"
> 29:  * @library /test/lib

Suggestion:


Can you test if you actually need this line?

test/hotspot/jtreg/compiler/loopopts/Test8335708.java line 35:

> 33:  *       -XX:+StressIGVN -Xcomp
> 34:  *       -XX:CompileCommand=compileonly,compiler.loopopts.Test8335708::mainTest
> 35:  *       compiler.loopopts.Test8335708

Can you please add a run without any flags? Sometimes that allows other bugs to trigger, because it can then be used without any flags, or other flag combinations.

-------------

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23977#pullrequestreview-2689275942
PR Review Comment: https://git.openjdk.org/jdk/pull/23977#discussion_r1998041679
PR Review Comment: https://git.openjdk.org/jdk/pull/23977#discussion_r1998036529
PR Review Comment: https://git.openjdk.org/jdk/pull/23977#discussion_r1998034747
PR Review Comment: https://git.openjdk.org/jdk/pull/23977#discussion_r1998038170
PR Review Comment: https://git.openjdk.org/jdk/pull/23977#discussion_r1998035486