RFR: 8320308: C2 compilation crashes in LibraryCallKit::inline_unsafe_access

[Tobias Holenstein]

On Thu, 4 Jul 2024 12:17:51 GMT, Tobias Holenstein <tholenstein at openjdk.org> wrote:

> We failed in `LibraryCallKit::inline_unsafe_access()` while trying to inline `Unsafe::getShortUnaligned`. 
> https://github.com/openjdk/jdk/blob/34c6e0deac567c0f4ed08aa2824671551d843e95/test/hotspot/jtreg/compiler/parsing/TestUnsafeArrayAccessWithNullBase.java#L86
> The reason is that base (the array) is `ConP #null` hidden behind two `CheckCastPP` with `speculative=byte[int:>=0]`
> 
> We call `Node* adr = make_unsafe_address(base, offset, type, kind == Relaxed);`
> https://github.com/openjdk/jdk/blob/34c6e0deac567c0f4ed08aa2824671551d843e95/src/hotspot/share/opto/library_call.cpp#L2361
> - with **base** = `147  CheckCastPP` 
> -  `118  ConP  === 0  [[[ 106 101 71 ] #null`
> ![fail](https://github.com/openjdk/jdk/assets/71546117/180e61e7-baa3-4274-842e-5ae94c4d73b6)
> 
> Depending on the **offset** we go two different paths in `LibraryCallKit::make_unsafe_address` which both lead to the same error in the end. 
> 1. For `UNSAFE.getShortUnaligned(array, 1_049_000)` we get kind = `Type::AnyPtr` because `offset >= os::vm_page_size()`.  Since we assume base can't be null we insert an assert:
> https://github.com/openjdk/jdk/blob/34c6e0deac567c0f4ed08aa2824671551d843e95/src/hotspot/share/opto/library_call.cpp#L2111
> 
> 2. whereas for `UNSAFE.getShortUnaligned(array, 1)` we get kind = `Type:: OopPtr`
> https://github.com/openjdk/jdk/blob/c17fa910cf3bad48547a3f0d68a30795ec3194e6/src/hotspot/share/opto/library_call.cpp#L2078
> and insert a null check
> https://github.com/openjdk/jdk/blob/34c6e0deac567c0f4ed08aa2824671551d843e95/src/hotspot/share/opto/library_call.cpp#L2090
> In both cases we return call `basic_plus_adr(..)` on a base being `top()` which returns **adr** =  `1  Con  === 0  [[ ]]  #top`
> 
> https://github.com/openjdk/jdk/blob/3d5d51e228c19aa216451f647023101ae8bdbc79/src/hotspot/share/opto/library_call.cpp#L2386 => `_gvn.type(adr)` is _top_
> 
> https://github.com/openjdk/jdk/blob/3d5d51e228c19aa216451f647023101ae8bdbc79/src/hotspot/share/opto/library_call.cpp#L2394 => `adr_type` is _nullptr_
> 
> https://github.com/openjdk/jdk/blob/3d5d51e228c19aa216451f647023101ae8bdbc79/src/hotspot/share/opto/library_call.cpp#L2405-L2406 => `BasicType bt` is  _T_ILLEGAL_
> 
> https://github.com/openjdk/jdk/blob/3d5d51e228c19aa216451f647023101ae8bdbc79/src/hotspot/share/opto/library_call.cpp#L2424 => we fail here with `SIGSEGV: null pointer dereference` because `alias_type->adr_type()` is _nullptr_
> 
> ### Fix
> the fix is to bailed out in this case
> https://github.com/openjdk/jdk/blob/3d5d51e228c19aa216451f647023101ae8bd...

> Even though the proposed check in `LibraryCallKit::inline_unsafe_access()` fixes the crash, IMO the root problem is in `LibraryCallKit::classify_unsafe_addr()` where `base_type == TypePtr::NULL_PTR` doesn't hold in presence of speculative part and the base is erroneously classified as on-heap (`Type::OopPtr`).
> 
> I'd prefer to see both places fixed. Seeing `make_unsafe_address` producing dead code signals about a bug. So, asserting that it never happens looks like a good idea.
> 
> Moreover, there are many places in the code susceptible to the same problem where a type is compared with `TypePtr::NULL_PTR`.
> 
> Even though the proposed check in `LibraryCallKit::inline_unsafe_access()` fixes the crash, IMO the root problem is in `LibraryCallKit::classify_unsafe_addr()` where `base_type == TypePtr::NULL_PTR` doesn't hold in presence of speculative part and the base is erroneously classified as on-heap (`Type::OopPtr`).
> 
> I'd prefer to see both places fixed. Seeing `make_unsafe_address` producing dead code signals about a bug. So, asserting that it never happens looks like a good idea.


Yes, it I agree to also fix `LibraryCallKit::inline_unsafe_access()`

Instead of:
https://github.com/openjdk/jdk/blob/34c6e0deac567c0f4ed08aa2824671551d843e95/src/hotspot/share/opto/library_call.cpp#L2044
we could do
```c++
} else if (_gvn.type(base->uncast()) == TypePtr::NULL_PTR) {

in which case we would see the `ConP #null` hidden behind the two `CheckCastPP`. Do you agree that this would fix `LibraryCallKit::inline_unsafe_access()`? 


> Moreover, there are many places in the code susceptible to the same problem where a type is compared with `TypePtr::NULL_PTR`.

Not sure I understand this correctly. Do you mean in the Hotspot codebase in general or which `TypePtr::NULL_PTR` compares do you mean?

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

PR Comment: https://git.openjdk.org/jdk/pull/20033#issuecomment-2221350121