### Mismatching Stack Sizes During Late Inlining The re-enabled assert and some tests in `TestNullableInlineTypes` fail with a stack mismatch assert when trying to combine exception states when replacing the non-static `CallStaticJava` node in `GraphKit::replace_call()` for a just late inlined call with the actual result . In `test84()`, for example, we have one exception state with the JVM expression stack pointer (later referred to as just "stack pointer") set to 1 and the other set to 0. This means that we throw from two different paths with unequal stack sizes which is a problem.
### Reason for Stack Size Mismatch At the time of the assertion failure, we compare two states from two exception safepoints: 1. `builtin_throw()` from a null check from an ordinary field access in the callee (currently being late inlined): https://github.com/openjdk/valhalla/blob/4c23f0ed5d6a89cd1e7ab8550b367bca296a9f30/src/hotspot/share/opto/graphKit.cpp#L1543-L1544 We clear the stack due to not having an exception handler: https://github.com/openjdk/valhalla/blob/931493bdedd35731569225e1a5c644fbeeea6372/src/hotspot/share/opto/graphKit.cpp#L614-L619 2. `builtin_throw()` from the receiver null check before the non-static call to the callee (i.e. still in the caller). We have an exception handler and thus do not clear the stack as for `1.`. We have all the arguments on the stack: https://github.com/openjdk/valhalla/blob/4c23f0ed5d6a89cd1e7ab8550b367bca296a9f30/src/hotspot/share/opto/graphKit.hpp#L692-L694 This explains the different stack sizes. When we have an exception handler in the callee, we would already handle the exception states in `Parse::do_exceptions()` and do not need to combine them in the caller after the callee was late inlined. ### Why Is this not Causing more Problems? This seems to be quite a common case to have different stack size, so why don't we hit more problems? First, we notice that when we normally add a new exception state, we combine states only when stack sizes are equal: https://github.com/openjdk/valhalla/blob/931493bdedd35731569225e1a5c644fbeeea6372/src/hotspot/share/opto/graphKit.cpp#L304-L308 We only combine exception states with different stack sizes when calling `combine_exception_states()` from `combine_and_pop_all_exception_states()`. This happens at two places: - `Compile::rethrow_exceptions()`: When we still have exceptions states when calling this method, it means that there is no exception handler in the current method. Therefore, the stack pointer should have been cleared as in `1.` above. - `GraphKit::replace_call()`: We just late inlined a call from which there are exception states to be combined (i.e. the callee had no handler). The stack pointer should be cleared as in `1.` and set to zero. `rethrow_exceptions()` was already called at this point. As a result, there are no exceptions left in the caller to handle and we only combine the callee related exception states. Since we already handle the callee exceptions with `do_exceptions()` when having a handler or reset the stack pointer to zero otherwise, it looks like we should be fine. But there is a problem with the receiver null check just before the late inlined call: We have all the arguments on the stack and adjust the stack size accordingly. When now having an additional exception in the callee without an exception handler (i.e. stack size set to zero), we combine a non-zero stack size with a zero stack size and hit the assertion failure. ### Valhalla Related Issue Why aren't we seeing the assertion failure above also in mainline? There were similar issues in the past but they all were fixed (e.g. [JDK-8275638](https://bugs.openjdk.org/browse/JDK-8275638) which added the stack clearing when we don't have any exception handler). But this bug here is specific to value classes: We have a bug in checking the null marker of a value object receiver. In parsing, we already emit a receiver null check for the call to be inlined later. When later late inlining the call, we emit another receiver null check (the one that is causing the assertion failure above) which normally gets folded because we already statically know that the receiver is non-null (we already added a receiver null check in parsing and now know that the receiver is non-null). But that's not always happening for value object receivers. We have this graph: <img width="575" height="274" alt="image" src="https://github.com/user-attachments/assets/2b7e1904-7a47-4dc2-a209-43499b33248e"; /> The receiver `20403 InlineType` is actually `NonNull` (i.e. null marker is one) but we do the following: https://github.com/openjdk/valhalla/blob/703ac967a48926c145cef69945dfc8fc7fc3be01/src/hotspot/share/opto/graphKit.cpp#L1372-L1378 For `InlineType` oop inputs, we walk up the oop chain until we hit a non-inline-type oop input and only then look at the null marker. In the failing case, we walk one `InlineType` up and then have a phi for the null marker and thus wrongly conclude that the receiver `20403 InlineType` is maybe null. We should have directly checked the null marker of the receiver instead of first walking up the oop inputs which is exactly what I propose as a fix in this patch. This lets the receiver null check be folded and avoids the problematic exception state. Note that it is unusual to have another `InlineType` as an oop input. But this is possible with late inlining and returning a value object. Thanks, Christian --------- - [X] I confirm that I make this contribution in accordance with the [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). ------------- Commit messages: - fix Changes: https://git.openjdk.org/valhalla/pull/2486/files Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=2486&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325632 Stats: 10 lines in 2 files changed: 0 ins; 7 del; 3 mod Patch: https://git.openjdk.org/valhalla/pull/2486.diff Fetch: git fetch https://git.openjdk.org/valhalla.git pull/2486/head:pull/2486 PR: https://git.openjdk.org/valhalla/pull/2486
