RFR: 8302491: NoClassDefFoundError omits the original cause of an error [v2]

[David Holmes]

On Wed, 15 Feb 2023 02:39:24 GMT, Ilarion Nakonechnyy <inakonechnyy at openjdk.org> wrote:

>> The proposed approach added a new function for getting the cause of an exception -`java_lang_Throwable::get_cause_simple `, that gets called within `InstanceKlass::add_initialization_error`  if an old one `java_lang_Throwable::get_cause_with_stack_trace` didn't succeed because of an exception during the VM call. The simple function doesn't call the VM for getting a stack trace but fills in any other information about an exception.
>> 
>> Besides that, the discovering information about an exception was added to `ConstantPoolCacheEntry::save_and_throw_indy_exc` function. 
>> 
>> Jtreg for reproducing the issue also was added to the commit. 
>> The commit was tested with tier1 tests.
>
> Ilarion Nakonechnyy has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Address review notes
>  - Correct the jtreg test -
>    check stacktrace only for NoClassDefFoundError

So if I understand correctly the scenario is:

1. During execution of clinit of class C a StackOverflowError is thrown.
2. The VM sees the exception and creates the NoClassDefFoundError for C
3. The VM calls `get_cause_with_stack_trace` to get the original SOE but that triggers a secondary SOE.
4. The VM ignores the secondary SOE and the NCDFE is created without a cause.

The fix is to retry step 3 avoiding the Java upcall and so avoiding a secondary SOE.

Is that right?

A few nits/suggestions below. 

Thanks

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

> 2781: }
> 2782: 
> 2783: Handle java_lang_Throwable::get_cause_simple(JavaThread* current, Handle throwable) {

How about `get_cause_without_stacktrace`?

The two methods should be able to share a lot of code rather than duplicating most of it.

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

> 2782: 
> 2783: Handle java_lang_Throwable::get_cause_simple(JavaThread* current, Handle throwable) {
> 2784:   // Same as get_cause_with_stack_trace, but without calling a JVM.

The comment at L2739:

// Call to JVM ...

is wrong as it is actually calling Java. Your comment should be more like:

// Similar to `get_cause_with_strace` but avoids the Java upcall to get the stacktrace, and
// so avoids triggering further exceptions.

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

> 2797:   }
> 2798: 
> 2799:   Symbol* exception_name =  throwable()->klass()->name();

Nit: extra space after `=`

src/hotspot/share/oops/cpCache.cpp line 478:

> 476:   oop cause = java_lang_Throwable::cause(PENDING_EXCEPTION);
> 477:   Symbol* cause_sym = NULL;
> 478:   Symbol* cause_msg = NULL;

s/NULL/nullptr/

src/hotspot/share/oops/cpCache.cpp line 480:

> 478:   Symbol* cause_msg = NULL;
> 479: 
> 480:   if (cause != NULL ) {

s/NULL/nullptr/

test/hotspot/jtreg/runtime/ClassInitErrors/TestNoClassDefFoundCause.java line 27:

> 25:  * @test
> 26:  * @bug 8302491
> 27:  * @requires vm.compMode != "Xint"

Why?

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

> 26:  * @bug 8302491
> 27:  * @requires vm.compMode != "Xint"
> 28:  * @summary Test that StackOverflowError is correctly reporting in stack trace

s/reporting/reported/

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

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12566