Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8243204 add class load time checking of abstract super classes of… #34

Closed
wants to merge 4 commits into from

Conversation

hseigel
Copy link
Member

@hseigel hseigel commented May 4, 2020

Please review this chagne. It adds checks of super classes of inline classes. It checks that an inline class's super class is either java.lang.Object or an abstract class. And, if abstract, checks that the super, and its super, etc. do not have instance fields, nor synchronized instance methods, or non-empty constructors, nor constructors with args.

The fix was tested with tiers 1 - 3.


Progress

  • Change must not contain extraneous whitespace

Reviewers

  • Frederic Parain (fparain - Committer)

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/34/head:pull/34
$ git checkout pull/34

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 4, 2020

👋 Welcome back hseigel! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request.

@openjdk
Copy link

@openjdk openjdk bot commented May 4, 2020

@hseigel This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

JDK-8243204 add class load time checking of abstract super classes of…

Reviewed-by: fparain
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there has been 1 commit pushed to the lworld branch:

  • e586cdb: 8244559: [lworld] Lambda and parameterized ref of an inline type doesn't work well

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge lworld into your branch, and then specify the current head hash when integrating, like this: /integrate e586cdbe737261930b0f302312f8881a0e26ae62.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 4, 2020

Webrevs

@fparain
Copy link
Collaborator

@fparain fparain commented May 4, 2020

Shouldn't ClassFileParser::check_super_of_inline_type() also check that no super classes implement the java.lang.IdentityObject interface?

@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2020

@hseigel this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout abstractSuper
git fetch https://git.openjdk.java.net/valhalla lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

@hseigel
Copy link
Member Author

@hseigel hseigel commented May 7, 2020

The update to the pull request implements John's "bit A and bit B" idea posted in the java-valhalla slack channel. Instead of A and B, the flags are named invalid_inline_super and invalid_identity_super. In this pull request, invalid_identity_super exists but is never set nor read. That is for a future change.

Flag invalid_inline_super is set to TRUE in a class's InstanceKlass, if, at class load time, the class is determined to be an invalid super class for an inline type.

Checking that the class's constructor is abstract will be done at a future date once there is compiler support.

bool _invalid_inline_super; // if true, invalid super type for an inline type.
bool _invalid_identity_super; // if true, invalid super type for an identity type.
Copy link
Collaborator

@fparain fparain May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to use two bits from _misc_flags instead of adding two bool fields to InstanceKlass?

Copy link
Member Author

@hseigel hseigel May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks for the suggestion!

bool ClassFileParser::is_invalid_super_for_inline_type(const InstanceKlass* ik) {
if (ik->name() == vmSymbols::java_lang_IdentityObject()) {
return true;
}
if (ik->is_interface() || ik->name() == vmSymbols::java_lang_Object()) {
return false;
}
if (!ik->is_abstract() || ik->has_nonstatic_fields()) {
return true;
} else {
Array<Method*>* methods = ik->methods();
// Look at each method.
for (int x = 0; x < methods->length(); x++) {
const Method* const method = methods->at(x);
if (method->is_synchronized() && !method->is_static()) {
return true;

} else if (method->name() == vmSymbols::object_initializer_name()) {
if (method->signature() != vmSymbols::void_method_signature() ||
!method->is_vanilla_constructor()) {
return true;
}
}
}
}
return false;
}
Copy link
Collaborator

@fparain fparain May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this method could be made static.

Copy link
Member Author

@hseigel hseigel May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// Return true if the specified class is not a valid super class for an inline type.
// A valid super class for an inline type is abstract, has no instance fields,
// does not implement interface java.lang.IdentityObject (checked elsewhere), has
// an empty body-less no-arg constructor, and no synchronized instance methods.
Copy link
Collaborator

@fparain fparain May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should be more explicit about the fact that it is performing a local test, without considering super types. The final decision taking account super types being performed in fill_instance_klass().

Copy link
Member Author

@hseigel hseigel May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest change updates this comment.

@hseigel
Copy link
Member Author

@hseigel hseigel commented May 8, 2020

Today's updated change improves the comment for function is_invalid_super_for_inline_type() and makes it a static function. Also, it moves the InstanceKlass invalid_inline_super and invalid_identity_super flags into misc_flags.

fparain
fparain approved these changes May 8, 2020
Copy link
Collaborator

@fparain fparain left a comment

Thank you for the changes.
Looks good to me.

Fred

@hseigel
Copy link
Member Author

@hseigel hseigel commented May 8, 2020

/integrate

@openjdk openjdk bot closed this May 8, 2020
@openjdk openjdk bot added integrated and removed ready labels May 8, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 8, 2020

@hseigel The following commits have been pushed to lworld since your change was applied:

  • e586cdb: 8244559: [lworld] Lambda and parameterized ref of an inline type doesn't work well

Your commit was automatically rebased without conflicts.

Pushed as commit b00578f.

@openjdk openjdk bot removed the rfr label May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants