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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Tom Rodriguez-2
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
Reply | Threaded
Open this post in threaded view
|

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

Erik Österlund-2
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.
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.

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

Reply | Threaded
Open this post in threaded view
|

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

Tom Rodriguez-2


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
>
Reply | Threaded
Open this post in threaded view
|

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

Tom Rodriguez-2
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
>>
Reply | Threaded
Open this post in threaded view
|

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

Erik Österlund-2
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.

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

Reply | Threaded
Open this post in threaded view
|

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

Tom Rodriguez-2


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
>>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Erik Österlund-2
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
>>>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

Igor Veresov
In reply to this post by Tom Rodriguez-2
Looks good to me.

igor

> On Jun 21, 2018, at 1:46 PM, Tom Rodriguez <[hidden email]> 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
>>>>>

Reply | Threaded
Open this post in threaded view
|

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

Tom Rodriguez-2
Thanks!

tom

Igor Veresov wrote on 6/21/18 9:37 PM:

> Looks good to me.
>
> igor
>
>> On Jun 21, 2018, at 1:46 PM, Tom Rodriguez <[hidden email]> 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
>>>>>>
>