Integrated: 8303512: Race condition when computing is_loaded property of TypePtr::InterfaceSet

[Tobias Hartmann]

On Mon, 8 May 2023 14:52:21 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> [JDK-8297933](https://bugs.openjdk.org/browse/JDK-8297933) introduced a race condition among compiler threads computing `TypePtr::InterfaceSet::_is_loaded` for shared types (for example, for [TypeInstPtr::NOTNULL](https://github.com/openjdk/jdk/blob/5726d31e56530bbe7dee61ae04b126e20cb3611d/src/hotspot/share/opto/type.cpp#L545)). One thread can set `_is_loaded_computed` before setting `_is_loaded`:
> 
> https://github.com/openjdk/jdk/blob/5726d31e56530bbe7dee61ae04b126e20cb3611d/src/hotspot/share/opto/type.cpp#L3473-L3482
> 
> while another thread can already access `is_loaded()` and wrongly observe `_is_loaded = false`:
> 
> https://github.com/openjdk/jdk/blob/5726d31e56530bbe7dee61ae04b126e20cb3611d/src/hotspot/share/opto/type.cpp#L3464-L3468
> 
> As a result, the klass of a `TypePtr` can be loaded while the interfaces it implements appear to be not loaded.
> 
> Another problem is that `TypePtr::InterfaceSet::eq` does not take the `_is_loaded` / `_is_loaded_computed` fields into account:
> 
> https://github.com/openjdk/jdk/blob/5726d31e56530bbe7dee61ae04b126e20cb3611d/src/hotspot/share/opto/type.cpp#L3290-L3301
> 
> A loaded type can therefore be replaced by an unloaded type during GVN.
> 
> In the case of the failure reported by this bug, `LibraryCallKit::inline_native_hashcode` first null checks the receiver and updates the type. Due to the issues described above, the null-free type is GVN'ed with it's unloaded counterpart and propagated to another, redundant null check emitted by `LibraryCallKit::generate_method_call` (I filed [JDK-8307625](https://bugs.openjdk.org/browse/JDK-8307625) to remove it). Since the type is now unloaded, an uncommon trap is emitted and parsing is stopped(). We crash when trying to de-reference `GraphKit::_map->_jvms` which is NULL (in debug we would hit an assert).
> 
> Another failure mode is reported by [JDK-8305339](https://bugs.openjdk.org/browse/JDK-8305339). Both failures are extremely intermittent and I was never able to reproduce them.
> 
> The fix I propose is to completely remove the `_is_loaded` logic because if a klass is loaded, the interface that it implements should be loaded as well. I added an assert to verify this. Higher tier testing is still running.
> 
> In addition, I noticed some `#ifdef DEBUG` checks where `DEBUG` is never defined. I replaced them by `#ifdef ASSERT` and fixed the verification code that failed to build.
> 
> Thanks,
> Tobias

This pull request has now been integrated.

Changeset: ad348a8c
Author:    Tobias Hartmann <thartmann at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/ad348a8cec50561d3e295b6289772530f541c6b1
Stats:     117 lines in 3 files changed: 38 ins; 46 del; 33 mod

8303512: Race condition when computing is_loaded property of TypePtr::InterfaceSet

Reviewed-by: roland, qamai, kvn

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

PR: https://git.openjdk.org/jdk/pull/13868