RFR: 8048190: NoClassDefFoundError omits original ExceptionInInitializerError [v2]

[David Holmes]

On Wed, 4 Aug 2021 19:59:51 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'master' into init-oom
>  - 8048190: NoClassDefFoundError omits original ExceptionInInitializerError
>  - 8048190: NoClassDefFoundError omits original ExceptionInInitializerError

Hi Coleen,

This was not quite what I had envisaged but initially I thought this looked really neat. However on further thought I think there are a number of potential problems with trying to re-create the original type of exception. The "exception" type may not exist or is not known to the current loader, or the "exception" type does not follow convention and have a string-taking constructor. But also for user-defined exception types you have no idea what execution context might be assumed by the exception constructor logic, or what that logic may do. It may be completely wrong/bad to try and create an exception of that type from the current thread in the current context.

Further comments/suggestions below.

Thanks,
David

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

> 2669: 
> 2670: objArrayOop java_lang_Throwable::get_stack_trace(Handle throwable, TRAPS) {
> 2671:   // Call to JVM to fill in the stack trace and clear declaringObject to not keep classes alive

declaringClassObject

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

> 2675: 
> 2676:   TempNewSymbol sym = SymbolTable::new_symbol("getStackTrace");
> 2677:   TempNewSymbol sig = SymbolTable::new_symbol("()[Ljava/lang/StackTraceElement;");

Should we just add these to vmSymbols to go with the other StackTraceElement related symbols?

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

> 2694:     st.print("in thread %s", thread_name);
> 2695:   } else {
> 2696:     st.print("%s in thread %s", message->as_C_string(), thread_name);

I still think this needs highlighting/emphasizing some way eg.

[In thread "name"]

and the name should be in quotes.

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

> 2696:     st.print("%s in thread %s", message->as_C_string(), thread_name);
> 2697:   }
> 2698:   Handle h_cause = Exceptions::new_exception(THREAD, exception, st.as_string());

This can fail for many reasons, causing the returned exception to not be an instance of "exception" at all, but an exception that was thrown trying to create an instance of  "exception". If that happens we should just return NULL here.

src/hotspot/share/oops/instanceKlass.cpp line 1067:

> 1065:   MutexLocker ml(THREAD, ClassInitError_lock);
> 1066:   InitErrorElement elem(THREAD, exception, stack_trace);
> 1067:   _initialization_error_table.put_if_absent(this, elem, &created);

As there can only ever be one initialization error for a class, surely we should just be asserting that get(this) is NULL, and then use put(this,elem) rather than putIfAbsent ?

src/hotspot/share/oops/instanceKlass.cpp line 1076:

> 1074:   InitErrorElement* h;
> 1075:   {
> 1076:     MutexLocker ml(ClassInitError_lock);

Pass THREAD.

src/hotspot/share/oops/instanceKlass.cpp line 1081:

> 1079:   if (h != nullptr) {
> 1080:     return java_lang_Throwable::recreate_cause(h->exception(), h->message(), h->thread_name(),
> 1081:                                                Handle(THREAD, h->stack_trace()), CHECK_NULL);

We don't use CHECK macros on a return statement.

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

> 25:  * @test
> 26:  * @bug 8048190
> 27:  * @summary Test that the CNFE saves original exception during class initialization.

s/CNFE/NCDFE/

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

> 26:  * @bug 8048190
> 27:  * @summary Test that the CNFE saves original exception during class initialization.
> 28:  * @compile InitNPE.jasm InitOOM.jasm

You don't need jasm files for this but you do need to know a little trick to get around the javac compiler:

public class InitNPE {
  static {
    if (true) throw new NullPointerException();
  }
}

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

Changes requested by dholmes (Reviewer).

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