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

Tue Mar 22 12:18:37 UTC 2016

Hi David,
Thank you for reviewing.

On 3/21/16 10:07 PM, David Holmes wrote:
> 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 ...
>

Yes, the CHECK in link_class is also a pretense.  I added some dummy 
code to throw a VerifyError and it got the assertion:

#  assert(resolved_method->method_holder()->is_linked()) failed: must be 
linked

 From trying to call the Throwable.<clinit>.  I thought we had fixed the 
jvm to handle this better.

I think the code I had is easier to read, but in appreciation for your 
code reviews, I've changed it to your if statement and retested.

http://cr.openjdk.java.net/~coleenp/8143269.02/webrev

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

Right, which I thought was the objection.
>
> 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 ??
>

I meant a fatal() call instead of vm_exit_during_initialization() since 
the case that I'm calling it is an error in the JVM, not really from a 
users perspective.

And YES, initialization is a messy business.  Kim filed this bug a while 
ago for one aspect of initialization.  Throwing exceptions and/or 
returning JNI_ERR is another aspect that makes it messy. Returning 
JNI_ERR to some process embedding the JVM means that we've cleaned up 
memory that we've allocated, of course, we haven't done that.  I think 
we have other bugs like that too.

https://bugs.openjdk.java.net/browse/JDK-8068392

Thanks,
Coleen

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