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

8245216: [lworld] The JVM should inject the IdentityObject interface to types which need it #51

Closed
wants to merge 8 commits into from

Conversation

fparain
Copy link
Collaborator

@fparain fparain commented May 18, 2020

Please review these changes for JDK-8245216.

This code makes the JVM to inject the IdentityObject interface when needed.
The details are in the CR:
https://bugs.openjdk.java.net/browse/JDK-8245216

Thank you,

Fred


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8245216: [lworld] The JVM should inject the IdentityObject interface to types which need it ⚠️ Title mismatch between PR and JBS.

Reviewers

  • Harold Seigel (hseigel - Committer) ⚠️ Review applies to 00cfcb6

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 18, 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 May 18, 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:

8245216: lworld] The JVM should inject the IdentityObject interface to types which need it

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.

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

  • e8894bd: 8245019: lworld] SIGSEGV in BufferBlob::buffered value type due to instruction memory corruption

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

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

Webrevs

@fparain
Copy link
Collaborator Author

@fparain fparain commented May 18, 2020

Got some crashes during testing, investigating...
[update] Class instrumentation/redefinition requires some fixes.

@sadayapalam
Copy link
Collaborator

@sadayapalam sadayapalam commented May 19, 2020

This patch addresses the two langtools failure it should. Thanks!

However, hotspot/jtreg/runtime/valhalla/valuetypes/testSupers/TestSuperClasses.java is still referencing withdrawn classes and fails. Are you able to include a fix for that too ?

I looked into preparing a patch, but not familiar with jcod files...

@fparain
Copy link
Collaborator Author

@fparain fparain commented May 19, 2020

The PR has been updated with the following changes:

  • fix JVMTI class file reconstruction to exclude injected interfaces
  • fix java.lang.Class.ArrayMethods and TestSuperClasses tests
  • fix JVMCI implementation and test

No more regression with tier1. Testing with tier2 and tier3 is in progress.

@fparain fparain changed the title 8245216: [lword] The JVM should inject the IdentityObject interface to types which need it 8245216: [lworld] The JVM should inject the IdentityObject interface to types which need it May 19, 2020
@fparain
Copy link
Collaborator Author

@fparain fparain commented May 19, 2020

Stress testing shown an issue with metaspace allocation/deallocation.
Withdrawing the PR until the issue has been solved.

@fparain fparain marked this pull request as draft May 19, 2020
@openjdk openjdk bot removed the rfr label May 19, 2020
@@ -64,7 +64,8 @@ bool VerificationType::resolve_and_check_assignability(InstanceKlass* klass, Sym
// Otherwise, we treat interfaces as java.lang.Object.
return !from_is_array ||
this_class == SystemDictionary::Cloneable_klass() ||
Copy link
Member

@hseigel hseigel May 20, 2020

Choose a reason for hiding this comment

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

Can you update the comment at line 63 to include java.lang.IdentityObject?

Copy link
Member

@hseigel hseigel left a comment

Changes look good, other than the nit about the comment in verificationType.cpp.
Thanks for doing this.

@fparain
Copy link
Collaborator Author

@fparain fparain commented May 20, 2020

Thank you Harold. I've updated the comment in verificationType.cpp in the last commit.

Fred

@fparain fparain marked this pull request as ready for review May 20, 2020
@openjdk openjdk bot added the rfr label May 20, 2020
@fparain
Copy link
Collaborator Author

@fparain fparain commented May 21, 2020

/integrate

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

@openjdk openjdk bot commented May 21, 2020

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

  • e8894bd: 8245019: lworld] SIGSEGV in BufferBlob::buffered value type due to instruction memory corruption

Your commit was automatically rebased without conflicts.

Pushed as commit ec75333.

@fparain fparain deleted the injected_interfaces branch May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants