[13] RFR (XS): 8225141: Better handling of classes in error state in fast class initialization checks

[Vladimir Ivanov]

Thanks, David!

Best regards,
Vladimir Ivanov

On 03/06/2019 09:35, David Holmes wrote:
> Hi Vladimir,
> 
> Thanks for clarifying how the different pieces connect. I'm somewhat 
> surprised this wasn't caught during pre-push testing, but hopefully this 
> now covers all cases.
> 
> Thanks,
> David
> 
> On 1/06/2019 9:53 pm, Vladimir Ivanov wrote:
>> Thanks for the feedback, David.
>>
>>>> What I propose is to set InstanceKlass::_init_thread only for the 
>>>> duration when the klass is in being_initialized state and reset it 
>>>> back to NULL when changing class state. It makes existing 
>>>> "_init_thread == current_thread" check equivalent to "_init_state == 
>>>> being_initialized && _init_thread == current_thread".
>>>
>>> That seems reasonable. By clearing the init_thread we no longer 
>>> consider this a recursive initialization by the current thread and 
>>> take the slow path which will reveal the class is erroneous.
>>>
>>> Looking at the fast init changes I'm unclear why these assertions are 
>>> valid:
>>>
>>> +void LIR_Assembler::clinit_barrier(ciMethod* method) {
>>> +  assert(method->holder()->is_being_initialized() || 
>>> method->holder()->is_initialized(),
>>> +         "initialization should have been started");
>>>
>>> +  if (C->clinit_barrier_on_entry()) {
>>> +    assert(C->method()->holder()->is_being_initialized() || 
>>> C->method()->holder()->is_initialized(),
>>> +           "initialization should have been started");
>>>
>>> I'm not sure how we get to these code fragments such that its 
>>> guaranteed initialization must already have been initiated (and 
>>> possibly completed) - can't this be the first time we've called a 
>>> static method and so the current thread would become responsible for 
>>> doing the initialization?
>>
>> First of all, sorry for the confusion. I should have mentioned that 
>> this patch is intended to go in along with the fix for JDK-8225106 
>> [1]. It relaxes the aforementioned asserts and makes class error state 
>> expected.
>>
>> Still, pre-initialized states shouldn't be seen by JITs there: even in 
>> -Xcomp mode, first invocation triggers resolution (which initiates 
>> class initialization) and only then resolved method is submitted for 
>> compilation.
>>
>>> And again what if the class is actually in an error state? This ties 
>>> in to JDK-8225106.
>>
>> Proposed fix for JDK-8225106 adds new logic which fail the compilation 
>> if method holder is in error state. Plus, if initialization fails 
>> after compilation is over, the barrier will forward execution into 
>> slow path and exception will be thrown during re-resolution. This 
>> scenario is tested by runtime/clinit/ClassInitBarrier (and the fix 
>> adds more scenarios).
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> [1] 
>> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-May/034058.html 
>>
>>
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Testing: hs-precheckin-comp, tier1-4
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8223213