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

[Tom Rodriguez]

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