RFR: 8336906: C2: assert(bb->is_reachable()) failed: getting result from unreachable basicblock [v3]

[Dean Long]

On Mon, 12 May 2025 09:25:43 GMT, Manuel Hässig <mhaessig at openjdk.org> wrote:

>> # Issue Summary
>> 
>> This PR addresses an `assert(bb->is_reachable())` that is triggered in the code for `-XX:+VerifyStack` after a deoptimization with reason `null_assert_or_unreached0` at a `getstatic` bytecode. Following the `getstatic` is an `areturn` and then an unreachable bytecode. When the code for `VerifyStack` tries to compute an oop map for the basic block of the unreachable bytecode, the assert triggers:
>> 
>> getstatic Field A.val:"LB"; // if class B is not loaded, C2 deopts with reason "null_assert_or_unreached0"
>> areturn;
>> // The following is unreachable
>> iconst_0;
>> 
>> 
>> This is a similar problem to [JDK-8271055](https://bugs.openjdk.org/browse/JDK-8271055) (#7331), but this particular deopt with reason `null_assert_or_unreached0` at `getstatic` of a field containing an object reference [deopts at the next bytecode](https://github.com/openjdk/jdk/blob/ad07426fab3396caefd7c08d924e085c1f6f61ba/src/hotspot/share/opto/parse3.cpp#L176-L199). The aforementioned issue introduced a check to skip stack verification of the next bytecode in the code if the execution after the deopted bytecode does not continue at the next bytecode in the code, i.e. falls through to the next bytecode.  Unfortunately, this check did not include `areturn` as a bytecode that does not fall-through:
>> https://github.com/openjdk/jdk/blob/ad07426fab3396caefd7c08d924e085c1f6f61ba/src/hotspot/share/runtime/deoptimization.cpp#L845-L856
>> 
>> # Change Summary
>> 
>> To fix the immediate issue described above, this PR adds `areturn` to the list of bytecodes that does not fall through. However, all return bytecodes exhibit the same behavior and might be susceptible to a similar issue. Even though I was not able to reproduce the same crash with `{d,f,i,l}return` because I could not get those or the preceding bytecode to deopt, I also added them to the `falls_through()` function. For the remaining bytecodes in `falls_through()` with the exception of `athrow` I wrote a regression test.
>> 
>> # Testing
>> 
>> - [x] [Github Actions](https://github.com/mhaessig/jdk/actions/runs/14595928439)
>> - [x] tier1 through tier3 on Oracle supported platforms and OSs plus Oracle internal testing
>> 
>> # Acknowledgements
>> Special thanks to @eme64 for his hard work on reducing a reproducer that works on all platforms.
>
> Manuel Hässig has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add jsr to falls_through()

What makes the debug info "before" or "after" is the state of the stack relative to the bci.  So if we are doing a getfield, and have already popped the object off the stack and pushed the result, then the state would be "after".  However, we then advance the bci, making the state "before", because we are now on a different bytecode.  Since uncommon traps have reexecute semantics, the interpreter will continue executing at that bci without advancing it first, so we need a "before" state in that case.  That's why trying to compute an "after" state doesn't make sense to me for uncommon traps.  They should all use a "before" state. 
In other deoptimization cases without reexecute semantics, we may cause the interpreter to advance to the bci that follows the one given in the debug info.  In this situation, the debug info would need to be the "after" state, and bci in the debug info would need to be one that falls through.
I plan to take a closer look at this VerifyStack code and possibly improve or remove it, but not as part of this PR.

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

PR Comment: https://git.openjdk.org/jdk/pull/25118#issuecomment-2873653422