From 2c72411dae299fd650965db4822723c258f3c33e Mon Sep 17 00:00:00 2001 From: jdelong Date: Fri, 22 Mar 2013 18:20:55 -0700 Subject: [PATCH] Don't use LdClsPropAddrCached for properties that might not be accessible Tx64 only translates CGetS when you do it with the context class the same as the class being looked up, to avoid the need for accessibility checks. The IR can translate it more often, but was using its fast path even when it was not safe to do so: if a previous (safe) access had allocated a targetcache entry, later accesses would be able to get to the property without an access check. For now just limit the optimiation to safe cases. --- .../vm/translator/hopt/hhbctranslator.cpp | 48 +++++++++---------- .../runtime/vm/translator/hopt/simplifier.cpp | 15 ++++-- hphp/test/vm/hopt_sprop_accessibility.php | 15 ++++++ hphp/test/vm/hopt_sprop_accessibility.php.exp | 1 + .../vm/hopt_sprop_accessibility.php.filter | 1 + hphp/test/vm/hopt_sprop_accessibility2.php | 14 ++++++ .../test/vm/hopt_sprop_accessibility2.php.exp | 1 + 7 files changed, 67 insertions(+), 28 deletions(-) create mode 100644 hphp/test/vm/hopt_sprop_accessibility.php create mode 100644 hphp/test/vm/hopt_sprop_accessibility.php.exp create mode 120000 hphp/test/vm/hopt_sprop_accessibility.php.filter create mode 100644 hphp/test/vm/hopt_sprop_accessibility2.php create mode 100644 hphp/test/vm/hopt_sprop_accessibility2.php.exp diff --git a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp b/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp index 788862735..aaf6e7057 100644 --- a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp +++ b/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp @@ -1037,15 +1037,15 @@ void HhbcTranslator::emitIssetL(int32_t id) { void HhbcTranslator::emitIssetG(const StringData* gblName) { TRACE(3, "%u: IssetG\n", m_bcOff); emitIsset(gblName, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedGblName, - &HPHP::VM::JIT::HhbcTranslator::emitLdGblAddr); + &HhbcTranslator::checkSupportedGblName, + &HhbcTranslator::emitLdGblAddr); } void HhbcTranslator::emitIssetS(const StringData* propName) { TRACE(3, "%u: IssetS\n", m_bcOff); emitIsset(propName, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedClsProp, - &HPHP::VM::JIT::HhbcTranslator::emitLdClsPropAddrOrExit); + &HhbcTranslator::checkSupportedClsProp, + &HhbcTranslator::emitLdClsPropAddrOrExit); } void HhbcTranslator::emitEmptyL(int32_t id) { @@ -1065,15 +1065,15 @@ void HhbcTranslator::emitEmptyL(int32_t id) { void HhbcTranslator::emitEmptyG(const StringData* gblName) { TRACE(3, "%u: EmptyG\n", m_bcOff); emitEmpty(gblName, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedGblName, - &HPHP::VM::JIT::HhbcTranslator::emitLdGblAddr); + &HhbcTranslator::checkSupportedGblName, + &HhbcTranslator::emitLdGblAddr); } void HhbcTranslator::emitEmptyS(const StringData* propName) { TRACE(3, "%u: EmptyS\n", m_bcOff); emitEmpty(propName, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedClsProp, - &HPHP::VM::JIT::HhbcTranslator::emitLdClsPropAddrOrExit); + &HhbcTranslator::checkSupportedClsProp, + &HhbcTranslator::emitLdClsPropAddrOrExit); } void HhbcTranslator::emitIsTypeC(Type t) { @@ -2251,41 +2251,41 @@ void HhbcTranslator::emitEmpty(const StringData* name, void HhbcTranslator::emitBindG(const StringData* gblName) { TRACE(3, "%u: BindG\n", m_bcOff); emitBind(gblName, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedGblName, - &HPHP::VM::JIT::HhbcTranslator::emitLdGblAddrDef); + &HhbcTranslator::checkSupportedGblName, + &HhbcTranslator::emitLdGblAddrDef); } void HhbcTranslator::emitBindS(const StringData* propName) { TRACE(3, "%u: BindS\n", m_bcOff); emitBind(propName, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedClsProp, - &HPHP::VM::JIT::HhbcTranslator::emitLdClsPropAddr); + &HhbcTranslator::checkSupportedClsProp, + &HhbcTranslator::emitLdClsPropAddr); } void HhbcTranslator::emitVGetG(const StringData* gblName) { TRACE(3, "%u: VGetG\n", m_bcOff); emitVGet(gblName, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedGblName, - &HPHP::VM::JIT::HhbcTranslator::emitLdGblAddrDef); + &HhbcTranslator::checkSupportedGblName, + &HhbcTranslator::emitLdGblAddrDef); } void HhbcTranslator::emitVGetS(const StringData* propName) { TRACE(3, "%u: VGetS\n", m_bcOff); emitVGet(propName, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedClsProp, - &HPHP::VM::JIT::HhbcTranslator::emitLdClsPropAddr); + &HhbcTranslator::checkSupportedClsProp, + &HhbcTranslator::emitLdClsPropAddr); } void HhbcTranslator::emitSetG(const StringData* gblName) { emitSet(gblName, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedGblName, - &HPHP::VM::JIT::HhbcTranslator::emitLdGblAddrDef); + &HhbcTranslator::checkSupportedGblName, + &HhbcTranslator::emitLdGblAddrDef); } void HhbcTranslator::emitSetS(const StringData* propName) { emitSet(propName, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedClsProp, - &HPHP::VM::JIT::HhbcTranslator::emitLdClsPropAddr); + &HhbcTranslator::checkSupportedClsProp, + &HhbcTranslator::emitLdClsPropAddr); } static Type getResultType(Type resultType, bool isInferedType) { @@ -2322,8 +2322,8 @@ void HhbcTranslator::emitCGetG(const StringData* gblName, bool isInferedType) { TRACE(3, "%u: CGetG\n", m_bcOff); emitCGet(gblName, resultType, isInferedType, true, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedGblName, - &HPHP::VM::JIT::HhbcTranslator::emitLdGblAddr); + &HhbcTranslator::checkSupportedGblName, + &HhbcTranslator::emitLdGblAddr); } void HhbcTranslator::emitCGetS(const StringData* propName, @@ -2331,8 +2331,8 @@ void HhbcTranslator::emitCGetS(const StringData* propName, bool isInferedType) { TRACE(3, "%u: CGetS\n", m_bcOff); emitCGet(propName, resultType, isInferedType, false, - &HPHP::VM::JIT::HhbcTranslator::checkSupportedClsProp, - &HPHP::VM::JIT::HhbcTranslator::emitLdClsPropAddrOrExit); + &HhbcTranslator::checkSupportedClsProp, + &HhbcTranslator::emitLdClsPropAddrOrExit); } void HhbcTranslator::emitBinaryArith(Opcode opc) { diff --git a/hphp/runtime/vm/translator/hopt/simplifier.cpp b/hphp/runtime/vm/translator/hopt/simplifier.cpp index e5a1bc213..ba04ce8c8 100644 --- a/hphp/runtime/vm/translator/hopt/simplifier.cpp +++ b/hphp/runtime/vm/translator/hopt/simplifier.cpp @@ -1214,12 +1214,12 @@ SSATmp* Simplifier::simplifyConvToStr(IRInstruction* inst) { SSATmp* Simplifier::simplifyLdClsPropAddr(IRInstruction* inst) { SSATmp* propName = inst->getSrc(1); - if (!propName->isConst()) return NULL; + if (!propName->isConst()) return nullptr; SSATmp* cls = inst->getSrc(0); const StringData* clsNameString = cls->isConst() ? cls->getValClass()->preClass()->name() - : NULL; + : nullptr; if (!clsNameString) { // see if you can get the class name from a LdCls IRInstruction* clsInst = cls->getInstruction(); @@ -1233,8 +1233,15 @@ SSATmp* Simplifier::simplifyLdClsPropAddr(IRInstruction* inst) { } if (!clsNameString) return nullptr; - // we known both the class name and the property name statically - // so use the caching version of LdClsPropAddr + // We known both the class name and the property name statically so + // we can use the caching version of LdClsPropAddr. To avoid doing + // accessibility checks, we only do this if the context class is the + // same as the actual class the property is on. + auto const ctxCls = inst->getSrc(2)->getValClass(); + if (!ctxCls || !clsNameString->isame(ctxCls->preClass()->name())) { + return nullptr; + } + return m_tb->gen(LdClsPropAddrCached, inst->getTaken(), cls, diff --git a/hphp/test/vm/hopt_sprop_accessibility.php b/hphp/test/vm/hopt_sprop_accessibility.php new file mode 100644 index 000000000..0ccb2d17a --- /dev/null +++ b/hphp/test/vm/hopt_sprop_accessibility.php @@ -0,0 +1,15 @@ +readVar(); +$nonstaticUnscoped(); diff --git a/hphp/test/vm/hopt_sprop_accessibility.php.exp b/hphp/test/vm/hopt_sprop_accessibility.php.exp new file mode 100644 index 000000000..e1c7ab47c --- /dev/null +++ b/hphp/test/vm/hopt_sprop_accessibility.php.exp @@ -0,0 +1 @@ +HipHop Fatal error: Invalid static property access: A::priv in hphp/test/vm/hopt_sprop_accessibility.php on line 4 diff --git a/hphp/test/vm/hopt_sprop_accessibility.php.filter b/hphp/test/vm/hopt_sprop_accessibility.php.filter new file mode 120000 index 000000000..7b75548ca --- /dev/null +++ b/hphp/test/vm/hopt_sprop_accessibility.php.filter @@ -0,0 +1 @@ +filepath.filter \ No newline at end of file diff --git a/hphp/test/vm/hopt_sprop_accessibility2.php b/hphp/test/vm/hopt_sprop_accessibility2.php new file mode 100644 index 000000000..68bbd048e --- /dev/null +++ b/hphp/test/vm/hopt_sprop_accessibility2.php @@ -0,0 +1,14 @@ +readVar(); + var_dump(isset(A::$priv)); +} +main(); diff --git a/hphp/test/vm/hopt_sprop_accessibility2.php.exp b/hphp/test/vm/hopt_sprop_accessibility2.php.exp new file mode 100644 index 000000000..5c0c0681d --- /dev/null +++ b/hphp/test/vm/hopt_sprop_accessibility2.php.exp @@ -0,0 +1 @@ +bool(false)