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

[Lindenmaier, Goetz]

Yes, please!

Thanks,
  Goetz.

> -----Original Message-----
> From: Doerr, Martin
> Sent: Monday, July 29, 2019 3:23 PM
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 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 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.