RFR: 8048190: NoClassDefFoundError omits original ExceptionInInitializerError [v5]

[David Holmes]

On Mon, 9 Aug 2021 17:29:09 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This is a change to save the class initialization error stack trace in a hashtable and return it as the cause when NoClassDefFoundError is thrown.  The first commit is a more limited version of this that just changes the message, by adding to the message string.  The second commit is getting and saving the stack trace for the original exception, and using the thread in the message.  See CR for more details about how the message looks.
>> 
>> Tested with tier1-3 tests on 3 platforms.  Tier 4-6 in progress (all but two done and passed).
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix java.base/bootstrap class loader comment.

Hi Coleen,

Still thinking about the details of this. One concern below, and a few minor comments.

Thanks,
David

src/hotspot/share/classfile/javaClasses.cpp line 2685:

> 2683:   assert(stack_trace->is_objArray(), "Should be an array");
> 2684: 
> 2685:   // If the original exception was in the bootstrap class loader, then use that

Not sure that is a sufficiently strong enough check due to -Xbootclasspath:/a. You may need to check actual java.base module.

src/hotspot/share/classfile/javaClasses.cpp line 2694:

> 2692:                              vmSymbols::java_lang_ExceptionInInitializerError();
> 2693: 
> 2694:   // Now create a the same exception with this stacktrace and thread name.

Typo: a the

src/hotspot/share/classfile/javaClasses.cpp line 2711:

> 2709: 
> 2710:   // If new exception returns a different exception while creating the exception, return null.
> 2711:   if (h_cause->klass()->name() != exception_name) {

Some logging here to show that an exception occurred trying to do this would be useful I think.

test/hotspot/jtreg/runtime/ClassInitErrors/InitExceptionTest.java line 43:

> 41: class InitOOM {
> 42:     static {
> 43:         if (true) foo();

I would just throw new OutOfmemoryError. Trying to force an actually OOME is too fragile, depends on heap size and GC etc. and we don't care that it is a real OOME.

test/hotspot/jtreg/runtime/ClassInitErrors/InitExceptionUnloadTest.java line 28:

> 26:  * @bug 8048190
> 27:  * @summary Test that the NCDFE saves original exception during class initialization,
> 28:  *          and doesn't cause the classes in the stacktrace to be unloaded.

Don't you mean "and doesn't prevent the classes in the stacktrace from being unloaded"?

test/hotspot/jtreg/runtime/ClassInitErrors/InitExceptionUnloadTest.java line 53:

> 51:     }
> 52:     // Has to be in jasm to throw special exception in class initializer.
> 53:     // static public class ThrowsSpecialException

The reason it has to be jasm is because you've made it a checked exception by extending Throwable directly. If you make it an unchecked exception (extend Error or RuntimeException) then you can use the same javac `if (true) throw` trick. Static initializers can never be declared to throw checked exceptions.

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

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