RFR: 8048190: NoClassDefFoundError omits original ExceptionInInitializerError [v2]

[Coleen Phillimore]

On Thu, 5 Aug 2021 04:09:47 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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
>
> 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

fixed.

> 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?

Yes, they should go there. I don't like the big global symbols file, but that's where they should go.

> 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.

ok, yes, I agree.

> 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.

Yes, regarding your other comment about whether the original exception should be reloaded and reinitialized, and whether its initialization could be the cause of the original exception.  I'll try to find a creative solution here so that the most common cases are still supported because I think this would be a helpful feature most of the time.  If you have some ideas, let me know (either here or offline).  Thanks.

> 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 ?

I was thinking about multi-threading but only one class initialization is happening at once.  It's already not concurrent. I need the lock for concurrent class unloading though.

> src/hotspot/share/oops/instanceKlass.cpp line 1076:
> 
>> 1074:   InitErrorElement* h;
>> 1075:   {
>> 1076:     MutexLocker ml(ClassInitError_lock);
> 
> Pass THREAD.

ok

> 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.

ok

> 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/

fixed

> 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();
>   }
> }

Oh wow, that's great.  I didn't think javac would fall for that.  Thanks!

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

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