RFR: 8370502: C2: segfault while adding node to IGVN worklist [v3]

[Kerem Kat]

On Thu, 20 Nov 2025 16:30:10 GMT, Manuel Hässig <mhaessig at openjdk.org> wrote:

>> Kerem Kat has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
>> 
>>  - rename test file
>>  - Merge branch 'master' into fix-c2-segfault-unlocknode
>>  - fix test spacing
>>  - Update src/hotspot/share/opto/macro.cpp
>>    
>>    Co-authored-by: Manuel Hässig <manuel at haessig.org>
>>  - Update src/hotspot/share/opto/macro.cpp
>>    
>>    Co-authored-by: Manuel Hässig <manuel at haessig.org>
>>  - copyright format fix?
>>  - 8370502: C2: segfault while adding node to IGVN worklist
>
> Thank you for working on this, @krk. And nice job reducing the test further!
> 
> I have a few questions and style comments below.

Thank you for the comments. To answer three remaining comments from @mhaessig, @dean-long and @TobiHartmann about the same subject:

`CallNode::extract_projections` sets `fallthrough_memproj` to `null` when the node has `outcnt == 0`. It does not assert on this because `do_asserts = false` is passed.

Regarding `mem` vs. `fallthrough_memproj`, replacing `fallthrough_memproj` with `mem` appears to change the semantics from memory outputs to memory inputs.

Nothing after unlock (exit from the synchronized block) uses memory, as `a[i] =0` would throw (`a` is a zero-length array), making the rest of the `test` method unreachable.

This raises the question: why is `synchronized` reachable at all, as it is also after `a[i] = 0`.

About the invariant, I have found mixed null checks of `fallthrough_memproj`:


## Analysis of all reads of `fallthrough_memproj`

### Does null-check

* `PhaseMacroExpand::expand_unlock_node` checks now with the current PR.
* `ArrayCopyNode::finish_transform`
* `ShenandoahBarrierC2Support::find_bottom_mem`
* `GraphKit::replace_call`
* `PhaseMacroExpand::expand_allocate_common`
* `PhaseMacroExpand::yank_alloc_node`
* `PhaseMacroExpand::eliminate_locking_node`
* `StringConcat::eliminate_call`

#### (checks equality with a non-null `Node*`)

* `MemoryGraphFixer::get_ctrl`
* `CallGenerator::do_late_inline_helper`

### No check, but `do_asserts = true`

* `PhaseMacroExpand::process_users_of_allocation` asserts for `callprojs`, checks for null for `_callprojs`.

### No check

* `PhaseMacroExpand::expand_lock_node`
* `PhaseMacroExpand::generate_arraycopy`
* `PhaseMacroExpand::generate_slow_arraycopy`
* `PhaseMacroExpand::expand_arraycopy_node`

## To summarize

1. If `do_asserts = false` is passed to `CallNode::extract_projections`, it means `fallthrough_memproj` (and some others) may be `null`. After this, `fallthrough_memproj` should be null-checked. Currently, only code that needs null-checking is in `PhaseMacroExpand` class.
2. Why is `synchronized` reachable in the repro at all, as it is also after `a[i] = 0`.

Based on this analysis, I propose creating new issues for these two points and keep the current PR focused on `PhaseMacroExpand::expand_unlock_node`.

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

PR Comment: https://git.openjdk.org/jdk/pull/28432#issuecomment-3586912195