Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion runtime/gc_base/IndexableObjectAllocationModel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,13 @@ MM_IndexableObjectAllocationModel::initializeIndexableObject(MM_EnvironmentBase
/* Lay out arraylet and arrayoid pointers */
switch (_layout) {
case GC_ArrayletObjectModel::InlineContiguous:
Assert_MM_true(1 == _numberOfArraylets);
/**
* _numberOfArraylets is set to 0 for 0-stride flattened array, and we
* can recognize this case by checking if _dataSize is 0.
* Note there are two cases that _dataSize is 0: 0-stride flattened array or 0-length array
* But the latter is treated as discontiguous and not following this path
*/
dmitripivkine marked this conversation as resolved.
Show resolved Hide resolved
Assert_MM_true((0 == _dataSize) || (1 == _numberOfArraylets));
break;

case GC_ArrayletObjectModel::Discontiguous:
Expand Down
2 changes: 1 addition & 1 deletion runtime/gc_glue_java/ArrayletObjectModel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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))) {
/* Zero sized NUA uses the discontiguous shape */
layout = Discontiguous;
}
Expand Down
8 changes: 7 additions & 1 deletion runtime/gc_glue_java/ArrayletObjectModel.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
alignedSize = MM_Math::roundToSizeofUDATA(size);
if (alignedSize < size) {
alignedSize = UDATA_MAX;
Expand Down Expand Up @@ -478,6 +478,9 @@ class GC_ArrayletObjectModel : public GC_ArrayletObjectModelBase
void* srcData = getDataPointerForContiguous(srcObject);
switch(elementSize)
{
case 0:
break;

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case 1:
// byte/boolean
{
Expand Down
5 changes: 3 additions & 2 deletions runtime/gc_include/ObjectAllocationAPI.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -447,10 +447,11 @@ class MM_ObjectAllocationAPI
VMINLINE j9object_t
inlineAllocateIndexableValueTypeObject(J9VMThread *currentThread, J9Class *arrayClass, uint32_t size, bool initializeSlots = true, bool memoryBarrier = true, bool sizeCheck = true)
{
uintptr_t dataSize = ((uintptr_t)size) * J9ARRAYCLASS_GET_STRIDE(arrayClass);
uintptr_t stride = J9ARRAYCLASS_GET_STRIDE(arrayClass);
uintptr_t dataSize = ((uintptr_t)size) * stride;
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif /* J9VM_ENV_DATA64 */
return inlineAllocateIndexableObjectImpl(currentThread, arrayClass, size, dataSize, validSize, initializeSlots, memoryBarrier);
}
Expand Down
3 changes: 3 additions & 0 deletions runtime/gc_structs/FlattenedContiguousArrayIterator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ class GC_FlattenedContiguousArrayIterator
*/
MMINLINE UDATA getIndex()
{
if (0 == _elementStride) {
return 0;
}
return ((uintptr_t)_scanPtr - (uintptr_t)_basePtr) / _elementStride;
}

Expand Down