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

[David Holmes]

On Fri, 22 Nov 2024 17:25:54 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

>> Currently, when retrieving a ClassFileStream during runtime, we call into the zip library to retrieve the stream based only on a class name. This doesn't work well if the class is in a multi-release jar under a versioned directory such as `META-INF/versions/9/Foo.class`. To address this issue, this change calls the java api `ClassLoader.getResourceAsStream()` to retrieve the stream.
>> 
>> Passed tiers 1 - 4 testing.
>
> 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.

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());

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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1855752520
PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1855752795
PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1855756738