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

[Tom Rodriguez]

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
>