RFR (s) 8143269: Refactor code in universe_post_init that sets up methods to upcall

[David Holmes]

Hi Coleen,

On 19/03/2016 10:57 PM, Coleen Phillimore wrote:
> Summary: Deferred code review cleanups
>
> This was a review comment I had when the StackWalk API was integrated.
> There were too many of this same code pattern in the middle of
> universe_post_init.  The only real change here is that if the method
> isn't found and doesn't have the right linkage because of a mismatch in
> JDK version, we can vm_exit_during_initialization() rather than try to
> return JNI_ERR.  This avoids ugly code to check
> initialize_known_method's boolean result for each call.  We can't throw
> an exception here.  Because of the EXCEPTION_MARK in universe_post_init,
> any exception gets turned into vm_exit_during_initialization anyway. But
> this wouldn't be a user error, so vm_exit_during_initialization seemed
> like the cleanest choice.

Our initialization code is truly awful. Until the VM is initialized 
any/all exceptions have to convert back to a different form of error 
because there will be no thread (or JVM for that matter) on which the 
user code could ask if an exception occurred. It then comes down to 
whether an error should be reported to the caller of JNI_CreateJavaVM so 
the host program doesn't get terminated, or we abort everything with 
vm_exit_during_initialization. And we're sliding further and further 
into always doing the latter.

So in an absolute sense the new code is incompatible with the old 
because it will terminate host applications that wouldn't previously 
have been terminated - but in a practical sense this is unlikely to make 
any real difference to anything.

That said something about all this is really bugging me. If we can do:

!   SystemDictionary::Finalizer_klass()->link_class(CHECK_false);

which purports to allow an exception to have been posted then how is it 
the immediately following code:

!   Method* m = SystemDictionary::Finalizer_klass()->find_method(
!                                   vmSymbols::register_method_name(),
!                                   vmSymbols::register_method_signature());
!   if (m == NULL || !m->is_static()) {
!     tty->print_cr("Unable to link/verify Finalizer.register method");
!     return false; // initialization failed (cannot throw exception yet)
!   }

can not throw an exception yet? Either both can throw or neither can 
throw AFAICS. So maybe the refactored link() call should not be using 
CHECK_ but also exit immediately?

> Also, I want to get the code to not have to include method names (and
> field names) in vmSymbols.cpp.
>
> I verified that this code won't merge conflict with jigsaw.  There are
> many additional cleanups that can be done here. universe_post_init
> should really be a method belonging to Universe since it sets many
> static fields in Universe, but to avoid merge conflicts, I didn't do that.

If you do that cleanup some time there are some very inaccurate comments 
in relation to the initialization code in init.cpp that could do with 
correcting. And I also spotted two paths to Interpreter::initialize.

Thanks,
David
-----

>
> Tested with JPRT and colocated tests.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8143269.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8143269
>
> Thanks,
> Coleen