RFR: 8267555: Fix class file version during redefinition after 8238048

[Coleen Phillimore]

On Fri, 21 May 2021 19:20:29 GMT, Volker Simonis <simonis at openjdk.org> wrote:

> In jdk15, [JDK-8238048](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/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](https://bugs.openjdk.java.net/browse/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?

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 line 1861:

> 1859:   // Save fields from the old_cp.
> 1860:   merge_cp->copy_fields(old_cp(), true /* skip_version */);
> 1861:   scratch_cp->copy_fields(old_cp(), true /* skip_version */);

Rather than calling copy_fields, maybe this should revert this:
-  if (old_cp->has_dynamic_constant()) {
-    merge_cp->set_has_dynamic_constant();
-    scratch_cp->set_has_dynamic_constant();
-  }

And then the merge_cp->copy_fields(scratch_cp); ?

The merged_cp should look like the scratch_cp (with version number and generic signature, etc) until after verification and then I guess it should look like the old_cp at the end in redefine_single_class().

src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 4412:

> 4410:   the_class->constants()->set_major_version(scratch_class->constants()->major_version());
> 4411:   scratch_class->constants()->set_major_version(old_major_version);
> 4412: 

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 line 1:

> 1: /*

Can you put this test in test/hotspot/jtreg/serviceability/jvmti/RedefineClasses directory?
All the tests do redefinition differently.  There is a library RedefineClassHelper that you might be able to use and not have to deal with starting the agent.

@ compile TestClassXXX.jasm   // this is the old one
then in the test read all of TestClassNew.jasm bytecodes, and s/New/XXX/ and use
RedefineClassHelper.redefine(TestClassXXX.class, newBytes);

See RedefineRunningMethods.java in that directory for an example.

-------------

Changes requested by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4149