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

[TobiHartmann]

JDK-8297933 introduced a race condition among compiler threads computing TypePtr::InterfaceSet::_is_loaded for shared types (for example, for TypeInstPtr::NOTNULL). One thread can set _is_loaded_computed before setting _is_loaded:

jdk/src/hotspot/share/opto/type.cpp

Lines 3473 to 3482 in 5726d31

	void TypePtr::InterfaceSet::compute_is_loaded() {
	_is_loaded_computed = 1;
	for (int i = 0; i < _list.length(); i++) {
	ciKlass* interface = _list.at(i);
	if (!interface->is_loaded()) {
	_is_loaded = false;
	return;
	}
	}
	_is_loaded = true;

while another thread can already access is_loaded() and wrongly observe _is_loaded = false:

jdk/src/hotspot/share/opto/type.cpp

Lines 3464 to 3468 in 5726d31

	bool TypePtr::InterfaceSet::is_loaded() const {
	if (_is_loaded_computed) {
	return _is_loaded;
	}
	const_cast<InterfaceSet*>(this)->compute_is_loaded();

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:

jdk/src/hotspot/share/opto/type.cpp

Lines 3290 to 3301 in 5726d31

	bool TypePtr::InterfaceSet::eq(const InterfaceSet& other) const {
	if (_list.length() != other._list.length()) {
	return false;
	}
	for (int i = 0; i < _list.length(); i++) {
	ciKlass* k1 = _list.at(i);
	ciKlass* k2 = other._list.at(i);
	if (!k1->equals(k2)) {
	return false;
	}
	}
	return true;

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

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8303512: Race condition when computing is_loaded property of TypePtr::InterfaceSet

Reviewers

• Roland Westrelin (@rwestrel - Reviewer) ⚠️ Review applies to d0dce7b6
• Quan Anh Mai (@merykitty - Committer) ⚠️ Review applies to d0dce7b6
• Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to d0dce7b6

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13868/head:pull/13868
$ git checkout pull/13868

Update a local copy of the PR:
$ git checkout pull/13868
$ git pull https://git.openjdk.org/jdk.git pull/13868/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13868

View PR using the GUI difftool:
$ git pr show -t 13868

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13868.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back thartmann! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@TobiHartmann The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 04: Full - Incremental (fbe7578f)
• 03: Full - Incremental (d5781d8a)
• 02: Full - Incremental (d0dce7b6)
• 01: Full - Incremental (27d502d0)
• 00: Full (ad9863c5)

[merykitty]

It seems to me that if 2 threads access a variable racily without using atomic, it results in undefined behaviours. This patch removes the logic and therefore removes the race condition. Is there any risk with that in the surrounding code, too? I see you reordered _hash_computed and _exact_klass_computed assignment, if these reorders matter, should they be at least releasing stores instead? Thanks a lot.

[TobiHartmann]

Thanks for looking at this, @merykitty. I did these reorders because I found it weird that we set ..._computed before initializing the corresponding fields. I thought that for _hash and _exact_klass it shouldn't really matter if they are racily accessed.

On second thought though, I think you are right that this could lead to undefined behavior, especially if the C++ compiler decides to reorder again. One thread could then observe _exact_klass_computed == true and read garbage from _exact_klass because another thread is still in the process of computing and setting _exact_klass.

Since lock-free data structures are hard to get rid, and the overhead would only be needed for a few shared types, I propose to eagerly compute _hash and _exact_klass. I pushed an updated fix.

[TobiHartmann]

I have another set of refactoring changes. Will push these later today after some more cleanup / testing.

[TobiHartmann]

Done.

[rwestrel]

Looks good to me. Nice refactoring and simplification.

[rwestrel]

Why not remove the interface_handling parameter? It doesn't seem useful.

[TobiHartmann]

Right, I'll remove it.

[openjdk]

@TobiHartmann This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: roland, qamai, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[vnkozlov]

Looks good.

[TobiHartmann]

Thanks for the reviews, Roland, Quan Anh and Vladimir!

[TobiHartmann]

I'm seeing issues in higher tier testing. Investigating.

[TobiHartmann]

False alarm. I'm integrating this now.

[TobiHartmann]

/integrate

[openjdk]

Going to push as commit ad348a8.
Since your change was applied there has been 1 commit pushed to the master branch:

• 911cc7c: 8305819: LogConfigurationTest intermittently fails on AArch64

Your commit was automatically rebased without conflicts.

[openjdk]

@TobiHartmann Pushed as commit ad348a8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.