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

Null-restricted checks for jitCheckCastForArrayStore #20331

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Null-restricted checks for jitCheckCastForArrayStore #20331

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions runtime/codert_vm/cnathelp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
extern "C" {

void* J9FASTCALL
old_slow_jitThrowNullPointerException(J9VMThread *currentThread);
old_slow_jitThrowArrayStoreException(J9VMThread *currentThread);

J9_EXTERN_BUILDER_SYMBOL(throwCurrentExceptionFromJIT);
J9_EXTERN_BUILDER_SYMBOL(handlePopFramesFromJIT);
Expand Down Expand Up @@ -1490,18 +1490,20 @@ old_fast_jitCheckCastForArrayStore(J9VMThread *currentThread)
{
void *slowPath = NULL;
OLD_JIT_HELPER_PROLOGUE(2);
DECLARE_JIT_CLASS_PARM(castClass, 1);
DECLARE_JIT_CLASS_PARM(castClassArray, 1); // <-- castClassArray should be the array class, not its base class
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering when VT is not enabled, it should still be checking the component class but if VT is enabled, it will check the array class. I don't have a strong preference either way. @hzongaro What do you think?

jitCheckCastForArrayStore is currently only used at one place in EA fixupFieldAccessForNonContiguousAllocation. If checking array class here regardless of if VT is enabled or not, it will require a coordinated merge with the JIT change so that it doesn't break the functionality for non-VT build. But the change here is cleaner without having to consider if VT is enabled or not.

Copy link
Member

Choose a reason for hiding this comment

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

@a7ehuo - Good catch! Escape Analysis is using the class that was specified on an anewarray operation, so it will need to get the corresponding array class instead. And in order for EA to stack allocate non-nullable arrays, it will need to recognize calls to ValueClass.newArrayInstance(NullRestrictedCheckedType.of(myself),...).

I'll take a look at implementing those changes. . . .

DECLARE_JIT_PARM(j9object_t, object, 2);
/* null can be cast to anything, except if castClass is a primitive VT */
Assert_CodertVM_true(J9CLASS_IS_ARRAY(castClassArray));
/* null can be cast to anything, except if castClassArray is a null-restricted array */
if (NULL != object) {
J9Class *instanceClass = J9OBJECT_CLAZZ(currentThread, object);
J9Class *castClass = ((J9ArrayClass*)castClassArray)->componentType;
if (!VM_VMHelpers::inlineCheckCast(instanceClass, castClass)) {
slowPath = (void*)old_slow_jitCheckCastForArrayStore;
}
}
#if defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES)
else if (J9_IS_J9CLASS_PRIMITIVE_VALUETYPE(castClass)) {
slowPath = (void*)old_slow_jitThrowNullPointerException;
else if (J9_IS_J9ARRAYCLASS_NULL_RESTRICTED(castClassArray)) {
slowPath = (void*)old_slow_jitThrowArrayStoreException;
}
#endif /* defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES) */
return slowPath;
Expand Down Expand Up @@ -3615,25 +3617,27 @@ fast_jitCheckCast(J9VMThread *currentThread, J9Class *castClass, j9object_t obje
void* J9FASTCALL
#if defined(J9VM_ARCH_X86) || defined(J9VM_ARCH_S390)
/* TODO Will be cleaned once all platforms adopt the correct parameter order */
fast_jitCheckCastForArrayStore(J9VMThread *currentThread, j9object_t object, J9Class *castClass)
fast_jitCheckCastForArrayStore(J9VMThread *currentThread, j9object_t object, J9Class *castClassArray)
#else /* J9VM_ARCH_X86 || J9VM_ARCH_S390*/
fast_jitCheckCastForArrayStore(J9VMThread *currentThread, J9Class *castClass, j9object_t object)
fast_jitCheckCastForArrayStore(J9VMThread *currentThread, J9Class *castClassArray, j9object_t object)
#endif /* J9VM_ARCH_X86 || J9VM_ARCH_S390*/
{
// extern void* slow_jitCheckCastForArrayStore(J9VMThread *currentThread);
JIT_HELPER_PROLOGUE();
void *slowPath = NULL;
/* null can be cast to anything, except if castClass is a primitive VT */
Assert_CodertVM_true(J9CLASS_IS_ARRAY(castClassArray));
/* null can be cast to anything, except if castClassArray is a null-restricted array */
if (NULL != object) {
J9Class *instanceClass = J9OBJECT_CLAZZ(currentThread, object);
J9Class *castClass = ((J9ArrayClass*)castClassArray)->componentType;
if (J9_UNEXPECTED(!VM_VMHelpers::inlineCheckCast(instanceClass, castClass))) {
SET_PARM_COUNT(0);
slowPath = (void*)old_slow_jitCheckCastForArrayStore;
}
}
#if defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES)
else if (J9_IS_J9CLASS_PRIMITIVE_VALUETYPE(castClass)) {
slowPath = (void*)old_slow_jitThrowNullPointerException;
else if (J9_IS_J9ARRAYCLASS_NULL_RESTRICTED(castClassArray)) {
slowPath = (void*)old_slow_jitThrowArrayStoreException;
}
#endif /* defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES) */
return slowPath;
Expand Down