RFR: 8287432: C2: assert(tn->in(0) != __null) failed: must have live top node [v2]

[Devin Smith]

On Tue, 7 Jun 2022 16:56:15 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> When intrisifying `java.lang.Thread::currentThread()`, we are creating an `AddP` node that has the `top` node as base to indicate that we do not have an oop (using `NULL` instead leads to crashes as it does not seem to be expected to have a `NULL` base):
>> https://github.com/openjdk/jdk/blob/6ff2d89ea11934bb13c8a419e7bad4fd40f76759/src/hotspot/share/opto/library_call.cpp#L904
>> 
>> This node is used on a chain of data nodes into two `MemBarAcquire` nodes as precedence edge in the test case:
>> ![Screenshot from 2022-06-07 11-12-38](https://user-images.githubusercontent.com/17833009/172344751-5338b72f-baa5-4e9e-a44c-6d970798d9f2.png)
>> 
>> Later, in `final_graph_reshaping_impl()`, we are removing the precedence edge of both `MemBarAcquire` nodes and clean up all now dead nodes as a result of the removal:
>> https://github.com/openjdk/jdk/blob/6ff2d89ea11934bb13c8a419e7bad4fd40f76759/src/hotspot/share/opto/compile.cpp#L3655-L3679
>> 
>> We iteratively call `disconnect_inputs()` for all nodes that have no output anymore (i.e. dead nodes). This code, however, also treats the `top` node as dead since `outcnt()` of `top` is always zero:
>> https://github.com/openjdk/jdk/blob/6ff2d89ea11934bb13c8a419e7bad4fd40f76759/src/hotspot/share/opto/node.hpp#L495-L500
>> 
>> And we end up disconnecting `top` which results in the assertion failure.
>> 
>> The code misses a check for `top()`. I suggest to add this check before processing a node for which `outcnt()` is zero. This is a pattern which can also be found in other places in the code. I've checked all other usages of `oucnt() == 0` and could not find a case where this additional `top()` check is missing. Maybe we should refactor these two checks into a single method at some point to not need to worry about `top` anymore in the future when checking if a node is dead based on the outputs.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update test/hotspot/jtreg/compiler/c2/TestRemoveMemBarPrecEdge.java
>   
>   Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>

I'm not qualified to review this, but I can confirm the test is triggering the `PhaseAggressiveCoalesce::coalesce` SIGSEGV against the 11/17 LTS versions I'm running. Thanks for your investigation and fix!

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

PR: https://git.openjdk.java.net/jdk/pull/9060