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

[David Holmes]

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