RFR(S) 8228618: s390: c1/c2 fail to add metadata relocation in the static call stub

[Doerr, Martin]

Hi Richard,

looks good to me.
 
Thanks,
Martin


> -----Original Message-----
> From: Reingruber, Richard
> Sent: Freitag, 26. Juli 2019 16:17
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-
> dev at openjdk.java.net
> Subject: RE: RFR(S) 8228618: s390: c1/c2 fail to add metadata relocation in the
> static call stub
> 
> Hi Martin,
> 
>   > nativeInst_s390:
>   > I think it'd be nice to use the existing relocType instead of the new
> ExpectedRelocType.
> 
> Sure! Here's the new webrev:
> http://cr.openjdk.java.net/~rrich/webrevs/2019/8228618/webrev.2/
> 
> Thanks for reviewing!
> 
> Richard.
> 
> -----Original Message-----
> From: Doerr, Martin
> Sent: Freitag, 26. Juli 2019 15:48
> To: Reingruber, Richard <richard.reingruber at sap.com>; hotspot-compiler-
> dev at openjdk.java.net
> Subject: RE: RFR(S) 8228618: s390: c1/c2 fail to add metadata relocation in the
> static call stub
> 
> Hi Richard,
> 
> thanks for fixing this bug.
> 
> nativeInst_s390:
> I think it'd be nice to use the existing relocType instead of the new
> ExpectedRelocType.
> 
> Looks good otherwise. I can sponsor this change.
> 
> Best regards,
> Martin
> 
> 
> > -----Original Message-----
> > From: hotspot-compiler-dev <hotspot-compiler-dev-
> > bounces at openjdk.java.net> On Behalf Of Reingruber, Richard
> > Sent: Freitag, 26. Juli 2019 11:31
> > To: hotspot-compiler-dev at openjdk.java.net
> > Subject: RFR(S) 8228618: s390: c1/c2 fail to add metadata
> > relocation in the static call stub
> >
> > Hi,
> >
> > could I please get reviews for the following bugfix on s390:
> >
> > Webrev:
> http://cr.openjdk.java.net/~rrich/webrevs/2019/8228618/webrev/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8228618
> >
> > On s390 the test vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine
> > crashed the vm
> > non-deterministically.
> >
> > Analysis:
> >
> > - c1/c2 generates static call stub without metadata relocation for the
> > instruction L that loads the target Method* T
> >   into Z_method.
> >   (see LIR_Assembler::emit_static_call_stub(),
> > CompiledStaticCall::emit_to_interp_stub(),
> > MacroAssembler::load_const_from_toc())
> >
> > - During call resolution the initialization of T in the metadata pool fails
> silently
> > because no
> >   metadata relocation for L is found.
> >   Note that the load does not load from the metadata pool, but from the
> toc,
> > which is accurately updated.
> >   (see CompiledDirectStaticCall::set_to_interpreted(),
> > NativeMovConstReg::set_data(), relocInfo::update_oop_pool())
> >
> > - T is not marked 'on-stack' during class redefinition, because it is not found
> in
> > the metadata pool of the caller
> >   (see MetadataOnStackMark, nmethod::metadata_do())
> >
> > - Metadata of T (e.g. constant pool) is reclaimed, because T was redefined.
> >
> > - static stub referencing T is executed and VM crashes
> >
> > The actual fix is in MacroAssembler::load_const_from_toc(). There are
> uses
> > apart from the static
> > stub, which had the same issue.
> >
> > I refactored the code a little bit and removed unreachable sections.
> >
> > NativeMovConstReg::set_data() erases type information of the data. We
> > could check for the existence
> > of appropriate relocations, if it didn't do that, but that would require
> changes
> > in shared
> > code.
> >
> > Thanks, Richard.