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

[Reingruber, Richard]

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.