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

[Doerr, Martin]

Hi Götz,

this change applies cleanly to 11u, 13 and 14.

Yes, all these jdk versions are affected. 
If you agree, I'll push it to 13 and we can request 11u backport afterwards.

Best regards,
Martin


> -----Original Message-----
> From: Lindenmaier, Goetz
> Sent: Montag, 29. Juli 2019 13:09
> To: Reingruber, Richard <richard.reingruber at sap.com>; 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 Richard,
> 
> Nice catch, thanks for fixing this.  Looks good.
> 
> While the cleanups make sense, I was a bit concerned
> whether they complicate downport of the fix to 11.
> As I understand, the bug is there, too?  But the removed
> method/arguments seem not to be used in 11, either.
> 
> Best regards,
>   Goetz.
> 
> > -----Original Message-----
> > From: hotspot-compiler-dev <hotspot-compiler-dev-
> > bounces at openjdk.java.net> On Behalf Of Reingruber, Richard
> > Sent: Friday, July 26, 2019 4:17 PM
> > To: Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-
> > dev at openjdk.java.net
> > Subject: [CAUTION] 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.