RFR: 8261480: MetaspaceShared::preload_and_dump should check exceptions [v2]

[Ioi Lam]

On Tue, 16 Mar 2021 04:09:40 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   @dholmes-ora comments
>
> src/hotspot/share/classfile/classListParser.cpp line 124:
> 
>> 122:       // and keep going to the next line.
>> 123:       CLEAR_PENDING_EXCEPTION;
>> 124:       log_warning(cds)("Preload Warning: Cannot find %s", _class_name);
> 
> Including some information about the exception would be useful - it might not be what we think.

This is the same as the current behavior -- all exceptions encountered during parsing of the classlist are printed as "Cannot find ...". Many of the test cases depend on this message.

I want to limit the size of this PR and stick to non-functional changes only. I plan to improve the message in do this in a separate RFE: [JDK-8263469](https://bugs.openjdk.java.net/browse/JDK-8263469) "CDS log should print specific reasons for loading failures"

> src/hotspot/share/classfile/classListParser.cpp line 456:
> 
>> 454:     log_info(cds)("Prohibited package for non-bootstrap classes: %s.class from %s",
>> 455:           _class_name, _source);
>> 456:     THROW_NULL(vmSymbols::java_lang_ClassNotFoundException());
> 
> Is that the right exception to use here? I'm not clear of the context for encountering this. What does regular classloading do in such a case?

Same as above. This will be improved in [JDK-8263469](https://bugs.openjdk.java.net/browse/JDK-8263469).

> src/hotspot/share/classfile/classListParser.cpp line 527:
> 
>> 525: }
>> 526: 
>> 527: void ClassListParser::resolve_indy(Thread* current, Symbol* class_name_symbol) {
> 
> Should current be a JavaThread here?

Currently all the `current` parameters will come from the THREAD/TRAPS of its caller, like

int ClassListParser::parse(TRAPS) {
    ...
    resolve_indy(THREAD, class_name_symbol);

I could change the caller to use THREAD->as_JavaThread(), but I think that's too much ceremony. It's better to change to `JavaThread*` after THREAD/TRAPS are changed to `JavaThread` (JDK-8252685)

> src/hotspot/share/classfile/classListParser.cpp line 498:
> 
>> 496:         int callee_index = pool->method_handle_klass_index_at(arg);
>> 497:         Klass* callee = pool->klass_at(callee_index, CHECK);
>> 498:         if (callee != NULL) {
> 
> AFAICS callee can never be NULL if no exception was thrown.

Fixed.

> src/hotspot/share/classfile/classLoaderExt.cpp line 271:
> 
>> 269:   ClassPathEntry* e = find_classpath_entry_from_cache(THREAD, path);
>> 270:   if (e == NULL) {
>> 271:     THROW_NULL(vmSymbols::java_lang_ClassNotFoundException());
> 
> Again not obvious this is the right exception to throw. It is also not very informative as to what the actual problem was - isn't it the "source:" that was not found here?

This will be improved in [JDK-8263469](https://bugs.openjdk.java.net/browse/JDK-8263469).

> src/hotspot/share/classfile/classLoaderExt.cpp line 276:
> 
>> 274:   stream = e->open_stream(file_name, CHECK_NULL);
>> 275:   if (stream == NULL) {
>> 276:     THROW_NULL(vmSymbols::java_lang_ClassNotFoundException());
> 
> Same comment as above.

This will be improved in [JDK-8263469](https://bugs.openjdk.java.net/browse/JDK-8263469).

> src/hotspot/share/classfile/classLoaderExt.cpp line 288:
> 
>> 286:                                           loader_data,
>> 287:                                           cl_info,
>> 288:                                           THREAD);
> 
> I think I'd prefer to see a local introduced and then CHECK used rather than THREAD.

This is our perennial problem: CHECK cannot be used on a `return` statement :-(

See [JDK-8064811](https://bugs.openjdk.java.net/browse/JDK-8064811) (Use THREAD instead of CHECK_NULL in return statements)

> src/hotspot/share/classfile/classLoaderExt.cpp line 298:
> 
>> 296: static GrowableArray<CachedClassPathEntry>* cached_path_entries = NULL;
>> 297: 
>> 298: ClassPathEntry* ClassLoaderExt::find_classpath_entry_from_cache(Thread* current, const char* path) {
> 
> If this is executed by the vmThread at dump time then I'd prefer to  see current called vmThread.

This is executed only in the main Java thread.

> src/hotspot/share/classfile/classLoaderExt.cpp line 327:
> 
>> 325:   Thread* THREAD = current; // For exception macros.
>> 326:   new_entry = create_class_path_entry(path, &st, /*throw_exception=*/false,
>> 327:                                       false, false, CATCH); // will never throw
> 
> Given we are actually dealing with the VMThread which can't (or shouldn't!) actually throw exceptions I'm concerned about this chunk of code. I don't think we have a good idiom for these cases where the VMThread calls potentially exception throwing code in a way that should never throw an exception. Ideally all the exception-related fields would be in JavaThread.

All code changed by this patch executes in the main Java thread.

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

PR: https://git.openjdk.java.net/jdk/pull/2925