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

Wed Mar 23 11:40:55 UTC 2016

Hi Coleen,

Thank goodness for inflight wi-fi, I nearly missed this when my battery 
died at the airport :)

Thumbs up! More inline ...

On 22/03/2016 10:18 PM, 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 we moved the failure point but ultimately there has to be some 
point before which you can't throw an exception as it depends on 
initialization of classes that have failed for some reason. But as I 
said it would be interesting to see the actual order of linking for this 
particular chunk of code.

> 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

Much appreciated. It really makes little sense to me to pretend to throw 
on one line and then vm-exit because we know we can't throw.

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

I was commenting on the potential change of behaviour but as I noted it 
is not a practical matter and as you said these failures really indicate 
a basic error in terms of the jdk+hotspot combination. So all good 
there. (I should make my commentary and my "review"  more separate.)

Thanks,
David
-----

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