RFR: 8261480: MetaspaceShared::preload_and_dump should check exceptions

[David Holmes]

On 16/03/2021 2:54 pm, David Holmes wrote:
> On Wed, 10 Mar 2021 20:37:17 GMT, Ioi Lam <iklam at openjdk.org> wrote:
> 
>> `MetaspaceShared::preload_and_dump()`, as well as many of the functions that it calls, have extensive use of `THREAD` when calling functions that can potentially throw exceptions. Many of these call sites explicitly check for failure status, or assume that the caller would terminate the VM if an exception were to happen. This makes the code hard to understand and potentially unsafe.
>>
>> I have make 3 types of changes:
>>
>> 1. In most cases, replace THREAD with CHECK. Moved all the fatal error check to `MetaspaceShared::preload_and_dump()`, where it handles any uncaught exceptions and explicitly terminates the VM.
>> 2. If a `TRAPS` function can never throw, replace the `TRAPS` declaration with `Thread* current`
>> 3. In cases where an exception is handled and discarded, use `THREAD` immediately followed by  `HAS_PENDING_EXCEPTION` -> `CLEAR_PENDING_EXCEPTION` to make the code easy to understand. HandleMark is used to assert that no exception escapes.
>>
>> (See `MetaspaceShared::try_link_class` for an example of 2 and 3).
>>
>> Tested with tiers 1-4.
> 
> Hi Ioi,
> 
> I started making comments below but then realized that I have a fundamental problem with this work - you are making the VMThread throw exceptions at dump time and the VMThread should not be throwing exceptions. The VMThread can't execute Java code and so it can only get away with creating a handful of exception types that the VM can construct directly without calling java code. This is somewhat of an abberration - the VMThread should not be creating exceptions and ideally would not have the mechanics to even throw or catch them (the fields would be in JavaThread not Thread). So while we have to deal with this aberration already I do not want to see the VMThread actually creating and "throwing" exceptions at dump time, only to eventually call vm_exit. So this change takes things in the wrong direction for me - the dumping code should not be using TRAPS in general and should be returning NULL up the call chain, or else calling vm_exit as appropriate.

Mea culpa. Ioi pointed out to me that while the code is for dumping it 
is not the code that is actually executed by the VMThread. So in all 
cases we should be in the main JavaThread. In which case please look at 
the comments and also consider where "Thread* current" could be 
JavaThread* current".

Thanks,
David

> Cheers,
> David
> 
> 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.
> 
> 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?
> 
> 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?
> 
> 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.
> 
> 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?
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/2925
>