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

[Coleen Phillimore]

On 3/21/16 1:12 AM, David Holmes wrote:
> 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?

I don't think we should refactor InstanceKlass::link_class() for this.

I can rewrite so that every initialize_known_method call does:

bool ok;
ok = initialize_known_method(<params>, CHECK);
if  (!ok) return ok;

ok = initialize_known_method(<params>, CHECK);
if (!ok) return ok;

...

return true;

Which I thought was really ugly and meaningless, since as I said, this 
case is really an assert that the library doesn't match.

I could change it to an assert.

Which is your preference?  This is not my preference.

Thanks,
Coleen
>
>> 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