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

8242402: multianewarray is missing checks on the bottom class #105

Closed
wants to merge 2 commits into from

Conversation

@fparain
Copy link
Collaborator

@fparain fparain commented Jul 6, 2020

Please review this patch which adds a missing check in array class resolution.

When an array class is resolved and the bottom type is not a primitive type, the envelope of the bottom type signature is removed in order to extract the class name and load it. Currently, there's no check that the kind of the loaded class matches the enveloped stripped from the signature.

The patch adds this check and includes an unit test.

Tested on all all platforms with mach5, tiers 1 to 3.

Thank you,

Fred


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8242402: multianewarray is missing checks on the bottom class

Reviewers

  • Harold Seigel (hseigel - Committer)

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 6, 2020

👋 Welcome back fparain! 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 Jul 6, 2020

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

8242402: multianewarray is missing checks on the bottom class

Reviewed-by: hseigel
  • 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 /issue command.

There are currently no new commits on the lworld branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate f5f65c5ef51406437ce9ac9e579f75776f61472b.

➡️ 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 Jul 6, 2020

Webrevs

hseigel
hseigel approved these changes Jul 6, 2020
if ((class_name->is_Q_array_signature() && !k->is_inline_klass()) ||
(!class_name->is_Q_array_signature() && k->is_inline_klass())) {
THROW_NULL(vmSymbols::java_lang_IncompatibleClassChangeError());
}
Copy link
Member

@hseigel hseigel Jul 6, 2020

Choose a reason for hiding this comment

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

Is it worthwhile using THROW_MSG_NULL to provide a helpful message with the exception?

Copy link
Collaborator Author

@fparain fparain Jul 6, 2020

Choose a reason for hiding this comment

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

Something like this?

THROW_MSG_NULL(vmSymbols::java_lang_IncompatibleClassChangeError(), "L/Q mismatch on bottom type");

Fred

Copy link
Member

@hseigel hseigel Jul 6, 2020

Choose a reason for hiding this comment

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

Yes. That looks good. Thanks.

@@ -0,0 +1,120 @@
class MultiANewArrayTypeCheck {
Copy link
Member

@hseigel hseigel Jul 6, 2020

Choose a reason for hiding this comment

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

I think a copyright notice is needed here. Also, could you add a comment containing Java pseudo-code that shows what this file is doing?

Copy link
Collaborator Author

@fparain fparain Jul 6, 2020

Choose a reason for hiding this comment

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

I've added the missing copyrights and a comment explaining the content of the jcod file.

Thank you,

Fred

Copy link
Member

@hseigel hseigel Jul 7, 2020

Choose a reason for hiding this comment

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

Looks Good.

hseigel
hseigel approved these changes Jul 7, 2020
Copy link
Member

@hseigel hseigel left a comment

All the changes look good!

@@ -0,0 +1,120 @@
class MultiANewArrayTypeCheck {
Copy link
Member

@hseigel hseigel Jul 7, 2020

Choose a reason for hiding this comment

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

Looks Good.

@fparain
Copy link
Collaborator Author

@fparain fparain commented Jul 7, 2020

@fparain
Copy link
Collaborator Author

@fparain fparain commented Jul 7, 2020

/integrate

@openjdk openjdk bot closed this Jul 7, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Jul 7, 2020

@fparain
Pushed as commit e9c4435.

@fparain fparain deleted the multianewarray_check branch Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants