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

[Emanuel Peter]

On Thu, 8 May 2025 13:22:55 GMT, Manuel Hässig <duke 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)
> - [ ] 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.

@mhaessig Thanks for looking into this!

A few comments:

1)
Did you go through all bytecodes we support here? I quickly scanned the wiki page, and found the (depricated but still present) `jsr` opcodes that also are essencially a goto, so do not fall-through, I think. At least it seems to me we could also have unreachable code after a `jsr`, right? And then there is a `ret` bytecode that does the symmetrical thing. Not sure if we even handle these in such a way that the bug could be reproduced, but worth a check!

Can you go over all bytecodes, and make sure we are not missing any? Because this is already the second bug of this kind, would be good to fix it once and for all now ;)

2)
> 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.

Hmm ok, I see. Might be worth investing just a little more time to see if we cannot get that done. Or else argue why it CANNOT be done. But then we might as well put an assert inside `falls_through` for those cases, to check that we actually never deopt like that at such an opcode, and revisit that assumption if we ever hit the assert. What do you think?

3)
For the test: It's a bit of a shame to have lots of separate files. Especially because the directory name `test/hotspot/jtreg/compiler/interpreter/verifyStack/` may at some point have more tests, and then it gets a little confusing. I wonder if `A` and `B` could be nested classes in the java file? And the java and jasm file could be named very similarly, so that it is directly clear that they belong together when browsing the test files. An alternative: create a subdirectory that has a very unique name, so that we could separate things that way.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25118#pullrequestreview-2825371630