Z: Value types: Nested field class for aconst_init is not patched correctly in non-SVM AOT compilation

[a7ehuo]

Value type bytecode aconst_init is represented as newvalue jitNewValue in ILGen and later lowered as new in CG. If a field of VT is also a VT, we will generate newvalue jitNewValue for the field instance as well.

I'm testing enabling AOT compilation for aconst_init, using the following test code, I noticed the field class SingleFieldPrimitive for NestedFieldsPrimitive is not patched correctly on AOT load on Z.

primitive public class NestedFieldsPrimitive {
    public final SingleFieldPrimitive f1;

    public NestedFieldsPrimitive() {
        this.f1 = new SingleFieldPrimitive();
    }

    public NestedFieldsPrimitive(int val) {
        this.f1 = new SingleFieldPrimitive(val);
    }
}
public class TestAOTAConstInit2 {

   public static NestedFieldsPrimitive _nfp;

   public static void test() {
      _nfp = NestedFieldsPrimitive.default; // aconst_init

   public static void main(String[] args) {
      test();
      test();
   }
}

n7n       treetop                                                                             [     0x3ff88201770] bci=[-1,0,-] rc=0 vc=0 vn=- li=- udi=- nc=1
n6n         newvalue  jitNewValue[#101  helper Method] [flags 0x400 0x0 ] (Identityless sharedMemory )  [     0x3ff88201720] bci=[-1,0,-] rc=2 vc=0 vn=- li=- udi=- nc=2 flg=0x2000
n4n           loadaddr  SingleFieldPrimitive[#383  Static] [flags 0x18307 0x0 ]               [     0x3ff88201680] bci=[-1,0,-] rc=1 vc=0 vn=- li=- udi=- nc=0
n5n           iconst 0                                                                        [     0x3ff882016d0] bci=[-1,0,-] rc=1 vc=0 vn=- li=- udi=- nc=0
n9n       treetop                                                                             [     0x3ff88201810] bci=[-1,0,-] rc=0 vc=0 vn=- li=- udi=- nc=1
n8n         newvalue  jitNewValue[#101  helper Method] [flags 0x400 0x0 ] (Identityless sharedMemory )  [     0x3ff882017c0] bci=[-1,0,-] rc=2 vc=0 vn=- li=- udi=- nc=2 flg=0x2000
n3n           loadaddr  NestedFieldsPrimitive[#382  Static] [flags 0x18307 0x0 ]              [     0x3ff88201630] bci=[-1,0,-] rc=1 vc=0 vn=- li=- udi=- nc=0
n6n           ==>newvalue

In AOT compilation, I'm guessing 0x3ff882e0ec0 loads class SingleFieldPrimitive. On AOT load, it's incorrectly patched as TestAOTAConstInit2.

---- AOT Compilation ----

------------------------------
 n7n      (  0)  treetop                                                                              [     0x3ff88201770] bci=[-1,0,-] rc=0 vc=80 vn=- li=2 udi=- nc=1
 n6n      (  3)    new  jitNewValue[#101  helper Method] [flags 0x400 0x0 ] (in &GPR_0034) (highWordZero Unsigned X!=0 skipZeroInit allocationCanBeRemoved sharedMemory )  [     0x3ff88201720] bci=[-1,0,-] rc=3 vc=80 vn=- li=2 udi=- nc=1 flg=0xc004
 n4n      (  0)      loadaddr  SingleFieldPrimitive[#383  Static] [flags 0x18307 0x0 ] (in GPR_0032)  [     0x3ff88201680] bci=[-1,0,-] rc=0 vc=80 vn=- li=2 udi=- nc=0
------------------------------

 [     0x3ff882e0ec0]	                       LGRL    GPR_0032, 0x000003FF882E0D30	
 [     0x3ff882e1190]	                       LGFI    GPR_0033,16	
 [     0x3ff882e1360]	                       AG      GPR_0033,#389 96(GPR13)
 [     0x3ff882e14f0]	                       CLG     GPR_0033,#390 104(GPR13)
 [     0x3ff882ef7a0]	                       Label L0022:	# (Start of internal control flow)	
 [     0x3ff882e15c0]	                       BRC     BL(0x2), Outlined Label L0016	
 [     0x3ff882e1760]	                       LG      &GPR_0034,#391 96(GPR13)
 [     0x3ff882e18f0]	                       STG     GPR_0033,#392 96(GPR13)
 [     0x3ff882ef5e0]	                       ST      GPR_0032,#393 0(&GPR_0034)


     0x3ff6af0009c 0000001c [     0x3ff882e0670] 17 00                                             XR      GPR0,GPR0	
     0x3ff6af0009e 0000001e [     0x3ff882e0ec0] c4 08 00 00 00 a1                                 LGRL    GPR0, 0x000003FF882E0D30, targetAddr=0x000003FF6AF001E0 (offset=0x0000000000000142)	
     0x3ff6af000a4 00000024 [     0x3ff882e1190] c0 11 00 00 00 10                                 LGFI    GPR1,16	
     0x3ff6af000aa 0000002a [     0x3ff882e1360] e3 10 d0 60 00 08                                 AG      GPR1,#389 96(GPR13)
     0x3ff6af000b0 00000030 [     0x3ff882e14f0] e3 10 d0 68 00 21                                 CLG     GPR1,#390 104(GPR13)

---- AOT Load ----

0x3ffdde7f09c {TestAOTAConstInit2.test} +28                       1700         xr        %r0, %r0
0x3ffdde7f09e {TestAOTAConstInit2.test} +30                       c408000000a1 lgrl      %r0, 0x3ffdde7f1e0 J9Class - {TestAOTAConstInit2} //<---- wrong class!! It should be SingleFieldPrimitive
0x3ffdde7f0a4 {TestAOTAConstInit2.test} +36                       c01100000010 lgfi      %r1, 0x10 
0x3ffdde7f0aa {TestAOTAConstInit2.test} +42                       e310d0600008 ag        %r1, 0x60(%r13)  J9VMThread.heapAlloc
0x3ffdde7f0b0 {TestAOTAConstInit2.test} +48                       e310d0680021 clg       %r1, 0x68(%r13)  J9VMThread.heapTop


(kca) what (0x3ffdde7f1e0)
0x3ffdde7f1e0: 0x0000000000240a00 J9Class - {TestAOTAConstInit2}

0623_NestedFieldsPrimitive_AOT.zip

[a7ehuo]

There is a similar issue on X86 but I'm not sure if Z is the same root cause.

For X86-64, it creates a relocation record TR_ClassAddress for the romClass of SingleFieldPrimitive in omr/compiler/x/amd64/codegen/OMRMemoryReference.cpp for the class load instruction. The TR_ClassAddress relocation record requires cpIndex for the class symref, but when the field of VT is VT, the cpIndex for the field class SingleFieldPrimitive is unknown, so it is set as 0 in ILGen. Because cpindex is incorrect, the patched address is also incorrect.

[r30shah]

Posting a link of similar code for Z that @a7ehuo has referenced from x86 in previous commit in [1] and [2]. We create similar relocation record. Relocating the ClassAddress in non-SVM AOT would be similar on Z and x86 I think. May be @dsouzai / @jdmpapin can share more light on what will be needed to get SingleFieldPrimitive relocated.

[1]. https://github.com/eclipse/omr/blob/a57ad601d6664d97958c59884c4d34a0ce2f960a/compiler/z/codegen/ConstantDataSnippet.cpp#L93-L120
[2].

openj9/runtime/compiler/z/codegen/S390AOTRelocation.cpp

Lines 53 to 73 in 7b717f7

	if (_reloType==TR_ClassAddress)
	{
	if (comp->getOption(TR_UseSymbolValidationManager))
	{
	TR_OpaqueClassBlock *clazz = (TR_OpaqueClassBlock*)(*((uintptr_t*)cursor));
	TR_ASSERT_FATAL(clazz, "TR_ClassAddress relocation : cursor = %x, clazz can not be null", cursor);
	cg->addExternalRelocation(new (cg->trHeapMemory()) TR::ExternalRelocation(cursor,
	(uint8_t *)clazz,
	(uint8_t *) TR::SymbolType::typeClass,
	TR_SymbolFromManager,
	cg),
	file, line, node);
	}
	else
	{
	*((uintptr_t*)cursor)=fej9->getPersistentClassPointerFromClassPointer((TR_OpaqueClassBlock*)(*((uintptr_t*)cursor)));
	cg->addExternalRelocation(new (cg->trHeapMemory()) TR::ExternalRelocation(cursor, (uint8_t *) _symbolReference, (uint8_t *)_inlinedSiteIndex, TR_ClassAddress, cg),
	file, line, node);
	}
	}

[a7ehuo]

Thanks @r30shah for the above information! Now I have a better understanding of it on Z. I think Z has the same issue as X86 [1]. One potential workaround we're considering is to abort AOT compilation in ILGen if it's non-SVM AOT and if the VT has a field that is also a VT. Since in this case, cpindex for the field is unknown in ILGen.

[1]
The instruction 0x3ff6af0009e is loadaddr SingleFieldPrimitive. It has target address 0x000003FF6AF001E0 which is a data snippet. It has a relocation record TR_ClassAddress with PC offset 0160 (or offset 01d0). On AOT load, (offset 1d0) is patched as TestAOTAConstInit2 because cpindex is 0.

n4n      (  0)      loadaddr  SingleFieldPrimitive[#383  Static] [flags 0x18307 0x0 ] (in GPR_0032)  [     0x3ff88201680] bci=[-1,0,-] rc=0 vc=80 vn=- li=2 udi=- nc=0
...
 [     0x3ff882e0ec0]	                       LGRL    GPR_0032, 0x000003FF882E0D30	
...
     0x3ff6af0009e 0000001e [     0x3ff882e0ec0] c4 08 00 00 00 a1                                 LGRL    GPR0, 0x000003FF882E0D30, targetAddr=0x000003FF6AF001E0 (offset=0x0000000000000142)	

TR_ClassAddress (40)                /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/omr/compiler/z/codegen/ConstantDataSnippet.cpp   114      01d0       0160 000003FF88201680
TargetAddress1:000003FF8824D3C0,  TargetAddress2:FFFFFFFFFFFFFFFF
...
        6a5ff14c  34   TR_ClassAddress (40)           2     Abs       01d0 
 Address Relocation (TR_ClassAddress): inlinedIndex = -1, constantPool = 000003FF496D0868, CPI = 0, flags = 0

000003FF6AF001E0 00000160                                                 Snippet Label L0018:		; Constant Data Snippet
     0x3ff6af001e0 00000160                      00 00 03 ff 49 6d 43 e8    DC   	0x00000000007fde00

----- AOT Load -------

TR_ClassAddress 000003FFDDE7C92C
	size 22 type 40 flags 0 reloFlags 0
	inlined site index FFFFFFFFFFFFFFFF
	constant pool 000003FF496D0868
	cpIndex 0000000000000000. //<------ invalid CPIndex
	reloLocation: from 000003FFDDE7C94C at 000003FFDDE7F1E0 (offset 1d0)
		computeNewConstantPool: newCP 0000000000240BC0
	computeNewClassObject: resolvedClass 0000000000240A00
	computeNewClassObject: resolvedClassName TestAOTAConstInit2 //<------ patched wrong class 

[jdmpapin]

the cpIndex for the field class SingleFieldPrimitive is unknown, so it is set as 0 in ILGen.

The usual value indicating an unknown or nonexistent constant pool index is -1, not 0. If -1 were passed, then at least there would have been an assertion from findOrCreateClassSymbol() stating that in non-SVM AOT it can't handle a class with an unknown CP index loaded by a non-bootstrap loader

[jdmpapin]

Aborting non-SVM AOT compilation when this situation arises is the most expedient fix. I don't believe there is currently any good way for non-SVM AOT to relocate the class in this situation. At least there's no general way. An exception could be made for a class defined by the bootstrap class loader (in which case the TR_ClassAddress will automatically be changed into a TR_ArbitraryClassAddress). We could also scrounge around for a constant pool entry that happens to have resolved to the needed class, and indicate that constant pool entry when it's available, but I really don't think we should go down that road

[jdmpapin]

It's also possible to try to make TR_ArbitraryClassAddress more general, but IMO any such effort would be better spent toward requiring SVM for all AOT compilations

[a7ehuo]

Thanks for the clarification @jdmpapin! I think I'll go with aborting AOT compilation in ILGen if it's non-SVM AOT + the VT has a field that is also a VT. In inlining allocation, different types of relocation record are created in different platforms: TR_ClassAddress on X86 and Z; TR_ConstantPool on Power. It wouldn't be trivial to make TR_ArbitraryClassAddress work on all platforms when the issue only exists in this specific case.

The usual value indicating an unknown or nonexistent constant pool index is -1

Thanks for pointing it out. I'll make sure to pass -1 when CPIndex is unknown in this case.

[a7ehuo]

The usual value indicating an unknown or nonexistent constant pool index is -1, not 0

I found another thing that's interesting regarding to cpIndex as -1 for arbitrary classes. It's still the case where a VT has a VT field.

Two validation records are created in ILGen for the same class NestedFieldsPrimitive. The first storeValidationRecordIfNecessary has correct cpIndex 3 and the type is TR_ValidateClass. The second storeValidationRecordIfNecessary has cpIndex 0 and type is TR_ValidateArbitraryClass [1].

The second one is triggered when we try to build the fields for the VT [2]. This second validation record is triggered in TR_ResolvedRelocatableJ9Method::validateArbitraryClass which specifically sets cpIndex as 0.

openj9/runtime/compiler/env/j9method.cpp

Lines 1202 to 1204 in 07be89b

	TR_ResolvedRelocatableJ9Method::validateArbitraryClass(TR::Compilation *comp, J9Class *clazz)
	{
	return storeValidationRecordIfNecessary(comp, cp(), 0, TR_ValidateArbitraryClass, ramMethod(), clazz);

@jdmpapin I'm wondering the following two question. I think I should double check with you.
(1) Is setting 0 in TR_ResolvedRelocatableJ9Method::validateArbitraryClass a potential issue? I guess since cpindex is not used by TR_ValidateArbitraryClass, it wouldn't be an issue?
(2) Is having two validation records for the same class with different validation types a potential issue? I looked at the AOT load log. It looks to me both validation records will materialize the same class NestedFieldsPrimitive. It might not be an issue.

[1]

   0: JBaconst_init
storeValidationRecordIfNecessary: // <--- (1)
   constantPool 00000000001D34C0 cpIndex 3
   reloKind 39 isStatic 0
   method 00000000001D3558 from class 00000000001D3300 TestAOTAConstInit
   definingClass 00000000001D3900
   definingClass name NestedFieldsPrimitive
   Created new AOT class info 00007FFFD4159130
...
storeValidationRecordIfNecessary:  // <--- (2)
   constantPool 00000000001D34C0 cpIndex 0
   reloKind 50 isStatic 0
   method 00000000001D3558 from class 00000000001D3300 TestAOTAConstInit
   definingClass 00000000001D3900
   definingClass name NestedFieldsPrimitive

[2]

(1) storeValidationRecordIfNecessary
(gdb) bt
#0  TR_ResolvedRelocatableJ9Method::storeValidationRecordIfNecessary (this=<optimized out>, comp=0x7fffd4109000, 
    constantPool=0x1d34c0, cpIndex=3, reloKind=TR_ValidateClass, ramMethod=0x1d3558, definingClass=<optimized out>)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/j9method.cpp:1407
#1  0x00007fffef3934a2 in TR_ResolvedRelocatableJ9Method::validateClassFromConstantPool (this=0x7fffd52059a0, 
    comp=0x7fffd4109000, clazz=0x1d3900, cpIndex=3, reloKind=TR_ValidateClass)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/j9method.cpp:1237
#2  0x00007fffef398893 in TR_ResolvedRelocatableJ9Method::getClassFromConstantPool (this=0x7fffd52059a0, 
    comp=0x7fffd4109000, cpIndex=3, returnClassForAOT=<optimized out>)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/j9method.cpp:1199
#3  0x00007fffef443c81 in TR_J9ByteCodeIlGenerator::genAconst_init (this=0x7fffd412b320, cpIndex=3)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/ilgen/Walker.cpp:6632
#4  0x00007fffef445668 in TR_J9ByteCodeIlGenerator::walker (this=this@entry=0x7fffd412b320, prevBlock=prevBlock@entry=0x0)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/ilgen/Walker.cpp:557

(2) storeValidationRecordIfNecessary

(gdb) bt
#0  TR_ResolvedRelocatableJ9Method::storeValidationRecordIfNecessary (this=<optimized out>, comp=0x7fffd4109000, 
    constantPool=0x1d34c0, cpIndex=0, reloKind=TR_ValidateArbitraryClass, ramMethod=0x1d3558, definingClass=<optimized out>)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/j9method.cpp:1407
#1  0x00007fffef3932b7 in TR_ResolvedRelocatableJ9Method::validateArbitraryClass (this=0x7fffd52059a0, comp=0x7fffd4109000, 
    clazz=0x1d3900) at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/j9method.cpp:1244
#2  0x00007fffef3d649a in TR_J9SharedCacheVM::getInstanceFieldOffset (this=0x7fffd0007990, classPointer=0x1d3900, 
    fieldName=0x7fffd5201e10 "f1", fieldLen=2, sig=0x7fffd5201e20 "QSingleFieldPrimitive;", sigLen=22, options=524288)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/VMJ9.cpp:8519
#3  0x00007fffef3bd022 in TR_J9VMBase::getInstanceFieldOffset (this=<optimized out>, clazz=<optimized out>, 
    fieldName=<optimized out>, fieldLen=<optimized out>, sig=<optimized out>, sigLen=<optimized out>)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/VMJ9.cpp:6627
#4  0x00007fffef3876f1 in TR_VMField::TR_VMField (this=0x7fffd5201de0, comp=0x7fffd4109000, aClazz=0x1d3900, 
    fieldShape=<optimized out>, allocKind=stackAlloc)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/j9fieldsInfo.cpp:74
#5  0x00007fffef3877dd in TR_VMFieldsInfo::buildField (this=this@entry=0x7fffd52005d0, aClazz=aClazz@entry=0x1d3900, 
    fieldShape=0x7fffb56c46f0)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/j9fieldsInfo.cpp:205
#6  0x00007fffef387886 in TR_VMFieldsInfo::collectFieldInfo (this=this@entry=0x7fffd52005d0, aClazz=aClazz@entry=0x1d3900)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/j9fieldsInfo.cpp:189
#7  0x00007fffef387985 in TR_VMFieldsInfo::TR_VMFieldsInfo (this=0x7fffd52005d0, comp=0x7fffd4109000, aClazz=0x1d3900, 
    buildFields=<optimized out>, allocKind=<optimized out>)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/j9fieldsInfo.cpp:112
#8  0x00007fffef382245 in J9::ClassEnv::enumerateFields (this=<optimized out>, region=..., 
    opaqueClazz=opaqueClazz@entry=0x1d3900, comp=comp@entry=0x7fffd4109000)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/env/J9ClassEnv.cpp:614
#9  0x00007fffef6e4e62 in OMR::Compilation::typeLayout (this=0x7fffd4109000, clazz=clazz@entry=0x1d3900)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/omr/compiler/compile/OMRCompilation.cpp:2775
#10 0x00007fffef44390f in TR_J9ByteCodeIlGenerator::genAconst_init (this=0x7fffd412b320, valueTypeClass=0x1d3900, 
    cpIndex=<optimized out>)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/ilgen/Walker.cpp:6693
#11 0x00007fffef443ccb in TR_J9ByteCodeIlGenerator::genAconst_init (this=<optimized out>, cpIndex=<optimized out>)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/ilgen/Walker.cpp:6638
#12 0x00007fffef445668 in TR_J9ByteCodeIlGenerator::walker (this=this@entry=0x7fffd412b320, prevBlock=prevBlock@entry=0x0)
    at /root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/openj9/runtime/compiler/ilgen/Walker.cpp:557

[jdmpapin]

It's redundant, but it should be fine. For these accesses generated by ilgen, the class came from the constant pool, and the class from constant pool validation (i.e. the first validation) will check on load that the constant pool entry is resolved to a class with the expected layout*

The arbitrary class validation should usually find the same class. It's possible for it to fail to find a loader or find the wrong loader, and then as a result fail to find the class or (I think) even find a different class. If it fails to find a class, or if it finds a class with a different layout,* then I'm pretty sure the load will just fail. This would be a spurious failure, since we're only really relying on the class as found from the constant pool. If it finds a different class that happens to also match the expected layout, then non-SVM AOT won't notice the discrepancy, but again it's irrelevant to the offsets for these ilgen-generated loads

There are some (pretty contrived but) possible scenarios in which a non-SVM AOT compilation could get hold of a single class from multiple sources, reason about that class during compilation in a way that implicitly assumes that those sources will all agree at load time, only for them to actually disagree at load time, resulting in a discrepancy of the kind that I mentioned above. I don't know of a straightforward way to prevent such an issue in general in non-SVM AOT - or rather, SVM is the straightforward way to prevent it - but I don't think that such problems are necessarily any worse for value types than for general classes

@dsouzai, anything to add or correct?

*The fine print: Really, the validations check that the class chain matches. With flattening, if we haven't already, we'll have to make sure that the class chain also specifies which fields are flattened and includes the class chains of (transitively) all of the types of those fields

[dsouzai]

@dsouzai, anything to add or correct?

Yeah, just regarding:

*The fine print: Really, the validations check that the class chain matches. With flattening, if we haven't already, we'll have to make sure that the class chain also specifies which fields are flattened and includes the class chains of (transitively) all of the types of those fields

We haven't added the notion of flattened fields in the class chain yet, so that's something we need to think about and design. I had spent some time thinking about this when this subtlety was first brought up a year or more ago, and I have a vague recollection of a relatively efficient way of handling the class chain validation of a flattened type but I need to see if I can dig up what that idea was.