SIGFPE for flattened array of value type that has no fields #17994

kangyining · 2023-08-21T21:09:00Z

J9ARRAYCLASS_GET_STRIDE() returns the element size, and could return 0 when flattened is enabled and the class is empty.

We should fix this somehow. Currently we hard set the stride to 1 inside division when it is 0.

dmitripivkine · 2023-08-22T12:11:37Z

runtime/gc_glue_java/ArrayletObjectModel.cpp

@@ -94,7 +94,7 @@ GC_ArrayletObjectModel::getArrayletLayout(J9Class* clazz, UDATA dataSizeInBytes,
 	/* CMVC 135307 : when checking for InlineContiguous layout, perform subtraction as adding to dataSizeInBytes could trigger overflow. */
 	if ((largestDesirableSpine == UDATA_MAX) || (dataSizeInBytes <= (largestDesirableSpine - minimumSpineSizeAfterGrowing - contiguousIndexableHeaderSize()))) {
 		layout = InlineContiguous;
-		if(0 == dataSizeInBytes) {
+		if(0 == dataSizeInBytes && 0 < J9ARRAYCLASS_GET_STRIDE(clazz)) {


please:

add extra brackets to not rely on order of operations

add space after "if"

dmitripivkine · 2023-08-22T12:13:10Z

runtime/gc_glue_java/ArrayletObjectModel.hpp

@@ -366,7 +366,7 @@ class GC_ArrayletObjectModel : public GC_ArrayletObjectModelBase
 		uintptr_t stride = J9ARRAYCLASS_GET_STRIDE(clazzPtr);
 		uintptr_t size = numberOfElements * stride;
 		uintptr_t alignedSize = UDATA_MAX;
-		if ((size / stride) == numberOfElements) {
+		if ((0 == stride) || (size / stride) == numberOfElements) {


please add extra brackets around (size / stride) == numberOfElements

dmitripivkine · 2023-08-22T12:20:31Z

runtime/gc_include/ObjectAllocationAPI.hpp

@@ -450,7 +450,7 @@ class MM_ObjectAllocationAPI
 		uintptr_t dataSize = ((uintptr_t)size) * J9ARRAYCLASS_GET_STRIDE(arrayClass);
 		bool validSize = true;
 #if !defined(J9VM_ENV_DATA64)
-		validSize = !sizeCheck || (size < ((uint32_t)J9_MAXIMUM_INDEXABLE_DATA_SIZE / J9ARRAYCLASS_GET_STRIDE(arrayClass)));
+		validSize = !sizeCheck || (0 == J9ARRAYCLASS_GET_STRIDE(arrayClass)) || (size < ((uint32_t)J9_MAXIMUM_INDEXABLE_DATA_SIZE / J9ARRAYCLASS_GET_STRIDE(arrayClass)));


please create another variable for J9ARRAYCLASS_GET_STRIDE(arrayClass) and use it instead of calling macro twice

dmitripivkine · 2023-08-22T12:23:08Z

runtime/gc_structs/FlattenedContiguousArrayIterator.hpp

@@ -91,6 +91,9 @@ class GC_FlattenedContiguousArrayIterator
 	 */
 	MMINLINE UDATA getIndex()
 	{
+		if(0 == _elementStride){


please add spaces after "if" and before "{"

dmitripivkine · 2023-08-22T18:45:13Z

runtime/gc_base/IndexableObjectAllocationModel.cpp

@@ -145,7 +145,7 @@ MM_IndexableObjectAllocationModel::initializeIndexableObject(MM_EnvironmentBase
 	/* Lay out arraylet and arrayoid pointers */
 	switch (_layout) {
 	case GC_ArrayletObjectModel::InlineContiguous:
-		Assert_MM_true(1 == _numberOfArraylets);
+		Assert_MM_true((_dataSize == 0) || (1 == _numberOfArraylets));


We need comment before this assertion explained (_dataSize == 0) for InlineContiguous is possible for 0-stride elements of Flattened Array

also please put constant first (0 == _dataSize)

runtime/gc_base/IndexableObjectAllocationModel.cpp

dmitripivkine · 2023-08-23T13:37:18Z

runtime/gc_glue_java/ArrayletObjectModel.hpp

@@ -637,6 +640,9 @@ class GC_ArrayletObjectModel : public GC_ArrayletObjectModelBase
 			void* destData = getDataPointerForContiguous(destObject);
 			switch(elementSize)
 			{
+			case 0:
+				break;
+


@TobiAjila This function is used for Primitive arrays only and supports element sizes 1, 2, 4, 8 bytes. Can Flattened array be declared Primitive and have different element size (except 0-stride we are adding now)? If so, what size of chunk we should use to copy? We can postpone the implementation and add it in different PR. Currently this function triggers assertion if size of the element is not supported.

If a flattened array is declared as primitive it will only have size 1, 2, 4 or 8.

Long term, it would be nice if we can deal with zero payload arrays. Where we have a header and no body.

dmitripivkine · 2023-08-23T13:42:02Z

runtime/gc_include/ObjectAllocationAPI.hpp

 		bool validSize = true;
 #if !defined(J9VM_ENV_DATA64)
-		validSize = !sizeCheck || (size < ((uint32_t)J9_MAXIMUM_INDEXABLE_DATA_SIZE / J9ARRAYCLASS_GET_STRIDE(arrayClass)));
+		validSize = (!sizeCheck) || (0 == stride) || (size < ((uint32_t)J9_MAXIMUM_INDEXABLE_DATA_SIZE / stride));


I think this change is not necessary due we don't support any 32-bit JVM for Java Next. However code is here and it is reasonable to fix it. So, keep this change.

dmitripivkine · 2023-08-23T13:47:05Z

@kangyining Please update comment and squash commits (except you want to add something else to the change).

Update gc code to support 0-stride flattened array and get rid of division by 0 exceptions This is done by adding manually comparison against 0, might find better alternatives in the future. Related: eclipse-openj9#14027 Signed-off-by: Frank Kang <[email protected]>

dmitripivkine · 2023-08-24T12:34:24Z

Jenkins test sanity,extended xlinuxval jdknext

hangshao0 · 2023-08-24T13:18:43Z

Will the test case be added ? It can be done in a separate PR though.

dmitripivkine · 2023-08-24T14:06:48Z

Will the test case be added ? It can be done in a separate PR though.

Yes, I think test can be added as a separate submission. We can use existed issue #14027 to track further work.
Test has been provided by @hzongaro. As I understand from #14027 (comment) there is extra tuning required to run test properly.

hzongaro · 2023-08-24T18:07:06Z

Will the test case be added ? It can be done in a separate PR though.

I will open a PR to add a test case.

dmitripivkine reviewed Aug 22, 2023

View reviewed changes

hangshao0 added project:valhalla Used to track Project Valhalla related work comp:gc labels Aug 22, 2023

dmitripivkine reviewed Aug 23, 2023

View reviewed changes

runtime/gc_base/IndexableObjectAllocationModel.cpp Show resolved Hide resolved

dmitripivkine reviewed Aug 23, 2023

View reviewed changes

kangyining force-pushed the Issue_14027 branch from d0eb34f to 4f81482 Compare August 23, 2023 20:11

dmitripivkine changed the title ~~WIP: SIGFPE for flattened array of value type that has no fields~~ SIGFPE for flattened array of value type that has no fields Aug 24, 2023

hzongaro mentioned this pull request Aug 29, 2023

Define unit tests involving arrays of empty value types #18041

Merged

1 task

kangyining self-assigned this Aug 30, 2023

dmitripivkine approved these changes Aug 31, 2023

View reviewed changes

dmitripivkine merged commit 3c5614a into eclipse-openj9:master Aug 31, 2023

hzongaro mentioned this pull request Sep 6, 2023

SIGFPE for flattened array of value type that has no fields #14027

Closed

dmitripivkine mentioned this pull request Oct 11, 2023

Add numberOfElements parameter to getArrayletLayout() #18268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SIGFPE for flattened array of value type that has no fields #17994

SIGFPE for flattened array of value type that has no fields #17994

kangyining commented Aug 21, 2023

dmitripivkine Aug 22, 2023

dmitripivkine Aug 22, 2023

dmitripivkine Aug 22, 2023

dmitripivkine Aug 22, 2023

dmitripivkine Aug 22, 2023

dmitripivkine Aug 22, 2023

dmitripivkine Aug 23, 2023

tajila Aug 23, 2023

tajila Aug 23, 2023

dmitripivkine Aug 23, 2023

dmitripivkine commented Aug 23, 2023

dmitripivkine commented Aug 24, 2023

hangshao0 commented Aug 24, 2023

dmitripivkine commented Aug 24, 2023

hzongaro commented Aug 24, 2023

SIGFPE for flattened array of value type that has no fields #17994

SIGFPE for flattened array of value type that has no fields #17994

Conversation

kangyining commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine commented Aug 23, 2023

dmitripivkine commented Aug 24, 2023

hangshao0 commented Aug 24, 2023

dmitripivkine commented Aug 24, 2023

hzongaro commented Aug 24, 2023