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

[David Holmes]

On 22/03/2016 12:05 AM, Coleen Phillimore wrote:
> 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 wasn't suggesting that. As I said either both bits of code can throw 
exceptions or neither can, and I would have to assume it is the latter. 
link_class can be replaced by link_class_or_fail ie this:

    ik->link_class(CHECK);
    TempNewSymbol name = SymbolTable::new_symbol(method, CHECK);
    Method* m = ik->find_method(name, signature);
    if (m == NULL || is_static != m->is_static()) {
      ResourceMark rm(THREAD);
      vm_exit_during_initialization(err_msg("Unable to link/verify %s.%s 
method",
                                            ik->name()->as_C_string(), 
method));
    }

becomes (hope I get this right):

TempNewSymbol name = SymbolTable::new_symbol(method, CHECK);
Method* m = NULL;
if (!ik->link_class_or_fail(THREAD) ||
     ((m = ik->find_method(name, signature)) == NULL) ||
     is_static != m->is_static()) {
   ResourceMark rm(THREAD);
   vm_exit_during_initialization(err_msg("Unable to link/verify %s.%s 
method",
                                  ik->name()->as_C_string(), method));
}

The CHECK on new_symbol is still a pretense but ...

> 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'm not sure why you are suggesting that. It would only make sense if if 
there were no vm_exit_during_initialization. ??

It would be interesting to see exactly when java_lang_Class is linked in 
relation to this code executing, so we could see when it would be 
possible to just throw the exceptions in all cases. Pity there seems to 
be no logging for class linking.

> I could change it to an assert.
>
> Which is your preference?  This is not my preference.

I'm not sure what you would assert and where ??

Thanks,
David

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