On Thu, 17 Apr 2025 07:14:25 GMT, Timofei Pushkin <tpush...@openjdk.org> wrote:
>> src/hotspot/share/cds/classListParser.cpp line 517:
>>
>>> 515: return;
>>> 516: }
>>> 517: assert(!supertype->is_shared_unregistered_class(), "unregistered
>>> supertype cannot be overshadowed");
>>
>> Does this always prevent an unregistered class to use any other unregistered
>> class as its super type?
>>
>> I think a better check would be here:
>>
>>
>> if (!k->local_interfaces()->contains(specified_interface)) {
>> + jio_fprintf(defaultStream::error_stream(), "Specified interface %s (id
>> %d) is not directly implemented\n",
>> + specified_interface->external_name(), _interfaces->at(i));
>> print_specified_interfaces();
>> print_actual_interfaces(k);
>> + throw exception .....
>> - error("Specified interface %s (id %d) is not directly implemented",
>> - specified_interface->external_name(), _interfaces->at(i));
>> }
>
> Overshadowing should be checked before attempting to load the class.
> Otherwise if the wrongly used super has a different classfile than the
> specified super we may get class loading errors (see JVMS 5.3.5.3 and
> 5.3.5.4), e.g. if the specified super wasn't final but we try to use a final
> class instead of it. I'll extend the related explanation comments in the code.
>
>> Does this always prevent an unregistered class to use any other unregistered
>> class as its super type?
>
> The only way I see for another class to be used instead is for the
> unregistered class being loaded to reference supertypes of unexpected names
> in its classfile. But this is an error and it will be detected as such in the
> C++ code you've cited. If you have any other cases in mind please share.
Sorry I didn't notice the `overshadower == supertype` check. Now I understand
how it works.
However, I think the logic of the code isn't expressed in the same way as
mentioned in the comments. That's why I misread it. I would suggest
restructuring it like this for clarity.
Also, the opposite to "unregistered" is "built-in". We used to have a third
type (classes for "registered" custom class loaders) but that has been removed,
leaving us the odd terminology. Maybe we'll clean that up when more support is
added to custom loaders in Leyden.
BTW, I think we should use "shadow" instead of "overshadow" for brevity.
void ClassListParser::check_supertype_overshadowing(int supertype_id, const
InstanceKlass* specified_supertype, TRAPS) {
if (!specified_supertype->is_shared_unregistered_class()) {
const InstanceKlass* overshadower =
SystemDictionaryShared::get_unregistered_class(specified_supertype->name());
if (overshadower != nullptr) {
// If an unregistered class U is specified to have a built-in supertype S1
// named SN but an unregistered class S2 also named SN has already been
loaded
// S2 will be incorrectly resolved as the supertype of U instead of S1
due to
// limitations in the loading mechanism of unregistered classes.
// This happens very rarely and we don't support such use cases in the
CDS archive.
ResourceMark rm;
THROW_MSG(vmSymbols::java_lang_UnsupportedOperationException(),
err_msg("%s (id %d) has super-type %s (id %d) overshadowed by
another class with the same name",
_class_name, _id, supertype->external_name(),
supertype_id));
}
}
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2049723549
