RFR: 8362832: compiler/macronodes/TestTopInMacroElimination.java hits assert(false) failed: unexpected node

[Daniel Lundén]

On Mon, 20 Oct 2025 15:53:59 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

> This PR prevents hitting an assert caused by encountering `top` while following the memory
> slice associated with a field when eliminating allocations in macro node elimination. This situation
> is the result of another elimination (boxing node elimination) that happened at the same
> macro expansion iteration.
> 
> ### Analysis
> 
> The issue appears in the macro expansion phase. We have a nested `synchronized` block,
> with the outer block synchronizing on `new A()` and the inner one on `TestTopInMacroElimination.class`.
> In the inner `synchronized` block we have an `Integer.valueOf` call in a loop.
> 
> In `PhaseMacroExpand::eliminate_boxing_node` we are getting rid of the `Integer.valueOf`
> call, as it is a non-escaping boxing node. After having eliminated the call,
> `PhaseMacroExpand::process_users_of_allocation` takes care of the users of the removed node.
> There, we replace usages of the fallthrough memory projection with `top`.
> 
> In the same macro expansion iteration, we later attempt to get rid of the `new A()` allocation
> in `PhaseMacroExpand::create_scalarized_object_description`. There, we have to make
> sure that all safepoints can still see the object fields as if the allocation was never deleted.
> For this, we attempt to find the last value on the slice of each specific field (`a`
> in this case). Because field `a` is never written to, and it is not explicitely initialized,
> there is no `Store` associated to it and not even a dedicated memory slice (we end up
> taking the `Bot` input on `MergeMem` nodes). By going up the memory chain, we eventually
> encounter the `MemBarReleaseLock` whose input was set to `top`. This is where the assert
> is hit.
> 
> ### Proposed Fix
> 
> In the end I opted for an analog fix as the similar [JDK-8325030](https://git.openjdk.org/jdk/pull/23104).
> If we get `top` from `scan_mem_chain` in `PhaseMacroExpand::value_from_mem`, then we can safely
> return `top` as well. This means that the safepoint will have `top` as data input, but this will
> eventually cleaned up by the next round of IGVN.
> 
> Another (tempting) option would have been to simply return `nullptr` from `PhaseMacroExpand::value_from_mem` when encoutering `top`. However this would result in bailing
> out from eliminating this allocation temporarily and effectively delaying it to a subsqequent
> macro expansion round.
> 
> 
> ### Testing
> - [x] [GitHub Actions](https://github.com/benoitmaillard/jdk/actions?query=branch%3AJDK-8362832)
> - [x] tier1-4, plus some internal testing
> 
> Thank you for reviewing!

Thanks for the fix @benoitmaillard!

> This means that the safepoint will have top as data input, but this will
eventually cleaned up by the next round of IGVN.

Is it valid for safepoints to even temporarily have top as data input? Even if this gets cleaned up eventually by IGVN, it seems potentially risky to have it in this state.

src/hotspot/share/opto/macro.cpp line 506:

> 504:     } else if (mem->is_top()) {
> 505:       // slice is on a dead path, returning top prevents bailing out
> 506:       // from the elimination, and we let IGVN clean up later

Suggestion:

      // The slice is on a dead path. Returning top prevents bailing out
      // from the elimination, and IGVN can later clean up.

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

PR Review: https://git.openjdk.org/jdk/pull/27903#pullrequestreview-3369288457
PR Review Comment: https://git.openjdk.org/jdk/pull/27903#discussion_r2454676696