[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR (S): 8198909: [Graal] compiler/codecache/stress/UnexpectedDeoptimizationTest.java crashed wi
From:       Tom Rodriguez <tom.rodriguez () oracle ! com>
Date:       2018-06-22 5:19:33
Message-ID: a7379ae9-73ce-e9e9-dc81-a95512630e53 () oracle ! com
[Download RAW message or body]

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 <tom.rodriguez@oracle.com> 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
> > > > > > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]