8267555: Fix class file version during redefinition after 8238048 #4149

simonis · 2021-05-21T19:20:29Z

In jdk15, JDK-8238048 moved the class file versions from the InstanceKlass into the ConstantPool and introduced a new method ConstantPool::copy_fields(const ConstantPool* orig) which copies not only the source_file_name_index, generic_signature_index and has_dynamic_constant from orig to the current ConstantPool object, but also the minor and major class file version.

This new copy_fields() method is now called during class redefinition (in VM_RedefineClasses::merge_cp_and_rewrite()) at places where previously only the has_dynamic_constant attribute was copied from the old to the new version of the class. If the new version of the class has a different class file version than the previous one, this different class file version will now be overwritten with the class file version of the previous, original class.

In VM_RedefineClasses::load_new_class_versions(), after VM_RedefineClasses::merge_cp_and_rewrite() has completed, we do another verification step to check the results of constant pool merging (in jdk15 this was controlled by VerifyMergedCPBytecodes which was on by default, in jdk16 and later this extra verification step only happens in debug builds). If the new class instance uses features which are not available for the class version of the previous class, this verification step will fail.

The solution is simple. Don't overwrite the class file version of the new class any more. This also requires reintroducing the update of the class file version for the newly redefined class in VM_RedefineClasses::redefine_single_class() like this was done before JDK-8238048.

I'm just not sure about the additional new field updates for source_file_name_index and generic_signature_index in ConstantPool::copy_fields() which were not done before JDK-8238048. Do we want to preserve these attributes from the original class and write them into the new redefined version? If yes, the new code would be correct and we could remove the following code from VM_RedefineClasses::redefine_single_class() because that was already done in ConstantPool::copy_fields():

  // Copy the "source file name" attribute from new class version
  the_class->set_source_file_name_index(
    scratch_class->source_file_name_index());

Otherwise the new would be wrong in the same sense like it was wrong for the class file versions. The differences of between the class file versions and source_file_name_index/generic_signature_index is that the former attributes are mandatory and therefor always available in the new class version while the latter ones are optional. So maybe we should only copy them from the original to the new class if they are not present there already? It currently seems like there's no optimal solution for these attributes?

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8267555: Fix class file version during redefinition after 8238048

Reviewers

Coleen Phillimore (@coleenp - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4149/head:pull/4149
$ git checkout pull/4149

Update a local copy of the PR:
$ git checkout pull/4149
$ git pull https://git.openjdk.java.net/jdk pull/4149/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4149

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4149.diff

bridgekeeper · 2021-05-21T19:21:43Z

👋 Welcome back simonis! 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 · 2021-05-21T19:24:58Z

@simonis The following labels will be automatically applied to this pull request:

hotspot
serviceability

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

mlbridge · 2021-05-21T19:29:16Z

Webrevs

01: Full - Incremental (45d6d006)
00: Full (25f07246)

coleenp

I'm sorry for the delay and for introducing the bug. I'm hopefully back in the frame of mind I was in when making the change. See if what I said makes sense.

src/hotspot/share/prims/jvmtiRedefineClasses.cpp

coleenp · 2021-05-26T16:44:57Z

src/hotspot/share/prims/jvmtiRedefineClasses.cpp

+  u2 old_major_version = the_class->constants()->major_version();
+  the_class->constants()->set_major_version(scratch_class->constants()->major_version());
+  scratch_class->constants()->set_major_version(old_major_version);
+


This looks correct. copy_fields doesn't swap the versions, and I think that's what we want to do.
The source_name_index should match the scratch_class which is above, and the generic_signature_index is checked that it matches, and the one in the scratch constant pool should be adjusted to match the merged constant pool. So I think that's ok.

test/hotspot/jtreg/runtime/ConstantPool/ClassVersion/ClassVersionAfterRedefine.java

simonis · 2021-05-27T10:23:39Z

Hi Coleen,

thanks for reviewing my change. I've updated the code as suggested and moved the test to the new location. Thanks for pointing me to RedefineClassHelper. That considerably simplifies the test. Please let me know what you think?

Best regards,
Volker

coleenp

Yes, this looks good. Isn't RedefineClassHelper great? Thanks!

openjdk · 2021-05-27T13:19:40Z

@simonis 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:

8267555: Fix class file version during redefinition after 8238048

Reviewed-by: coleenp, sspitsyn

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 125 new commits pushed to the master branch:

8a31c07: 8267886: ProblemList javax/management/remote/mandatory/connection/RMIConnector_NPETest.java
ae258f1: 8265418: Clean-up redundant null-checks of Class.getPackageName()
41185d3: 8229517: Support for optional asynchronous/buffered logging
7c85f35: 8267123: Remove RMI Activation
0754266: 8267709: Investigate differences between HtmlStyle and stylesheet.css
23189a1: 8191786: Thread-SMR hash table size should be dynamic
ef368b3: 8265836: OperatingSystemImpl.getCpuLoad() returns incorrect CPU load inside a container
10a6f5d: 8230623: Extract command-line help for -Xlint sub-options to new --help-lint
bea4109: 8187649: ArrayIndexOutOfBoundsException in java.util.JapaneseImperialCalendar
ec65cf8: 8240347: remove undocumented options from jlink --help message
... and 115 more: https://git.openjdk.java.net/jdk/compare/88b114235c5716ea43c55a9c4bc886bf5bcf4b42...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

sspitsyn

Hi Volker,
The fix looks okay to me.
Thank you for fixing it!
Serguei

simonis · 2021-05-28T08:31:04Z

Thanks for the reviews @sspitsyn and @coleenp.

simonis · 2021-05-28T08:31:39Z

/integrate

openjdk · 2021-05-28T08:33:15Z

@simonis Since your change was applied there have been 130 commits pushed to the master branch:

97ec5ad: 8265753: Remove manual JavaThread transitions to blocked
6eb9114: 8266877: Missing local debug information when debugging JEP-330
0c9daa7: 8265029: Preserve SIZED characteristics on slice operations (skip, limit)
95b1fa7: 8267529: StringJoiner can create a String that breaks String::equals
7f52c50: 8182043: Access to Windows Large Icons
8a31c07: 8267886: ProblemList javax/management/remote/mandatory/connection/RMIConnector_NPETest.java
ae258f1: 8265418: Clean-up redundant null-checks of Class.getPackageName()
41185d3: 8229517: Support for optional asynchronous/buffered logging
7c85f35: 8267123: Remove RMI Activation
0754266: 8267709: Investigate differences between HtmlStyle and stylesheet.css
... and 120 more: https://git.openjdk.java.net/jdk/compare/88b114235c5716ea43c55a9c4bc886bf5bcf4b42...master

Your commit was automatically rebased without conflicts.

Pushed as commit 1d2c7ac.

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

8267555: Fix class file version during redefinition after 8238048

25f0724

openjdk bot added the rfr Pull request is ready for review label May 21, 2021

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels May 21, 2021

coleenp suggested changes May 26, 2021

View reviewed changes

8267555: Fix class file version during redefinition after 8238048

45d6d00

coleenp approved these changes May 27, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 27, 2021

sspitsyn approved these changes May 27, 2021

View reviewed changes

openjdk bot closed this May 28, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8267555: Fix class file version during redefinition after 8238048 #4149

8267555: Fix class file version during redefinition after 8238048 #4149

simonis commented May 21, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 21, 2021

openjdk bot commented May 21, 2021

mlbridge bot commented May 21, 2021 •

edited

Loading

coleenp left a comment

coleenp May 26, 2021

simonis commented May 27, 2021

coleenp left a comment

openjdk bot commented May 27, 2021 •

edited

Loading

sspitsyn left a comment

simonis commented May 28, 2021

simonis commented May 28, 2021

openjdk bot commented May 28, 2021

8267555: Fix class file version during redefinition after 8238048 #4149

8267555: Fix class file version during redefinition after 8238048 #4149

Conversation

simonis commented May 21, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 21, 2021

openjdk bot commented May 21, 2021

mlbridge bot commented May 21, 2021 • edited Loading

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

coleenp May 26, 2021

Choose a reason for hiding this comment

simonis commented May 27, 2021

coleenp left a comment

Choose a reason for hiding this comment

openjdk bot commented May 27, 2021 • edited Loading

sspitsyn left a comment

Choose a reason for hiding this comment

simonis commented May 28, 2021

simonis commented May 28, 2021

openjdk bot commented May 28, 2021

simonis commented May 21, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented May 21, 2021 •

edited

Loading

openjdk bot commented May 27, 2021 •

edited

Loading