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

[Tom Rodriguez]

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