On Fri, 21 May 2021 19:20:29 GMT, Volker Simonis <simo...@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
