RFR: 8345911: Enhance error message when IncompatibleClassChangeError is thrown for sealed class loading failures [v4]

[Alan Bateman]

On Fri, 13 Dec 2024 04:22:20 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> When a sealed superclass and its permitted subclass get loaded by different classloaders they end up in different modules (the unnamed module of each loader) and cause an `IncompatibleClassChangeError` to be thrown when loading the subclass. The existing error message is not very useful in recognizing this situation:
>> 
>> java.lang.IncompatibleClassChangeError: class SealedSubClass cannot inherit from sealed class SealedClass
>> 
>> as inspection of the class sources will show the expected `permits` and `extends` clauses.
>> 
>> This change augments the `InstanceKlass::has_as_permitted_subclass` method to provide a means to return a meaningful error message to the `ClassFileParser` callers for use in any ICCE that is to be thrown. That same message is shared between unified logging within the method, and the throwing of the ICCE by the caller. We introduce a new helper to throw the ICCE in this case.
>> 
>> The changes also adds a little helper method to `ModuleEntry` to make it easier to gets its name.
>> 
>> The existing tests are updated to reflect the updated error messages, and a new test for the missing "different module" situation is added.
>> 
>> Note: as with the existing logging code the message always refers to class/subclass when in some cases it is really interface and/or subinterface. This can be changed to be more accurate but greatly complicates the formulation of the messages. It is not uncommon to use "(sub)class" when we mean "(sub)class or (sub)interface".
>> 
>> The presented implementation exposes the error message by having the caller pass in a `stringStream` which is then used to store the message. This has the downside of always requiring a `stringStream` in the caller, even if there is no problem to report. Alternatively, we could create the `stringStream` in the callee and then "return" its `as_string()` result to the caller via an out parameter (e.g. `char** error_msg`). However because of the lifetime of the `stringStream` we would need to `os::strdup` the `as_string()` value before returning it, and then free it in the caller. A third alternative would be to push the throwing of the ICCE down into the `InstanceKlass` method so there is no need to pass the message around - though that blurs the line of "division of responsibility" between checking the condition and deciding it should result in ICCE being thrown (the normal pattern is to have the `ClassFileParser` code make that decision, while `InstanceKlass` just provides the que
 ries - ...
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update messages per feedback from Alan

src/hotspot/share/oops/instanceKlass.cpp line 246:

> 244:              this->external_name(), this->package() != nullptr ? this->package()->name()->as_C_string() : "unnamed",
> 245:              this->module()->loader_data()->loader_name_and_id());
> 246:     log_trace(class, sealed)(" - %s", ss.as_string());

I'm a bit puzzled by the check so I'm not commenting on the log/exception message just yet.

If the super class (this) is in an unnamed module then the permitted subclass (k) must be in the same package.

If the super class is in a named module then permitted subclass can be in a different package but both need to be public in order.

I'm just trying to see how this maps to the check because it doesn't appear to take named vs. unnamed module into account, e.g. the subclass k might be public but in a different package of the same unnamed module. It's possible that I've mis-read this but it to goes to what the exception message is for these cases.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22703#discussion_r1883895233