RFR (S): 8198909: [Graal] compiler/codecache/stress/UnexpectedDeoptimizationTest.java crashed with SIGSEGV

[Erik Österlund]

Hi Tom,

I approve of having holder_phantom() on Klass. I tried to introduce it 
there a long time ago but got some push back at the time. But I think it 
really ought to be on Klass.

Thanks,
/Erik

On 2018-06-21 22:46, Tom Rodriguez wrote:
>
>
> Erik Österlund wrote on 6/21/18 6:26 AM:
>> Hi,
>>
>>
>> In src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>>
>> Please remove
>> #include "gc/g1/g1BarrierSet.hpp"
>>
>> In src/hotspot/share/jvmci/jvmciCompilerToVM.hpp:
>>
>> I would prefer if KlassRefHandle was called JVMCIKlassHandle, because 
>> it is very specific to JVMCI. On that note it is unfortunate that we 
>> can not simply reuse ciInstanceKlass, which is the klass handle used 
>> by the other compilers.
>>
>> Klass*     _value;
>> should be called _klass
>>
>> Handle     _phantom;
>> should be called _holder
>>
>> Klass*        obj()
>> should be called klass()
>>
>> Otherwise, this looks good, and I don't need another webrev for this.
>
> I've made all the requested edits.  Additionally I never really got an 
> answer to my question about handling of ObjArrayKlass but concluded 
> that it must be handled, so I've moved phantom_holder from 
> InstanceKlass to Klass so it can be used in a uniform way.  I guess 
> the CI handles it implicitly under the assumption that 
> klass->class_loader_data() == 
> ObjArrayKlass::cast(klass)->bottom_klass()->class_loader_data() which 
> should presumably be true.  The new webrev is 
> http://cr.openjdk.java.net/~never/8198909.2/webrev.  I'll consider the 
> movement of phantom_holder to be acceptable unless I hear an objection 
> soon.
>
> tom
>
>>
>> Thanks,
>> /Erik
>>
>> On 2018-06-19 23:34, Tom Rodriguez wrote:
>>> I've generated a webrev with a new KlassRefHandle protecting 
>>> questionable uses in JVMCI. 
>>> http://cr.openjdk.java.net/~never/8198909.1/webrev
>>>
>>> One outstanding question is whether ObjArrayKlass also needs a 
>>> working holder_phantom method.  It would seem so to me but maybe 
>>> there's some reason not?
>>>
>>> tom
>>>
>>> Tom Rodriguez wrote on 6/11/18 10:04 AM:
>>>>
>>>>
>>>> Erik Österlund wrote on 6/11/18 2:09 AM:
>>>>> Hi Tom,
>>>>>
>>>>> Could you please call InstanceKlass::holder_phantom() instead to 
>>>>> keep the class alive? That is the more general mechanism that is 
>>>>> also used by ciInstanceKlass. We don't want to use explicit G1 
>>>>> enqueue calls anymore.
>>>>
>>>> Ok.  I guess the same fix in JDK8 will have the use the explicit 
>>>> enqueue though or is it not required in JDK8?
>>>>
>>>>> Also, you must not perform any thread transition between loading 
>>>>> the weak klass from the MDO until you call holder_phantom, 
>>>>> otherwise it might have been unloaded before you get to call 
>>>>> holder_phantom(). Is this guaranteed somehow in this scenario? I 
>>>>> looked through all callsites and could not find where the Klass 
>>>>> pointer is read in the MDO and subsequently passed into the 
>>>>> CompilerToVM::get_jvmci_type API, and therefore I do not know if 
>>>>> this is guaranteed.
>>>>
>>>> The obviously problematic path is at 
>>>> http://hg.openjdk.java.net/jdk/jdk/file/50469fb301c4/src/hotspot/share/jvmci/jvmciCompilerToVM.cpp#l334 
>>>> when either base_address is a Klass* or base_object is NULL which 
>>>> is where we are reading from non-heap memory.  There are other 
>>>> paths which are reading Klasses through more standard APIs from the 
>>>> ConstantPool for instance.
>>>>
>>>> There isn't an easy way to ensure no safepoint occurs in between so 
>>>> maybe we require the caller of get_jvmci_type to pass in the 
>>>> phantom_holder() as a way of forcing the caller to call 
>>>> holder_phantom() at the appropriate places?  Or is it the case that 
>>>> getResolvedType is the only place where special effort is required? 
>>>> All the other paths are fairly normal HotSpot code but though place 
>>>> that uses klass->implementor() for instance seems like it could be 
>>>> considered to be weak by G1.
>>>>
>>>> http://hg.openjdk.java.net/jdk/jdk/file/50469fb301c4/src/hotspot/share/jvmci/jvmciCompilerToVM.cpp#l368 
>>>>
>>>>
>>>> The lack of a properly working KlassHandle seems like an oversight 
>>>> in the API to me.
>>>>
>>>> tom
>>>>
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>>>
>>>>> On 2018-06-08 22:46, Tom Rodriguez wrote:
>>>>>> The JVMCI API may read Klass* and java.lang.Class instances from 
>>>>>> locations which G1 would consider to be weakly referenced.  This 
>>>>>> can result in HotSpotResolvedObjectTypeImpl instances with 
>>>>>> references to Classes that have been unloaded.  In the this 
>>>>>> crash, JVMCI was reading a Klass* from the profile in an MDO and 
>>>>>> building a wrapper around it. The MDO reference is weak and was 
>>>>>> the only remaining reference to the type so it could be dropped 
>>>>>> resulting in an eventual crash.
>>>>>>
>>>>>> I've added an explicit G1 enqueue before we call out to create 
>>>>>> the wrapper object but is there a more recommended way of doing 
>>>>>> this? Dean had pointed out the oddly named 
>>>>>> InstanceKlass::holder_phantom which is used by the CI. Should I 
>>>>>> be using that?  The G1 barrier is only really need when reading 
>>>>>> from non-Java heap memory but since the get_jvmci_type method is 
>>>>>> the main entry point for this logic it safest to always perform 
>>>>>> it in this path.
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8198909
>>>>>> http://cr.openjdk.java.net/~never/8198909/webrev
>>>>>
>>