RFR: 8343427: Class file load hook crashes on archived classes from multi-release JARs [v4]

[Calvin Cheung]

On Mon, 25 Nov 2024 04:11:20 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   update comments
>
> src/hotspot/share/cds/filemap.cpp line 2718:
> 
>> 2716:   // The result should be a [B
>> 2717:   assert(obj->is_typeArray(), "just checking");
>> 2718:   assert(TypeArrayKlass::cast(obj->klass())->element_type() == T_BYTE, "just checking");
> 
> This seems unnecessary. What kind of errors are you trying to detect with this? The method signature indicates it returns a byte-array.

I adapted the asserts from diagnosticCommand.cpp. I'm keeping the first assert. Is it ok?

> src/hotspot/share/cds/filemap.cpp line 2728:
> 
>> 2726:   return new ClassFileStream(buffer,
>> 2727:                              len,
>> 2728:                              cpe->name());
> 
> Suggestion:
> 
>   return new ClassFileStream(buffer, len,  cpe->name());

Fixed.

> src/java.base/share/classes/java/lang/ClassLoader.java line 1698:
> 
>> 1696:         } catch (IOException e) {
>> 1697:             return null;
>> 1698:         }
> 
> Any `IOException` should just be left pending so that it gets reported eventually. Otherwise you are returning null here but asserting in the VM that null is never returned.

I've changed the java method to throw IOException and removed the try-catch statements.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1857233789
PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1857233832
PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1857233887