SIGFPE for flattened array of value type that has no fields

[hzongaro]

Allocating an array of a value type that has no fields results in an exception for a division by zero if array element flattening is enabled. This only happens with JIT-compiled code, but I don't believe the problem lies in the JIT-compiled code itself.

The problem can be reproduced using this test:

Empty.java

public primitive class Empty {
}

EmptyArr.java

public class EmptyArr {
    public static final void sub() {
        Empty[] arr = new Empty[4];

        for (int i = 0; i < arr.length; i++) {
            System.out.println(arr[0] == arr[i]);
        }
    }

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

with this command invocation (empty.zip is attached)

java -Xjit:count=1,disableAsyncCompilation -XX:+EnableArrayFlattening -XX:ValueTypeFlatteningThreshold=999999 -Xverify:none -cp empty.zip EmptyArr

Following is the call stack from gdb:

Thread 2 "main" received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7ffff6f7e700 (LWP 4212)]
0x00007fffee883c6e in GC_ArrayletObjectModel::getDataSizeInBytes (numberOfElements=4, clazzPtr=<optimized out>, this=<optimized out>)
    at /root/hostdir/value-types/openj9-openjdk-jdk/openj9/runtime/gc_glue_java/ArrayletObjectModel.hpp:369
369                     if ((size / stride) == numberOfElements) {
(gdb) p stride
$1 = 0
(gdb) where
#0  0x00007fffee883c6e in GC_ArrayletObjectModel::getDataSizeInBytes (numberOfElements=4, clazzPtr=<optimized out>, this=<optimized out>)
    at /root/hostdir/value-types/openj9-openjdk-jdk/openj9/runtime/gc_glue_java/ArrayletObjectModel.hpp:369
#1  MM_IndexableObjectAllocationModel::MM_IndexableObjectAllocationModel (this=0x7ffff6f7d760, env=0x7ffff005e538, clazz=0x1b7c00, 
    numberOfIndexedFields=4, allocateObjectFlags=<optimized out>)
    at /root/hostdir/value-types/openj9-openjdk-jdk/openj9/runtime/gc_base/IndexableObjectAllocationModel.hpp:85
#2  0x00007fffee880e7c in J9AllocateIndexableObjectNoGC (vmThread=0x16f00, clazz=0x1b7c00, numberOfIndexedFields=4, allocateFlags=33)
    at /root/hostdir/value-types/openj9-openjdk-jdk/openj9/runtime/gc_modron_startup/mgcalloc.cpp:349
#3  0x00007fffef53ab7f in fast_jitANewArrayImpl (nonZeroTLH=false, size=4, elementClass=0x1b7a00, currentThread=0x16f00)
    at /root/hostdir/value-types/openj9-openjdk-jdk/openj9/runtime/codert_vm/cnathelp.cpp:1092
#4  fast_jitANewArray (currentThread=0x16f00, size=4, elementClass=0x1b7a00)
    at /root/hostdir/value-types/openj9-openjdk-jdk/openj9/runtime/codert_vm/cnathelp.cpp:3516
#5  0x00007fffef54aef6 in jitANewArray ()
    at /root/hostdir/value-types/openj9-openjdk-jdk/build/linux-x86_64-server-release/vm/runtime/codert_vm/xnathelp.s:1369

[dmitripivkine]

looks like J9ARRAYCLASS_GET_STRIDE(clazzPtr) returns 0
@tajila

	MMINLINE uintptr_t
	getDataSizeInBytes(J9Class *clazzPtr, uintptr_t numberOfElements)
	{	
		uintptr_t stride = J9ARRAYCLASS_GET_STRIDE(clazzPtr);
		uintptr_t size = numberOfElements * stride;
		uintptr_t alignedSize = UDATA_MAX;
		if ((size / stride) == numberOfElements) {
			alignedSize = MM_Math::roundToSizeofUDATA(size);
			if (alignedSize < size) {
				alignedSize = UDATA_MAX;
			}
		}
		return alignedSize;
	}

[tajila]

@dmitripivkine Yes, if the value type has no fields then the stride would be zero.

Ideally in this case we would just allocated an array object with a header and no elements if the the array is flattened. Is this possible?

If so, we would need to augment this check to handle the zero case.

[hangshao0]

If so, we would need to augment this check to handle the zero case.

Looks like it is easier to do the zero check in the GC code.

[dmitripivkine]

If so, we would need to augment this check to handle the zero case.

Looks like it is easier to do the zero check in the GC code.

strongly disagree, at least with current model J9ARRAYCLASS_GET_STRIDE() is used to get size of the element of (any) array. And when number of the elements in array might be 0, element size should not.

[dmitripivkine]

@dmitripivkine Yes, if the value type has no fields then the stride would be zero.

Ideally in this case we would just allocated an array object with a header and no elements if the the array is flattened. Is this possible?

If so, we would need to augment this check to handle the zero case.

Should it be just 0-length array? If not (and stride must be zero) we should review array model (again) to have this case handled properly

[dmitripivkine]

Well, I am not sure iteration of 0-length Flattened array is going to work "as-is" either

[a7ehuo]

Not sure if it'd be related, I see this crash happens with -Xgcpolicy:gencon. -Xgcpolicy:optthruput is fine.

[dmitripivkine]

@tajila In general conceptually if we allow array element size be 0 is it required "array of N elements of 0-size" to be N != 0 (or only trivial case N == 0) ?
Also to have 0 element size is it really language requirement (and we have no choice to support it in GC somehow) or is it current VM implementation detail?

[tajila]

@tajila In general conceptually if we allow array element size be 0 is it required "array of N elements of 0-size" to be N != 0 (or only trivial case N == 0) ?
Also to have 0 element size is it really language requirement (and we have no choice to support it in GC somehow) or is it current VM implementation detail?

The JVM spec allows arrays to be of length zero. Ie. byte arr = new byte[0]; is legal.

In the case of ValueTypes, if it is flattened and there are no fields, the payload (stride) is effectively zero, meaning that there is no data to put in the array element. This is where the requirement for zero element size arrays come from.

If the array type has no fields and is flattened we still need to be able to do:

EmptyArr arr = new EmptyArr[10];

EmptyArr element = arr[5]; //cant throw NPE or AIOBE

This means even for zero element size arrays the length must be accurate.

Given that the following is an overflow check for the aligned size.

		if ((size / stride) == numberOfElements) {
			alignedSize = MM_Math::roundToSizeofUDATA(size);
			if (alignedSize < size) {
				alignedSize = UDATA_MAX;
			}
		}

We know that zero element sized length arrays will never overflow, so the check is not needed in that case.

The alternative is to give a zero element sized array a stride of 1, but that will incur overhead in the J9AllocateIndexableObject* calls and unnecessary footprint growth.

[dmitripivkine]

We know that zero element sized length arrays will never overflow, so the check is not needed in that case.

Just for the record it is easy to fix this particular piece of code. However there are dozen more places where this macro is used. So it is more a general indexable object problem - zero element size arrays were not allowed before.
If language required such support (and looks like it is) we need to review code from this point of view

[dmitripivkine]

@0xdaryl FYI - support for arrays with zero element size

[dmitripivkine]

I am going to review GC code and make changes to support element size 0 arrays

[kangyining]

Test again. Cannot reproduce this issue on my side.

[hzongaro]

@kangyining, I had to modify the test case slightly in order to reproduce the failure, as some JIT optimizations resulted in the array allocation being optimized away. You should be able to reproduce the failure with the attached issue14027.zip, using the following command invocation:

java -Xjit:count=1,disableAsyncCompilation -XX:+EnableArrayFlattening -XX:ValueTypeFlatteningThreshold=999999 -cp issue14027.zip EmptyArr

[dmitripivkine]

GC part should be completed for now. Please let us know if something is missed.
There is an example of the instance in the heap class size padding

0xFF0C0138 :  0020c100 00000004 00000000 00000000 [ .......... ..... ]

Padding is required due minimum object size 16 bytes.

I think we need to improve DDR support for 0-stride arrays, currently it is decoded as [L somehow:

> !j9object 0x00000000FF0C0138
!J9IndexableObject 0x00000000FF0C0138 {
    struct J9Class* clazz = !j9arrayclass 0x20C100   // [L
    Object flags = 0x00000000;
    U_32 size = 0x00000004;
        [0] = !j9object 0xff0c0138 [0]
        [1] = !j9object 0xff0c0138 [1]
        [2] = !j9object 0xff0c0138 [2]
        [3] = !j9object 0xff0c0138 [3]
}

[hangshao0]

It could be helpful if the core file is attached here (or upload it to box and paste a link here) for the possible DDR format issue.

[dmitripivkine]

It could be helpful if the core file is attached here (or upload it to box and paste a link here) for the possible DDR format issue.

uploaded to https://ibm.box.com/s/m4mamghl6zh81u9jnc1q6c7mrs17h2im

[hangshao0]

It seems that DDR always display [L for arrays. I tried on a non-Valhalla build:

The object is Object[]:

!j9object 0x00000007FFF75038
!J9IndexableObject 0x00000007FFF75038 {
    struct J9Class* clazz = !j9arrayclass 0x59600   // [L
    Object flags = 0x00000000;
    U_32 size = 0x00000003;
        [0] = null
        [1] = null
        [2] = null
}

The object is String[]:

!j9object 0x00000007FFF75050
!J9IndexableObject 0x00000007FFF75050 {
    struct J9Class* clazz = !j9arrayclass 0x5CB00   // [L
    Object flags = 0x00000000;
    U_32 size = 0x00000002;
        [0] = null
        [1] = null
}

[dmitripivkine]

I am not sure last statement is accurate, this is example from the same core for Byte Array:

> !j9object 0xFF000458
!J9IndexableObject 0x00000000FF000458 {
    struct J9Class* clazz = !j9arrayclass 0x4BD00   // [B
    Object flags = 0x00000000;
    U_32 size = 0x00000011;
	[0] =  53, 0x35
	[1] =  46, 0x2e
	[2] =  52, 0x34
	[3] =  46, 0x2e
	[4] =  48, 0x30
	[5] =  45, 0x2d
	[6] =  49, 0x31
	[7] =  53, 0x35
	[8] =  54, 0x36
	[9] =  45, 0x2d
	[10] = 103, 0x67
	[11] = 101, 0x65
	[12] = 110, 0x6e
	[13] = 101, 0x65
	[14] = 114, 0x72
	[15] = 105, 0x69
To print entire range: !j9indexableobject 0x00000000FF000458 0 17
}

[L should be used for References arrays only.

[hangshao0]

[L should be used for References arrays only.

Yes, I should mention that is for all reference types on non-valhalla builds. The Q type descriptor is going to be removed from Valhalla, and all value types will still be L type in the upcoming spec. For now I am not worried much about displaying [L here.

The array class names are from:

openj9/runtime/vm/romclasses.c

Lines 30 to 38 in 022a2a4

	#define OBJECT_ARRAY_NAME "[L"
	#define BOOLEAN_ARRAY_NAME "[Z"
	#define CHAR_ARRAY_NAME "[C"
	#define FLOAT_ARRAY_NAME "[F"
	#define DOUBLE_ARRAY_NAME "[D"
	#define BYTE_ARRAY_NAME "[B"
	#define SHORT_ARRAY_NAME "[S"
	#define INT_ARRAY_NAME "[I"
	#define LONG_ARRAY_NAME "[J"

If we want, we could potentially add a new name for flattened value type array (which may imply a new romclass for flattened value type array needs to be generated).

[hangshao0]

Since the fix and test have been merged, @hzongaro can this be closed ?

[hzongaro]

Fixed under pull request #17994