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

Tue Mar 22 17:24:58 UTC 2016

Hi Coleen,

Your latest changes look good.

Thanks, Harold

On 3/22/2016 8:18 AM, Coleen Phillimore wrote:
>
> 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
>>>
>