8310874: Runthese30m crashes with klass should be in the placeholders during verification #14889
Conversation
… during verification
|
👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into |
… during verification
| if (definer_acquire() != nullptr) { | ||
| st->print(", definer "); | ||
| definer()->print_value_on(st); | ||
| definer_acquire()->print_value_on(st); |
There was a problem hiding this comment.
Ignoring whether we need acquire semantics at all for definer, you never need to issue two acquires like this. The first acquire performs any necessary memory synchronization related to the associated release. And if the definer could actually be changing concurrently at this point then your null check is ineffective.
There was a problem hiding this comment.
Since I added the acquire version, I wanted to remove the non-acquire version to avoid mistakes.
| // If a class loader supports parallel classloading with AllowParallelDefineClass for parallel-capable class non-boot | ||
| // class loaders, handle parallel define requests. In this case, find_or_define_instance_class may return a different | ||
| // InstanceKlass, in which case the old k would be deallocated | ||
| k = find_or_define_instance_class(h_name, class_loader, k, CHECK_NULL); |
There was a problem hiding this comment.
Now that you call this for parallel-capable and non-parallel-capable loaders the comments preceding the definition of find_or_define_instance_class no longer make complete sense.
There was a problem hiding this comment.
This comment is still true. non bootstrap parallel-capable class loaders might return a different k.
There was a problem hiding this comment.
Edit. yes this comment repeats what find_or_define_instance_class already says later on, so doesn't really add anything here.
There was a problem hiding this comment.
What would make more sense is find_or_define_instance_class was renamed define_instance_class (which actually handled the AllowParallelDefine logic without bothering the caller about it).
There was a problem hiding this comment.
maybe not. It does return a 'k' and the comment explains why.
There was a problem hiding this comment.
My point is that this:
// If a class loader supports parallel classloading handle parallel define requests.
// find_or_define_instance_class may return a different InstanceKlass
InstanceKlass* SystemDictionary::find_or_define_instance_class(Symbol* class_name, Handle class_loader,
InstanceKlass* k, TRAPS) {
makes it seem like this method is only for parallel-capable loaders when that is no longer true. The same goes for the large comment block ahead of find_or_define_helper. These methods do not document how they work for non-parallel-capable loaders.
|
Opening for review with @dholmes-ora comments on the Draft PR. |
Webrevs
|
|
Remerged with master because this PR got old and forgotten. It still passes all the tests and it is ready for review. |
|
@iklam pointed out to me privately that SystemDictionary::load_instance_class has the same pattern and the placeholder tokens are not consistent here either. For LOAD_INSTANCE (ie loading), we only take a placeholder token for the bootstrap loader and for non-parallel capable class loaders, and NOT for parallel capable class loaders. |
|
FYI, I like this patch for other reasons but not for fixing this bug. |
This is a better fix and cleanup to make placeholder use for non-parallel capable class loaders the same as the other sort of class loaders, and fixes potential problems with thread safety in the definer field of the PlaceholderEntry. It's not easy to backport, which is why I chose the other fix for this bug.
Tested with tier1-4 with and without injected LoaderConstrantTable::verify code.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14889/head:pull/14889$ git checkout pull/14889Update a local copy of the PR:
$ git checkout pull/14889$ git pull https://git.openjdk.org/jdk.git pull/14889/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14889View PR using the GUI difftool:
$ git pr show -t 14889Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14889.diff
Webrev
Link to Webrev Comment