RFR: 8259839 SystemDictionary exports too much implementation [v3]

[David Holmes]

On Thu, 28 Jan 2021 15:38:25 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/share/classfile/systemDictionary.cpp line 269:
>> 
>>> 267:   if (HAS_PENDING_EXCEPTION || klass == NULL) {
>>> 268:     handle_resolution_exception(class_name, throw_error, CHECK_NULL);
>>> 269:   }
>> 
>> I find the logic for this very confusing. If klass == NULL then isn't it the case that there must be a pending exception?
>> Also, looking inside handle_resolution_exception, if the pending exception is not a ClassNotFoundException then we completely ignore it and throw either ClassNotFoundException or NoClassDefFoundError. I would have expected to at least chain the original exception.
>
> No, a klass can be NULL if load_instance_class returns NULL in a user-defined class loader.
> ClassLoader.loadClass() can return NULL.
>     // Class is not found or has the wrong name, return NULL
>     return NULL;
> There's also a NULL return from the bootclasspath.
> For instance when loading sun/launcher/resources/spi/launcherProvider, this package isn't yet in the bootloader's list of packages, so there's a NULL return from load_instance_class, which is returned by resolve_instance_class_or_null.
> 
> It is sort of confusing that resolve_instance_class_or_null can return an exception OR null.  if it returns an exception, the exception and throw_error is true, the exception is wrapped in the NoClassDefFoundError.  It's in the JVMS 5.3:
> 
> If the Java Virtual Machine ever attempts to load a class C during verification (§5.4.1) or resolution (§5.4.3) (but not initialization (§5.5)), and the class loader that is used to initiate loading of C throws an instance of ClassNotFoundException, then the Java Virtual Machine must throw an instance of NoClassDefFoundError whose cause is the instance of ClassNotFoundException.
> 
> We don't ignore the exception if it's not ClassNotFoundException, we don't clear the exception so it's returned by handle_resolution_exception. Wait, you're right.  I should have a return NULL in the HAS_PENDING_EXCEPTION case to not ignore the existing exception.
> 
> I was trying to figure out why no tests failed with this bug, as it appears that it would override an incoming exception, for instance ClassCircularityException.  Turns out that THROW_MSG -> new_exception() which throws the original exception if one is pending.  I'll fix this code anyway, but I was surprised by this.

`// Class is not found or has the wrong name, return NULL`

If Class is not found then there should be a ClassNotFoundException, or NoClassDefFoundError pending.
If the representation has the wrong name then again a NoClassDefFoundError should be pending.

However, I see there are some paths in `load_instance_class` related to the module system that will return NULL without there being a pending exception.

>> src/hotspot/share/classfile/systemDictionary.cpp line 2028:
>> 
>>> 2026: // Verify that this placeholder exists since this class is in the middle of loading.
>>> 2027: void verify_placeholder(Symbol* class_name, ClassLoaderData* loader_data) {
>>> 2028:   // Only parallel capable class loaders use placeholder table for define class.
>> 
>> Maybe I'm having trouble following the call sequence, but non-parallel capable loaders also add placeholder entries, and then call check_constraints. So why do we not need to verify the placeholder in that case?
>
> Non-parallel capable class loaders do NOT add placeholder entries for DEFINE_CLASS (capitalized because that's the enum).  There are three kinds of placeholder entries for three phases of class loading, LOAD_INSTANCE, LOAD_SUPER (for ClassCircularityException testing), and DEFINE_CLASS.  The latter is only used for parallel capable class loaders (which includes the bootstrap class loader) and is used to implement parallel class definition via AllowParallelDefindClass (except we always add them even when the option is false).

Sorry I'm still not seeing why we only verify DEFINE_CLASS placeholder entries?

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

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