Skip to content

Commit

Permalink
Throw ArrayStoreException if null is stored in non-nullable array
Browse files Browse the repository at this point in the history 
Attempting to store a null reference to a non-nullable array is expected
to throw an ArrayStoreException.  Previously that was expected to result
in a NullPointerException.  This modifies Tree Lowering optimization's
transformation of calls to <nonNullableArrayNullStoreCheckhelper> to use
ZEROCHK to call jitArrayStoreException if the value to be stored is null
and the array is non-nullable.

Further, if Value Propagation determines at compile-time that an array
is non-nullable, it used to generate a NULLCHK for the value.  With this
change, it will generate a ZEROCHK that tests whether the value is a
null reference.

Finally, this change renames the utility method
TR_J9VMBase::checkArrayCompClassPrimitiveValueType to
TR_J9VMBase::testIsArrayClassNullRestrictedType, to reflect more recent
terminology.  It also removes the argument that expected an "if" OpCode
and will instead generate IL that yields a zero or non-zero result to
indicate whether the array is null-restricted, leaving it to the caller
to decide whether to generate IL that will branch or perform some other
action based on the result.

Signed-off-by:  Henry Zongaro <zongaro@ca.ibm.com>
  • Loading branch information
@hzongaro
hzongaro committed Oct 7, 2024
1 parent 845b1ce commit 3cd0898
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 63 deletions.
12 changes: 6 additions & 6 deletions runtime/compiler/env/VMJ9.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2937,6 +2937,12 @@ TR_J9VMBase::testIsClassArrayType(TR::Node *j9ClassRefNode)
return testAreSomeClassDepthAndFlagsSet(j9ClassRefNode, getFlagValueForArrayCheck());
}

TR::Node *
TR_J9VMBase::testIsArrayClassNullRestrictedType(TR::Node *j9ClassRefNode)
{
return testAreSomeClassFlagsSet(j9ClassRefNode, J9ClassArrayIsNullRestricted);
}

TR::Node *
TR_J9VMBase::loadArrayClassComponentType(TR::Node *j9ClassRefNode)
{
Expand All @@ -2959,12 +2965,6 @@ TR_J9VMBase::checkSomeArrayCompClassFlags(TR::Node *arrayBaseAddressNode, TR::IL
return ifNode;
}

TR::Node *
TR_J9VMBase::checkArrayCompClassPrimitiveValueType(TR::Node *arrayBaseAddressNode, TR::ILOpCodes ifCmpOp)
{
return checkSomeArrayCompClassFlags(arrayBaseAddressNode, ifCmpOp, J9ClassIsPrimitiveValueType);
}

TR::Node *
TR_J9VMBase::checkArrayCompClassValueType(TR::Node *arrayBaseAddressNode, TR::ILOpCodes ifCmpOp)
{
Expand Down
17 changes: 9 additions & 8 deletions runtime/compiler/env/VMJ9.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1321,6 +1321,15 @@ class TR_J9VMBase : public TR_FrontEnd
*/
TR::Node * testIsClassArrayType(TR::Node *j9ClassRefNode);

/**
* \brief Generate IL to test whether the array's class is null-restricted
* \param j9ClassRefNode A node representing a reference to a \ref J9Class
* \return \ref TR::Node that tests whether the array's class flags has the
* \ref J9ClassArrayIsNullRestricted flag set, yielding a non-zero result if the
* flag is set, or zero otherwise.
*/
TR::Node * testIsArrayClassNullRestrictedType(TR::Node *j9ClassRefNode);

/**
* \brief Test whether any of the specified flags is set on the array's component class
* \param arrayBaseAddressNode A node representing a reference to the array base address
Expand All @@ -1338,14 +1347,6 @@ class TR_J9VMBase : public TR_FrontEnd
*/
TR::Node * checkArrayCompClassValueType(TR::Node *arrayBaseAddressNode, TR::ILOpCodes ifCmpOp);

/**
* \brief Check whether or not the array component class is a primitive value type
* \param arrayBaseAddressNode A node representing a reference to the array base address
* \param ifCmpOp If comparison opCode such as ificmpeq or ificmpne
* \return \ref TR::Node that compares the array component class J9ClassIsPrimitiveValueType flag to a zero integer
*/
TR::Node * checkArrayCompClassPrimitiveValueType(TR::Node *arrayBaseAddressNode, TR::ILOpCodes ifCmpOp);

virtual J9JITConfig *getJ9JITConfig() { return _jitConfig; }

virtual int32_t getCompThreadIDForVMThread(void *vmThread);
Expand Down
5 changes: 3 additions & 2 deletions runtime/compiler/optimizer/J9ValuePropagation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2653,8 +2653,9 @@ J9::ValuePropagation::transformFlattenedArrayElementStore(TR_OpaqueClassBlock *a
// The value that is being stored into the array element has to be non null.
if (needsNullValueCheck)
{
TR::Node *passThru = TR::Node::create(callNode, TR::PassThrough, 1, valueNode);
TR::Node *nullCheck = TR::Node::createWithSymRef(callNode, TR::NULLCHK, 1, passThru, comp()->getSymRefTab()->findOrCreateNullCheckSymbolRef(comp()->getMethodSymbol()));
TR::Node *isNonNull = TR::Node::create(callNode, TR::acmpne, 2, valueNode, TR::Node::aconst(0));
TR::Node *nullCheck = TR::Node::createWithSymRef(callNode, TR::ZEROCHK, 1, isNonNull,
comp()->getSymRefTab()->findOrCreateArrayStoreExceptionSymbolRef(comp()->getMethodSymbol()));
callTree->insertBefore(TR::TreeTop::create(comp(), nullCheck));
if (trace())
{
Expand Down
94 changes: 47 additions & 47 deletions runtime/compiler/optimizer/TreeLowering.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -690,20 +690,14 @@ NonNullableArrayNullStoreCheckTransformer::lower(TR::Node* const node, TR::TreeT
// | BBEnd | |
// +--------------------------------+ |
// | BBStart (Extension) | |
// | ificmpeq -->------------------*---------+
// | iand | |
// | iloadi <isClassFlags> | |
// | aloadi <componentClass> | |
// | ZEROCHK jitArrayStoreException | |
// | icmpeq | |
// | iand | |
// | iloadi <isClassFlags> | |
// | aloadi <vft-symbol> | |
// | ==><array-reference> | |
// | iconst J9ClassIsPrimitiveValueType |
// | iconst 0 | |
// | BBEnd | |
// +--------------------------------+ |
// | BBStart (Extension) | |
// | NULLCHK | |
// | Passthrough | |
// | ==><value-reference> | |
// | iconst J9ClassArrayIsNullRestricted|
// | iconst 0 | |
// | BBEnd | |
// +--------------------------------+ |
// | |
Expand Down Expand Up @@ -731,17 +725,18 @@ NonNullableArrayNullStoreCheckTransformer::lower(TR::Node* const node, TR::TreeT
tt->getPrevTreeTop()->join(nextTT);
TR::Block *nextBlock = prevBlock->splitPostGRA(nextTT, cfg);

TR::Node *ifNode = comp()->fej9()->checkArrayCompClassPrimitiveValueType(destChild, TR::ificmpeq);
TR::SymbolReference* const vftSymRef = comp()->getSymRefTab()->findOrCreateVftSymbolRef();

ifNode->setBranchDestination(nextBlock->getEntry());
TR::Node *vftNode = TR::Node::createWithSymRef(node, TR::aloadi, 1, destChild, vftSymRef);

// Copy register dependencies from the end of the block split
// to the ificmpeq, which checks for a value type, that's being
// added to the end of that block
//
copyRegisterDependency(prevBlock->getExit()->getNode(), ifNode);
TR::Node *testIsNullRestrictedArray = comp()->fej9()->testIsArrayClassNullRestrictedType(vftNode);
TR::Node *testIsNotNullRestrictedArray = TR::Node::create(TR::icmpeq, 2, testIsNullRestrictedArray, TR::Node::iconst(0));

TR::TreeTop *ifArrayCompClassPrimitiveValueTypeTT = prevBlock->append(TR::TreeTop::create(comp(), ifNode));
TR::ResolvedMethodSymbol *currentMethod = comp()->getMethodSymbol();

TR::SymbolReference *jitThrowArrayStoreException = comp()->getSymRefTab()->findOrCreateArrayStoreExceptionSymbolRef(currentMethod);
TR::Node *checkNotNullRestrictedArray = TR::Node::createWithSymRef(TR::ZEROCHK, 1, 1, testIsNotNullRestrictedArray, jitThrowArrayStoreException);
TR::TreeTop *checkNotNullRestrictedArrayTT = prevBlock->append(TR::TreeTop::create(comp(), checkNotNullRestrictedArray));

bool enableTrace = trace();
auto* const nullConst = TR::Node::aconst(0);
Expand All @@ -753,35 +748,24 @@ NonNullableArrayNullStoreCheckTransformer::lower(TR::Node* const node, TR::TreeT
//
copyRegisterDependency(prevBlock->getExit()->getNode(), checkValueNull);

TR::TreeTop *checkValueNullTT = ifArrayCompClassPrimitiveValueTypeTT->insertBefore(TR::TreeTop::create(comp(), checkValueNull));
TR::TreeTop *checkValueNullTT = checkNotNullRestrictedArrayTT->insertBefore(TR::TreeTop::create(comp(), checkValueNull));

if (enableTrace)
{
traceMsg(comp(),"checkValueNull n%dn is inserted before n%dn in prevBlock %d\n", checkValueNull->getGlobalIndex(), ifNode->getGlobalIndex(), prevBlock->getNumber());
traceMsg(comp(),"checkValueNull n%dn is inserted before n%dn in prevBlock %d\n", checkValueNull->getGlobalIndex(), checkNotNullRestrictedArray->getGlobalIndex(), prevBlock->getNumber());
}

TR::Block *compTypeTestBlock = prevBlock->split(ifArrayCompClassPrimitiveValueTypeTT, cfg);
compTypeTestBlock->setIsExtensionOfPreviousBlock(true);
TR::Block *checkNotNullRestrictedBlock = prevBlock->split(checkNotNullRestrictedArrayTT, cfg);
checkNotNullRestrictedBlock->setIsExtensionOfPreviousBlock(true);

cfg->addEdge(prevBlock, nextBlock);

if (enableTrace)
{
traceMsg(comp(),"ifArrayCompClassValueTypeTT n%dn is isolated in compTypeTestBlock %d\n", ifNode->getGlobalIndex(), compTypeTestBlock->getNumber());
traceMsg(comp(),"checkNotNullRestrictedArray n%dn is isolated in checkNotNullRestrictedBlock %d\n", checkNotNullRestrictedArray->getGlobalIndex(), checkNotNullRestrictedBlock->getNumber());
}

TR::ResolvedMethodSymbol *currentMethod = comp()->getMethodSymbol();

TR::Node *passThru = TR::Node::create(node, TR::PassThrough, 1, sourceChild);
TR::Node *nullCheck = TR::Node::createWithSymRef(node, TR::NULLCHK, 1, passThru,
comp()->getSymRefTab()->findOrCreateNullCheckSymbolRef(currentMethod));
TR::TreeTop *nullCheckTT = compTypeTestBlock->append(TR::TreeTop::create(comp(), nullCheck));

TR::Block *nullCheckBlock = compTypeTestBlock->split(nullCheckTT, cfg);

nullCheckBlock->setIsExtensionOfPreviousBlock(true);

cfg->addEdge(compTypeTestBlock, nextBlock);
cfg->addEdge(checkNotNullRestrictedBlock, nextBlock);
}
else
{
Expand Down Expand Up @@ -901,6 +885,9 @@ static bool skipBoundChecks(TR::Compilation *comp, TR::Node *node)
*
Before:
+----------------------------------------+
|NULLCHK |
| PassThrough |
| ==>iRegLoad (array address) |
|treetop |
| acall jitLoadFlattenableArrayElement |
| ==>iRegLoad |
Expand All @@ -911,15 +898,16 @@ static bool skipBoundChecks(TR::Compilation *comp, TR::Node *node)
After:
+------------------------------------------+
|BBStart |
|treetop |
| ==>iRegLoad |
|NULLCHK |
| PassThrough |
| ==>iRegLoad (array address) |
|treetop |
| ==>aRegLoad |
|ificmpne ----->---------------------------+-----------+
| iand | |
| iloadi <isClassFlags> | |
| ... | |
| iconst J9ClassIsPrimitiveValueType | |
| iconst J9ClassArrayIsNullRestricted | |
| iconst 0 | |
| GlRegDeps () | |
| ==>aRegLoad | |
Expand Down Expand Up @@ -1120,8 +1108,13 @@ LoadArrayElementTransformer::lower(TR::Node* const node, TR::TreeTop* const tt)
///////////////////////////////////////
// 9. Create ificmpne node that checks classFlags

TR::SymbolReference* const vftSymRef = comp->getSymRefTab()->findOrCreateVftSymbolRef();

TR::Node *vftNode = TR::Node::createWithSymRef(node, TR::aloadi, 1, anchoredArrayBaseAddressNode, vftSymRef);
TR::Node *testIsArrayClassNullRestrictedNode = comp->fej9()->testIsArrayClassNullRestrictedType(vftNode);

// The branch destination will be set up later
TR::Node *ifNode = comp->fej9()->checkArrayCompClassPrimitiveValueType(anchoredArrayBaseAddressNode, TR::ificmpne);
TR::Node *ifNode = TR::Node::createif(TR::ificmpne, testIsArrayClassNullRestrictedNode, TR::Node::iconst(0));

// Copy register dependency to the ificmpne node that's being appended to the current block
copyRegisterDependency(arrayElementLoadBlock->getExit()->getNode(), ifNode);
Expand Down Expand Up @@ -1226,6 +1219,9 @@ static bool skipArrayStoreChecks(TR::Compilation *comp, TR::Node *node)
*
Before:
+-------------------------------------------+
|NULLCHK |
| PassThrough |
| aload <ArrayAddress> |
|treetop |
| acall jitStoreFlattenableArrayElement |
| aload <value> |
Expand All @@ -1237,8 +1233,9 @@ static bool skipArrayStoreChecks(TR::Compilation *comp, TR::Node *node)
After:
+-------------------------------------------+
|BBStart |
|treetop |
| aload <ArrayAddress> |
|NULLCHK |
| PassThrough |
| aload <ArrayAddress> |
|treetop |
| aload <index> |
|treetop |
Expand All @@ -1247,7 +1244,7 @@ static bool skipArrayStoreChecks(TR::Compilation *comp, TR::Node *node)
| iand | |
| iloadi <isClassFlags> | |
| ... | |
| iconst J9ClassIsPrimitiveValueType | |
| iconst J9ClassArrayIsNullRestricted | |
| iconst 0 | |
| GlRegDeps () | |
| PassThrough rcx | |
Expand All @@ -1260,8 +1257,6 @@ static bool skipArrayStoreChecks(TR::Compilation *comp, TR::Node *node)
+-------------------------------------------+ |
+-------------------------------------------+ |
|BBStart (extension of previous block) | |
|NULLCHK on n82n [if required] | |
| ... | |
|BNDCHK | |
| ... | |
|treetop | |
Expand Down Expand Up @@ -1476,8 +1471,13 @@ StoreArrayElementTransformer::lower(TR::Node* const node, TR::TreeTop* const tt)
///////////////////////////////////////
// 8. Create the ificmpne node that checks classFlags

TR::SymbolReference* const vftSymRef = comp->getSymRefTab()->findOrCreateVftSymbolRef();

TR::Node *vftNode = TR::Node::createWithSymRef(node, TR::aloadi, 1, anchoredArrayBaseAddressNode, vftSymRef);
TR::Node *testIsArrayClassNullRestrictedNode = comp->fej9()->testIsArrayClassNullRestrictedType(vftNode);

// The branch destination will be set up later
TR::Node *ifNode = comp->fej9()->checkArrayCompClassPrimitiveValueType(anchoredArrayBaseAddressNode, TR::ificmpne);
TR::Node *ifNode = TR::Node::createif(TR::ificmpne, testIsArrayClassNullRestrictedNode, TR::Node::iconst(0));

// Copy register dependency to the ificmpne node that's being appended to the current block
copyRegisterDependency(arrayElementStoreBlock->getExit()->getNode(), ifNode);
Expand Down

0 comments on commit 3cd0898

Please sign in to comment.