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

[Coleen Phillimore]

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

I just had a couple of stylistic requested changes.  The option you picked - passing in stringStream - is better than the other two options.  Also a question about ResourceMarks.

src/hotspot/share/classfile/classFileError.cpp line 97:

> 95: void ClassFileParser::classfile_icce_error(const char* msg,
> 96:                                            TRAPS) const {
> 97:   ResourceMark rm(THREAD);

I don't know what this ResourceMark does since you allocate resources in the caller, not here.

src/hotspot/share/classfile/classFileParser.cpp line 4068:

> 4066: 
> 4067:     if (super_ik->is_sealed()) {
> 4068:       stringStream ss;

Seems like the ResourceMark belongs here above the has_as_permitted_subclass call as the as_C_string() functions are called in there.  Not in the classfile_icce_error.

src/hotspot/share/classfile/moduleEntry.hpp line 191:

> 189:   const char* name_as_C_string() {
> 190:     return is_named() ? name()->as_C_string() : UNNAMED_MODULE;
> 191:   }

Maybe PackageEntry::name_as_C_string() should get the same helper function.

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

> 232:              "and sealed class %s is in module '%s' with loader %s",
> 233:              k->external_name(), k->module()->name_as_C_string(), k->module()->loader_data()->loader_name_and_id(),
> 234:              this->external_name(), this->module()->name_as_C_string(), this->module()->loader_data()->loader_name_and_id());

I was going to suggest less verbosity by creating local variables above like this_loader_data and k_loader_data but maybe it's not that useful.  At any rate, could you put each of these long parameters on each line?

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

Same here - put each parameter on its own line.

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

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22703#pullrequestreview-2507486513
PR Review Comment: https://git.openjdk.org/jdk/pull/22703#discussion_r1887669639
PR Review Comment: https://git.openjdk.org/jdk/pull/22703#discussion_r1887668436
PR Review Comment: https://git.openjdk.org/jdk/pull/22703#discussion_r1887665877
PR Review Comment: https://git.openjdk.org/jdk/pull/22703#discussion_r1887664445
PR Review Comment: https://git.openjdk.org/jdk/pull/22703#discussion_r1887666645