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.

Sign up for GitHub

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

Jump to bottom

Correctly handle primitive VTs in System.arraycopy #17048

Merged
merged 1 commit into from
May 17, 2023
Merged

Correctly handle primitive VTs in System.arraycopy #17048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 37 additions & 4 deletions runtime/vm/BytecodeInterpreter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,10 +1195,43 @@ class INTERPRETER_CLASS
_currentThread->tempSlot = (UDATA)invalidIndex;
rc = THROW_AIOB;
} else {
I_32 value = VM_ArrayCopyHelpers::referenceArrayCopy(_currentThread, srcObject, srcStart, destObject, destStart, elementCount);
if (-1 != value) {
buildInternalNativeStackFrame(REGISTER_ARGS);
rc = THROW_ARRAY_STORE;
#if defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES)
J9Class *srcClazz = J9OBJECT_CLAZZ(_currentThread, srcObject);
J9Class *destClazz = J9OBJECT_CLAZZ(_currentThread, destObject);
J9Class *srcComponentClass = ((J9ArrayClass *)srcClazz)->componentType;
J9Class *destComponentClass = ((J9ArrayClass *)destClazz)->componentType;

if (J9_IS_J9CLASS_FLATTENED(srcClazz) && J9_IS_J9CLASS_FLATTENED(destClazz) && J9_ARE_NO_BITS_SET(srcComponentClass->classFlags, J9ClassHasReferences) && J9_ARE_NO_BITS_SET(destComponentClass->classFlags, J9ClassHasReferences)) {
if (srcClazz == destClazz) {
UDATA elementSize = J9ARRAYCLASS_GET_STRIDE(srcClazz);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment this only works for contiguous flattened arrays for now as GC does not support the discontiguous case.

Copy link
Author

Choose a reason for hiding this comment

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

added in 3eecee5

/* This only works for contiguous flattened arrays, since discontiguous flattened arrays are not supported by GC */
VM_ArrayCopyHelpers::primitiveArrayCopy(_currentThread, srcObject, srcStart * elementSize, destObject, destStart * elementSize, elementSize * elementCount, 0);
} else {
rc = THROW_ARRAY_STORE;
}
} else if (J9_IS_J9CLASS_FLATTENED(srcClazz) || J9_IS_J9CLASS_FLATTENED(destClazz) || J9_IS_J9CLASS_PRIMITIVE_VALUETYPE(destComponentClass)) {
/* VM_ArrayCopyHelpers::referenceArrayCopy cannot handle flattened arrays or null elements being copied into arrays of primitive value types, so for those cases use copyFlattenableArray instead */
buildGenericSpecialStackFrame(REGISTER_ARGS, 0);
updateVMStruct(REGISTER_ARGS);
I_32 value = VM_ValueTypeHelpers::copyFlattenableArray(_currentThread, _objectAccessBarrier, _objectAllocate, srcObject, destObject, srcStart, destStart, elementCount);
VMStructHasBeenUpdated(REGISTER_ARGS);
restoreGenericSpecialStackFrame(REGISTER_ARGS);

if (-1 == value) {
buildInternalNativeStackFrame(REGISTER_ARGS);
rc = THROW_ARRAY_STORE;
} else if (-2 == value) {
buildInternalNativeStackFrame(REGISTER_ARGS);
rc = THROW_NPE;
}
} else
#endif /* defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES) */
{
I_32 errorIndex = VM_ArrayCopyHelpers::referenceArrayCopy(_currentThread, srcObject, srcStart, destObject, destStart, elementCount);
if (-1 != errorIndex) {
buildInternalNativeStackFrame(REGISTER_ARGS);
rc = THROW_ARRAY_STORE;
}
}
}
}
Expand Down
95 changes: 95 additions & 0 deletions runtime/vm/ValueTypeHelpers.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,101 @@ class VM_ValueTypeHelpers {
return value;
}

/**
* Copies an array of non-primitive objects
* Handles flattened and non-flattened cases
* A generic special stack frame must be built before calling this function
*
* Assumes srcObject and destObject are not null
* Assumes array bounds (srcIndex to (srcIndex + lengthInSlots), and destIndex to (destIndex + lengthInSlots)) are valid
* Assumes a generic special stack frame has been built on the stack
*
* @param[in] currentThread thread token
* @param[in] objectAccessBarrier access barrier
* @param[in] objectAllocate allocator
* @param[in] srcObject the source array to copy objects from
* @param[out] destObject the destination array in which objects should be stored
* @param[in] srcIndex the index in the source array to begin copying objects from
* @param[in] destIndex the index in the destination array to begin storing objects
* @param[in] lengthInSlots the number of elements to copy
*
* @return 0 if copy was successful, -1 if there was an array store error, -2 if there was a null pointer exception
*/
static VMINLINE I_32
copyFlattenableArray(J9VMThread *currentThread, MM_ObjectAccessBarrierAPI objectAccessBarrier, MM_ObjectAllocationAPI objectAllocate, j9object_t srcObject, j9object_t destObject, I_32 srcIndex, I_32 destIndex, I_32 lengthInSlots)
{
I_32 srcEndIndex = srcIndex + lengthInSlots;
J9Class *srcClazz = J9OBJECT_CLAZZ(currentThread, srcObject);
J9Class *destClazz = J9OBJECT_CLAZZ(currentThread, destObject);
J9Class *destComponentClass = ((J9ArrayClass *)destClazz)->componentType;

/* Array elements must be copied backwards if source and destination overlap in memory and source is before destination */
if ((srcObject == destObject) && (srcIndex < destIndex) && ((srcIndex + lengthInSlots) > destIndex)) {
srcEndIndex = srcIndex;
srcIndex += lengthInSlots;
destIndex += lengthInSlots;

while (srcIndex > srcEndIndex) {
srcIndex--;
destIndex--;

j9object_t copyObject = loadFlattenableArrayElement(currentThread, objectAccessBarrier, objectAllocate, srcObject, srcIndex, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible that loadFlattenableArrayElement() failed in the fast path and the returned copyObject is NULL.

Copy link
Author

Choose a reason for hiding this comment

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

Added checks for this in 5db20fe


/*
When the return value of loadFlattenableArrayElement is NULL, 2 things are possible:
1: The array element at that index is actually NULL
2: There was an allocation failure
But loadFlattenableArrayElement only tries to allocate when srcClazz is flattened, and so if copyObject is NULL and srcClazz is flattened then there has been an allocation failure
*/
if ((NULL == copyObject) && J9_IS_J9CLASS_FLATTENED(srcClazz)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is only possible if srcClazz is flattened, then you should assert J9_IS_J9CLASS_FLATTENED(srcClazz) is true inside if (NULL == copyObject)

Copy link
Author

Choose a reason for hiding this comment

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

What I mean by only possible if srcClazz is flattened is that when copyObject is NULL, 2 things are possible:

1: The flattenable array element at that index is actually NULL
Or
2: There was an allocation failure

Allocation failures are only possible when srcClazz is flattened, and so we need to make sure both (NULL == copyObject) and J9_IS_J9CLASS_FLATTENED(srcClazz) are true.

I guess I should update the comment to make that more clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please update the comment.

Copy link
Author

Choose a reason for hiding this comment

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

updated in 25f3eed

VM_VMHelpers::pushObjectInSpecialFrame(currentThread, srcObject);
copyObject = loadFlattenableArrayElement(currentThread, objectAccessBarrier, objectAllocate, srcObject, srcIndex, false);
srcObject = VM_VMHelpers::popObjectInSpecialFrame(currentThread);
}

/* No type checks required since srcObject == destObject */

storeFlattenableArrayElement(currentThread, objectAccessBarrier, destObject, destIndex, copyObject);
}
} else {

UDATA typeChecksRequired = !isSameOrSuperClassOf(destClazz, srcClazz);

while (srcIndex < srcEndIndex) {
j9object_t copyObject = loadFlattenableArrayElement(currentThread, objectAccessBarrier, objectAllocate, srcObject, srcIndex, true);

/*
When the return value of loadFlattenableArrayElement is NULL, 2 things are possible:
1: The array element at that index is actually NULL
2: There was an allocation failure
But loadFlattenableArrayElement only tries to allocate when srcClazz is flattened, and so if copyObject is NULL and srcClazz is flattened then there has been an allocation failure
*/
if ((NULL == copyObject) && J9_IS_J9CLASS_FLATTENED(srcClazz)) {
ehrenjulzert marked this conversation as resolved.
Show resolved Hide resolved
VM_VMHelpers::pushObjectInSpecialFrame(currentThread, srcObject);
copyObject = loadFlattenableArrayElement(currentThread, objectAccessBarrier, objectAllocate, srcObject, srcIndex, false);
srcObject = VM_VMHelpers::popObjectInSpecialFrame(currentThread);
}

if (typeChecksRequired) {
if (J9_IS_J9CLASS_PRIMITIVE_VALUETYPE(destComponentClass) && (NULL == copyObject)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to do the NPE check before the objectArrayStoreAllowed() check.

Copy link
Author

Choose a reason for hiding this comment

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

moved in 25f3eed

/* Null objects cannot be stored in an array of primitive value types */
return -2;
}
if (!VM_VMHelpers::objectArrayStoreAllowed(currentThread, destObject, copyObject)) {
return -1;
}
}

storeFlattenableArrayElement(currentThread, objectAccessBarrier, destObject, destIndex, copyObject);

srcIndex++;
destIndex++;
}
}

return 0;
}

};

#endif /* VALUETYPEHELPERS_HPP_ */