Skip to content

Commit df57c65

Browse files
committed
Add range check test around profiled guard method test
In OpenJ9 we take advantage of the profiled information in inlining method for virtual call. While inlining the method, we generate a guard around inlined code to make sure that the inlined implementation is indeed what we need to execute. For this TR_ProfiledGuard** kind of guard it either TR_VftTest or TR_MethodTest. In case of TR_MethodTest, it records the offset of the inlined method in the Virtual Function Table of the class in which that method belongs to and compares the value at the same offset of class of receiver object with the J9Method of inlined method. This comparison introduces a functional bug. Some classes would have smaller function table and recorded offset w.r.t the class might point to a garbage value. This commit adds a range check test around the TR_MethodTest to verify that address that we are going to reference points to valid address. Signed-off-by: Rahil Shah <rahil@ca.ibm.com>
1 parent 99e024a commit df57c65

8 files changed

+146
-8
lines changed

runtime/compiler/codegen/CodeGenPhaseToPerform.hpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2000, 2018 IBM Corp. and others
2+
* Copyright (c) 2000, 2019 IBM Corp. and others
33
*
44
* This program and the accompanying materials are made available under
55
* the terms of the Eclipse Public License 2.0 which accompanies this
@@ -28,6 +28,7 @@
2828

2929

3030
ReserveCodeCachePhase,
31+
FixUpProfiledInterfaceGuardTest,
3132

3233

3334
InliningReportPhase,

runtime/compiler/codegen/J9CodeGenPhase.cpp

+7
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,11 @@ J9::CodeGenPhase::getNumPhases()
5858
return static_cast<int>(TR::CodeGenPhase::LastJ9Phase);
5959
}
6060

61+
void
62+
J9::CodeGenPhase::performFixUpProfiledInterfaceGuardTestPhase(TR::CodeGenerator *cg, TR::CodeGenPhase *phase)
63+
{
64+
cg->fixUpProfiledInterfaceGuardTest();
65+
}
6166

6267
void
6368
J9::CodeGenPhase::performAllocateLinkageRegistersPhase(TR::CodeGenerator * cg, TR::CodeGenPhase * phase)
@@ -145,6 +150,8 @@ J9::CodeGenPhase::getName(TR::CodeGenPhase::PhaseValue phase)
145150
return "IdentifyUnneededByteConvsPhase";
146151
case LateSequentialConstantStoreSimplificationPhase:
147152
return "LateSequentialConstantStoreSimplification";
153+
case FixUpProfiledInterfaceGuardTest:
154+
return "FixUpProfiledInterfaceGuardTest";
148155
default:
149156
return OMR::CodeGenPhaseConnector::getName(phase);
150157
}

runtime/compiler/codegen/J9CodeGenPhase.hpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2000, 2018 IBM Corp. and others
2+
* Copyright (c) 2000, 2019 IBM Corp. and others
33
*
44
* This program and the accompanying materials are made available under
55
* the terms of the Eclipse Public License 2.0 which accompanies this
@@ -47,7 +47,7 @@ class OMR_EXTENSIBLE CodeGenPhase: public OMR::CodeGenPhaseConnector
4747
public:
4848

4949
void reportPhase(PhaseValue phase);
50-
50+
static void performFixUpProfiledInterfaceGuardTestPhase(TR::CodeGenerator *cg, TR::CodeGenPhase *);
5151
static void performAllocateLinkageRegistersPhase(TR::CodeGenerator * cg, TR::CodeGenPhase *);
5252
static void performPopulateOSRBufferPhase(TR::CodeGenerator * cg, TR::CodeGenPhase *);
5353
static void performMoveUpArrayLengthStoresPhase(TR::CodeGenerator * cg, TR::CodeGenPhase *);

runtime/compiler/codegen/J9CodeGenPhaseEnum.hpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2000, 2018 IBM Corp. and others
2+
* Copyright (c) 2000, 2019 IBM Corp. and others
33
*
44
* This program and the accompanying materials are made available under
55
* the terms of the Eclipse Public License 2.0 which accompanies this
@@ -29,7 +29,7 @@
2929
#include "codegen/OMRCodeGenPhaseEnum.hpp"
3030

3131
// The entries in this file must be kept in sync with codegen/J9CodeGenPhaseFunctionTable.hpp
32-
32+
FixUpProfiledInterfaceGuardTest,
3333
AllocateLinkageRegisters,
3434
PopulateOSRBufferPhase,
3535
MoveUpArrayLengthStoresPhase,

runtime/compiler/codegen/J9CodeGenPhaseFunctionTable.hpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2000, 2018 IBM Corp. and others
2+
* Copyright (c) 2000, 2019 IBM Corp. and others
33
*
44
* This program and the accompanying materials are made available under
55
* the terms of the Eclipse Public License 2.0 which accompanies this
@@ -28,6 +28,7 @@
2828
#include "codegen/OMRCodeGenPhaseFunctionTable.hpp"
2929

3030
// The entries in this file must be kept in sync with codegen/J9CodeGenPhaseEnum.hpp
31+
TR::CodeGenPhase::performFixUpProfiledInterfaceGuardTestPhase,
3132
TR::CodeGenPhase::performAllocateLinkageRegistersPhase, //AllocateLinkageRegisters
3233
TR::CodeGenPhase::performPopulateOSRBufferPhase, //PopulateOSRBufferPhase
3334
TR::CodeGenPhase::performMoveUpArrayLengthStoresPhase, //MoveUpArrayLengthStoresPhase

runtime/compiler/codegen/J9CodeGenerator.cpp

+127
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@
5353
#include "il/symbol/LabelSymbol.hpp"
5454
#include "infra/Assert.hpp"
5555
#include "infra/BitVector.hpp"
56+
#include "infra/ILWalk.hpp"
5657
#include "infra/List.hpp"
5758
#include "optimizer/Structure.hpp"
5859
#include "optimizer/TransformUtil.hpp"
@@ -3916,6 +3917,132 @@ J9::CodeGenerator::collectSymRefs(
39163917
return true;
39173918
}
39183919

3920+
/** \brief
3921+
* Following codegen phase walks the blocks in the CFG and checks for the virtual guard performing TR_MethodTest
3922+
* and guarding an inlined interface call.
3923+
*
3924+
* \details
3925+
* Virtual Guard performing TR_MethodTest would look like following.
3926+
* n1n BBStart <block_X>
3927+
* ...
3928+
* n2n ifacmpne goto -> nXXn
3929+
* n3n aloadi <offset of inlined method in VTable>
3930+
* n4n aload <vft>
3931+
* n5n aconst <J9Method of inlined method>
3932+
* n6n BBEnd <block_X>
3933+
* For virtual dispatch sequence, we know that this is the safe check but in case of interface call, classes implementing
3934+
* that interface would have different size of VTable. This makes executing above check unsafe when VTable of the class of
3935+
* the receiver object is smaller, effectively making reference in n3n to pointing to a garbage location which might lead
3936+
* to a segmentation fault if the reference in not memory mapped or if bychance it contains J9Method pointer of same inlined
3937+
* method then it will execute a code which should not be executed.
3938+
* For this kind of Virtual guards which are not nop'd we need to add a range check to make sure the address we are going to
3939+
* access is pointing to a valid location in VTable. There are mainly two ways we can add this range check test. First one is
3940+
* during the conception of the virtual guard. There are many downsides of doing so especially when other optimizations which
3941+
* can moved guards around (for example loop versioner, virtualguard head merger, etc) needs to make sure to move range check
3942+
* test around as well. Other way is to scan for this type of guards after optimization is finished like here in CodeGen Phase
3943+
* and add a range check test here.
3944+
* At the end of this function, we would have following code around them method test.
3945+
* BBStart <block_X>
3946+
* ...
3947+
* ifacmple goto nXXn
3948+
* aloadi <offset of VTableHeader.size from J9Class*>
3949+
* aload <vft>
3950+
* aconst <Index of the inlined method in VTable of class of inlined method>
3951+
* BBEnd <block_X>
3952+
*
3953+
* BBStart <block_Y>
3954+
* ifacmpne goto -> nXXn
3955+
* aloadi <offset of inlined method in VTable>
3956+
* aload <vft>
3957+
* aconst <J9Method of inlined method>
3958+
* BBEnd <block_Y>
3959+
*/
3960+
void
3961+
J9::CodeGenerator::fixUpProfiledInterfaceGuardTest()
3962+
{
3963+
TR::Compilation *comp = self()->comp();
3964+
TR::CFG * cfg = comp->getFlowGraph();
3965+
TR::NodeChecklist checklist(comp);
3966+
for (TR::AllBlockIterator iter(cfg, comp); iter.currentBlock() != NULL; ++iter)
3967+
{
3968+
TR::Block *block = iter.currentBlock();
3969+
TR::TreeTop *treeTop = block->getLastRealTreeTop();
3970+
TR::Node *node = treeTop->getNode();
3971+
if (node->getOpCode().isIf() && node->isTheVirtualGuardForAGuardedInlinedCall() && !checklist.contains(node))
3972+
{
3973+
TR_VirtualGuard *vg = comp->findVirtualGuardInfo(node);
3974+
// Mainly we need to make sure that virtual guard which performs the TR_MethodTest and can be NOP'd are needed the range check.
3975+
if (vg && vg->getTestType() == TR_MethodTest &&
3976+
!(comp->performVirtualGuardNOPing() && (node->isNopableInlineGuard() || comp->isVirtualGuardNOPingRequired(vg))))
3977+
{
3978+
TR::SymbolReference *callSymRef = vg->getSymbolReference();
3979+
TR_ASSERT_FATAL(callSymRef != NULL, "Guard n%dn for the inlined call should have stored symbol reference for the call", node->getGlobalIndex());
3980+
if (callSymRef->getSymbol()->castToMethodSymbol()->isInterface())
3981+
{
3982+
TR::DebugCounter::incStaticDebugCounter(comp, TR::DebugCounter::debugCounterName(comp, "profiledInterfaceTest/({%s}{%s})", comp->signature(), comp->getHotnessName(comp->getMethodHotness())));
3983+
dumpOptDetails(comp, "Need to add a rangecheck before n%dn in block_%d\n",node->getGlobalIndex(), block->getNumber());
3984+
3985+
// We need a VFT Load of the receiver object to get the VTableHeader.size to check the range. As this operation is happening during codegen phase, only
3986+
// known concrete way we can have this information is through aloadi child of the guard that has single child which is vft load of receiver object.
3987+
// Now instead of accessing VFT load from the child of the aloadi, we could have treetop's the aloadi during inlining where we generate the virtual guard
3988+
// to access information from the treetop. Because of the same reasons lined up behind adding range check test during codegen phase in the description of this function,
3989+
// we would need to make changes in all optimizations moving Virtual Guard around to keep that treetop together before guard which will be very difficult to enforce.
3990+
// Also as children of virtual guard is very self contained and atm it is very unlikely that other optimizations are going to find opportunity of manipulating them and
3991+
// Because of the fact that it is very unlikely that we will have another aloadi node with same VTable offset of same receiver object, this child would not be commoned out
3992+
// and have only single reference in this virtual guard therefore splitting of block will not store it to temp slot.
3993+
// In rare case child of the virtual guard is manipulated then illegal memory reference load would hace occured before the Virtual Guard which
3994+
// is already a bug as mentioned in the description of this function and it would be safer to fail compilation.
3995+
TR::Node *vTableLoad = node->getFirstChild();
3996+
if (!(vTableLoad->getOpCodeValue() == TR::aloadi && comp->getSymRefTab()->isVtableEntrySymbolRef(vTableLoad->getSymbolReference())))
3997+
comp->failCompilation<TR::CompilationException>("Abort compilation as Virtual Guard has generated illegal memory reference");
3998+
TR::Node *vTableSizeOfReceiver = NULL;
3999+
TR::Node *rangeCheckTest = NULL;
4000+
if (TR::Compiler->target.is64Bit())
4001+
{
4002+
vTableSizeOfReceiver = TR::Node::createWithSymRef(TR::lloadi, 1, 1, vTableLoad->getFirstChild(),
4003+
comp->getSymRefTab()->findOrCreateVtableEntrySymbolRef(comp->getMethodSymbol(),
4004+
sizeof(J9Class)+ offsetof(J9VTableHeader, size)));
4005+
rangeCheckTest = TR::Node::createif(TR::iflcmple, vTableSizeOfReceiver,
4006+
TR::Node::lconst(node, (vTableLoad->getSymbolReference()->getOffset() - sizeof(J9Class) - sizeof(J9VTableHeader)) / sizeof(UDATA)) ,
4007+
node->getBranchDestination());
4008+
}
4009+
else
4010+
{
4011+
vTableSizeOfReceiver = TR::Node::createWithSymRef(TR::iloadi, 1, 1, vTableLoad->getFirstChild(),
4012+
comp->getSymRefTab()->findOrCreateVtableEntrySymbolRef(comp->getMethodSymbol(),
4013+
sizeof(J9Class)+ offsetof(J9VTableHeader, size)));
4014+
rangeCheckTest = TR::Node::createif(TR::ificmple, vTableSizeOfReceiver,
4015+
TR::Node::iconst(node, (vTableLoad->getSymbolReference()->getOffset() - sizeof(J9Class) - sizeof(J9VTableHeader)) / sizeof(UDATA)) ,
4016+
node->getBranchDestination());
4017+
}
4018+
TR::TreeTop *rangeTestTT = TR::TreeTop::create(comp, treeTop->getPrevTreeTop(), rangeCheckTest);
4019+
TR::Block *newBlock = block->split(treeTop, cfg, false, false);
4020+
cfg->addEdge(block, node->getBranchDestination()->getEnclosingBlock());
4021+
newBlock->setIsExtensionOfPreviousBlock();
4022+
if (node->getNumChildren() == 3)
4023+
{
4024+
TR::Node *currentBlockGlRegDeps = node->getChild(2);
4025+
TR::Node *exitGlRegDeps = TR::Node::create(TR::GlRegDeps, currentBlockGlRegDeps->getNumChildren());
4026+
for (int i = 0; i < currentBlockGlRegDeps->getNumChildren(); i++)
4027+
{
4028+
TR::Node *child = currentBlockGlRegDeps->getChild(i);
4029+
exitGlRegDeps->setAndIncChild(i, child);
4030+
}
4031+
rangeCheckTest->addChildren(&exitGlRegDeps, 1);
4032+
}
4033+
// While walking all blocks in CFG, when we find the location to add the range check, it will split the original block and
4034+
// We will have actual Virtual Guard in new block. As Block Iterator guarantees to visit all block in the CFG,
4035+
// While going over the blocks, we will encounter same virtual guard in newly created block after split.
4036+
// We need to make sure we are not examining already visited guard.
4037+
// Add checked virtual guard node to NodeChecklist to make sure we check all the nodes only once.
4038+
checklist.add(node);
4039+
}
4040+
}
4041+
}
4042+
}
4043+
}
4044+
4045+
39194046

39204047
void
39214048
J9::CodeGenerator::allocateLinkageRegisters()

runtime/compiler/codegen/J9CodeGenerator.hpp

+2
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,8 @@ class OMR_EXTENSIBLE CodeGenerator : public OMR::CodeGeneratorConnector
100100

101101
void allocateLinkageRegisters();
102102

103+
void fixUpProfiledInterfaceGuardTest();
104+
103105
void zeroOutAutoOnEdge(TR::SymbolReference * liveAutoSym, TR::Block *block, TR::Block *succBlock, TR::list<TR::Block*> *newBlocks, TR_ScratchList<TR::Node> *fsdStores);
104106

105107
TR::Linkage *createLinkageForCompilation();

runtime/compiler/z/codegen/CodeGenPhaseToPerform.hpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2000, 2018 IBM Corp. and others
2+
* Copyright (c) 2000, 2019 IBM Corp. and others
33
*
44
* This program and the accompanying materials are made available under
55
* the terms of the Eclipse Public License 2.0 which accompanies this
@@ -28,7 +28,7 @@
2828

2929

3030
ReserveCodeCachePhase,
31-
31+
FixUpProfiledInterfaceGuardTest,
3232

3333
InliningReportPhase,
3434
LateSequentialConstantStoreSimplificationPhase,

0 commit comments

Comments
 (0)