On Thu, 27 May 2021 10:26:46 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? > > Volker Simonis has updated the pull request incrementally with one additional > commit since the last revision: > > 8267555: Fix class file version during redefinition after 8238048 Yes, this looks good. Isn't RedefineClassHelper great? Thanks! ------------- Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4149
