From b1eedaa49fabd04bb5c74db8fff342e5c5a31832 Mon Sep 17 00:00:00 2001 From: mwilliams Date: Fri, 31 May 2013 09:42:50 -0700 Subject: [PATCH] Change semantics of DecodeCufIter Previously it returned a bool to say whether or not it had succeeded, but always initialized the iter. The other iterators branch on failure. This adds the branch target which brings it closer to the other iterators, and avoids the need to do a (pointless) CIterFree on the failure path just to keep the validator happy. I've also added a translation for it. --- hphp/doc/ir.specification | 8 +- hphp/runtime/vm/as.cpp | 9 +- hphp/runtime/vm/bytecode.cpp | 28 ++-- hphp/runtime/vm/func.cpp | 3 +- hphp/runtime/vm/hhbc.h | 2 +- hphp/runtime/vm/jit/codegen.cpp | 7 + hphp/runtime/vm/jit/extradata.h | 1 + hphp/runtime/vm/jit/hhbctranslator.cpp | 14 ++ hphp/runtime/vm/jit/hhbctranslator.h | 1 + hphp/runtime/vm/jit/ir.h | 2 + hphp/runtime/vm/jit/irtranslator.cpp | 7 + .../runtime/vm/jit/translator-x64-helpers.cpp | 30 ++++ hphp/runtime/vm/jit/translator-x64.h | 2 + hphp/runtime/vm/jit/translator.cpp | 2 +- hphp/runtime/vm/jit/translator.h | 1 + hphp/runtime/vm/verifier/check_func.cpp | 3 - hphp/test/slow/builtin_support/setwithref.php | 13 +- .../builtin_support/setwithref.php.expect | 14 +- hphp/test/slow/builtin_support/support.hhas | 142 ++++++++++++------ 19 files changed, 211 insertions(+), 78 deletions(-) diff --git a/hphp/doc/ir.specification b/hphp/doc/ir.specification index b60deb0ef..ce153b850 100644 --- a/hphp/doc/ir.specification +++ b/hphp/doc/ir.specification @@ -1616,9 +1616,15 @@ IterFree S0:FramePtr Free the iterator variable whose index is given by S1 in the stack frame pointed to by S0. +D:Bool = DecodeCufIter S0:{Arr|Obj|Str} S1:FramePtr + + Decode S0 as a callable, and write the decoded values to the iterator + specified by IterId. Returns true iff it successfully decoded the + callable. Does not raise warnings, or throw exceptions. + CIterFree S0:FramePtr - Free the iterator variable whose index is given by S1 in the stack + Free the iterator variable whose index is given by IterId in the stack frame pointed to by S0. diff --git a/hphp/runtime/vm/as.cpp b/hphp/runtime/vm/as.cpp index 3eb36a884..4c9ced843 100644 --- a/hphp/runtime/vm/as.cpp +++ b/hphp/runtime/vm/as.cpp @@ -1160,7 +1160,7 @@ void parse_numiters(AsmState& as) { void parse_function_body(AsmState&, int nestLevel = 0); /* - * directive-fault : identifier '{' function-body + * directive-fault : identifier integer? '{' function-body * ; */ void parse_fault(AsmState& as, int nestLevel) { @@ -1170,6 +1170,11 @@ void parse_fault(AsmState& as, int nestLevel) { if (!as.in.readword(label)) { as.error("expected label name after .try_fault"); } + int iterId = -1; + as.in.skipWhitespace(); + if (as.in.peek() != '{') { + iterId = read_opcode_arg(as); + } as.in.expectWs('{'); parse_function_body(as, nestLevel + 1); @@ -1177,7 +1182,7 @@ void parse_fault(AsmState& as, int nestLevel) { eh.m_ehtype = EHEnt::EHType_Fault; eh.m_base = start; eh.m_past = as.ue->bcPos(); - eh.m_iterId = -1; // only used for debug printing + eh.m_iterId = iterId; as.addLabelEHFault(label, as.fe->ehtab().size() - 1); } diff --git a/hphp/runtime/vm/bytecode.cpp b/hphp/runtime/vm/bytecode.cpp index 7def15076..d2145a66d 100644 --- a/hphp/runtime/vm/bytecode.cpp +++ b/hphp/runtime/vm/bytecode.cpp @@ -5903,6 +5903,7 @@ inline void OPTBLD_INLINE VMExecutionContext::iopFPushCtorD(PC& pc) { inline void OPTBLD_INLINE VMExecutionContext::iopDecodeCufIter(PC& pc) { NEXT(); DECODE_IA(itId); + DECODE(Offset, offset); Iter* it = frame_iter(m_fp, itId); CufIter &cit = it->cuf(); @@ -5919,26 +5920,21 @@ inline void OPTBLD_INLINE VMExecutionContext::iopDecodeCufIter(PC& pc) { const Func* f = vm_decode_function(tvAsVariant(func), ar, false, obj, cls, invName, - true); + false); - bool ret = true; if (f == nullptr) { - f = SystemLib::s_nullFunc; - obj = nullptr; - cls = nullptr; - invName = nullptr; - ret = false; - } - - cit.setFunc(f); - if (obj) { - cit.setCtx(obj); - obj->incRefCount(); + pc += offset; } else { - cit.setCtx(cls); + cit.setFunc(f); + if (obj) { + cit.setCtx(obj); + obj->incRefCount(); + } else { + cit.setCtx(cls); + } + cit.setName(invName); } - cit.setName(invName); - tvAsVariant(m_stack.top()) = ret; + m_stack.popC(); } inline void OPTBLD_INLINE VMExecutionContext::iopFPushCufIter(PC& pc) { diff --git a/hphp/runtime/vm/func.cpp b/hphp/runtime/vm/func.cpp index d2c84a91b..a4467a185 100644 --- a/hphp/runtime/vm/func.cpp +++ b/hphp/runtime/vm/func.cpp @@ -479,8 +479,9 @@ void Func::prettyPrint(std::ostream& out) const { if (params[i].funcletOff() != InvalidAbsoluteOffset) { out << " DV for parameter " << i << " at " << params[i].funcletOff(); if (params[i].phpCode()) { - out << " = " << params[i].phpCode()->data() << std::endl; + out << " = " << params[i].phpCode()->data(); } + out << std::endl; } } const EHEntVec& ehtab = shared()->m_ehtab; diff --git a/hphp/runtime/vm/hhbc.h b/hphp/runtime/vm/hhbc.h index 61956c993..23dae657f 100644 --- a/hphp/runtime/vm/hhbc.h +++ b/hphp/runtime/vm/hhbc.h @@ -537,9 +537,9 @@ enum SetOpOp { O(IterNextK, FOUR(IA,BA,HA,HA),NOV, NOV, CF) \ O(MIterNextK, FOUR(IA,BA,HA,HA),NOV, NOV, CF) \ O(WIterNextK, FOUR(IA,BA,HA,HA),NOV, NOV, CF) \ + O(DecodeCufIter, TWO(IA,BA), ONE(CV), NOV, CF) \ O(IterFree, ONE(IA), NOV, NOV, NF) \ O(MIterFree, ONE(IA), NOV, NOV, NF) \ - O(DecodeCufIter, ONE(IA), ONE(CV), ONE(CV), NF) \ O(CIterFree, ONE(IA), NOV, NOV, NF) \ O(Incl, NA, ONE(CV), ONE(CV), CF) \ O(InclOnce, NA, ONE(CV), ONE(CV), CF) \ diff --git a/hphp/runtime/vm/jit/codegen.cpp b/hphp/runtime/vm/jit/codegen.cpp index 64cca0ba3..c56661d15 100644 --- a/hphp/runtime/vm/jit/codegen.cpp +++ b/hphp/runtime/vm/jit/codegen.cpp @@ -5269,6 +5269,13 @@ void CodeGenerator::cgIterFree(IRInstruction* inst) { ArgGroup(m_regs).addr(fpReg, offset)); } +void CodeGenerator::cgDecodeCufIter(IRInstruction* inst) { + PhysReg fpReg = m_regs[inst->src(1)].reg(); + int64_t offset = iterOffset(inst->extra()->iterId); + cgCallHelper(m_as, (TCA)decodeCufIterHelper, inst->dst(), kSyncPoint, + ArgGroup(m_regs).addr(fpReg, offset).typedValue(inst->src(0))); +} + void CodeGenerator::cgCIterFree(IRInstruction* inst) { PhysReg fpReg = m_regs[inst->src(0)].reg(); int64_t offset = iterOffset(inst->extra()->iterId); diff --git a/hphp/runtime/vm/jit/extradata.h b/hphp/runtime/vm/jit/extradata.h index b9d1aac39..c9b9c73d9 100644 --- a/hphp/runtime/vm/jit/extradata.h +++ b/hphp/runtime/vm/jit/extradata.h @@ -312,6 +312,7 @@ X(StLoc, LocalId); X(StLocNT, LocalId); X(IterFree, IterId); X(CIterFree, IterId); +X(DecodeCufIter, IterId); X(CufIterSpillFrame, FPushCufData); X(DefConst, ConstData); X(LdConst, ConstData); diff --git a/hphp/runtime/vm/jit/hhbctranslator.cpp b/hphp/runtime/vm/jit/hhbctranslator.cpp index 155247f94..37031cf7e 100644 --- a/hphp/runtime/vm/jit/hhbctranslator.cpp +++ b/hphp/runtime/vm/jit/hhbctranslator.cpp @@ -963,6 +963,20 @@ void HhbcTranslator::emitIterFree(uint32_t iterId) { gen(IterFree, IterId(iterId), m_tb->getFp()); } +void HhbcTranslator::emitDecodeCufIter(uint32_t iterId, int offset) { + SSATmp* src = popC(); + Type type = src->type(); + if (type.subtypeOfAny(Type::Arr, Type::Str, Type::Obj)) { + SSATmp* res = gen(DecodeCufIter, Type::Bool, + IterId(iterId), src, m_tb->getFp()); + gen(DecRef, src); + emitJmpCondHelper(offset, true, res); + } else { + gen(DecRef, src); + emitJmp(offset, true, false); + } +} + void HhbcTranslator::emitCIterFree(uint32_t iterId) { gen(CIterFree, IterId(iterId), m_tb->getFp()); } diff --git a/hphp/runtime/vm/jit/hhbctranslator.h b/hphp/runtime/vm/jit/hhbctranslator.h index bc95bec11..606e2b02d 100644 --- a/hphp/runtime/vm/jit/hhbctranslator.h +++ b/hphp/runtime/vm/jit/hhbctranslator.h @@ -353,6 +353,7 @@ struct HhbcTranslator { uint32_t keyLocalId); void emitIterFree(uint32_t iterId); + void emitDecodeCufIter(uint32_t iterId, int targetOffset); void emitCIterFree(uint32_t iterId); void emitVerifyParamType(uint32_t paramId); diff --git a/hphp/runtime/vm/jit/ir.h b/hphp/runtime/vm/jit/ir.h index b49657f75..1d052bb39 100644 --- a/hphp/runtime/vm/jit/ir.h +++ b/hphp/runtime/vm/jit/ir.h @@ -481,6 +481,8 @@ O(WIterNext, D(Bool), S(FramePtr) \ O(WIterNextK, D(Bool), S(FramePtr) \ C(Int) C(Int) C(Int), E|N|Mem|Refs) \ O(IterFree, ND, S(FramePtr), E|N|Mem|Refs) \ +O(DecodeCufIter, D(Bool), S(Arr,Obj,Str) \ + S(FramePtr), E|N|Mem|Refs) \ O(CIterFree, ND, S(FramePtr), E|N|Mem|Refs) \ O(DefMIStateBase, D(PtrToCell), NA, NF) \ O(BaseG, D(PtrToGen), C(TCA) \ diff --git a/hphp/runtime/vm/jit/irtranslator.cpp b/hphp/runtime/vm/jit/irtranslator.cpp index 0cb34aa5a..80a3e12f9 100644 --- a/hphp/runtime/vm/jit/irtranslator.cpp +++ b/hphp/runtime/vm/jit/irtranslator.cpp @@ -1461,6 +1461,13 @@ TranslatorX64::irTranslateIterFree(const Tracelet& t, HHIR_EMIT(IterFree, i.imm[0].u_IVA); } +void +TranslatorX64::irTranslateDecodeCufIter(const Tracelet& t, + const NormalizedInstruction& i) { + + HHIR_EMIT(DecodeCufIter, i.imm[0].u_IVA, i.offset() + i.imm[1].u_BA); +} + void TranslatorX64::irTranslateCIterFree(const Tracelet& t, const NormalizedInstruction& i) { diff --git a/hphp/runtime/vm/jit/translator-x64-helpers.cpp b/hphp/runtime/vm/jit/translator-x64-helpers.cpp index 5481c8df5..54118b3c4 100644 --- a/hphp/runtime/vm/jit/translator-x64-helpers.cpp +++ b/hphp/runtime/vm/jit/translator-x64-helpers.cpp @@ -18,6 +18,7 @@ #include "hphp/runtime/vm/jit/targetcache.h" #include "hphp/runtime/eval/runtime/file_repository.h" #include "hphp/runtime/vm/event_hook.h" +#include "hphp/runtime/base/builtin_functions.h" #include "hphp/runtime/base/stats.h" namespace HPHP { @@ -269,4 +270,33 @@ void functionEnterHelper(const ActRec* ar) { sp = g_vmContext->m_stack.top(); } +HOT_FUNC_VM +int64_t decodeCufIterHelper(Iter* it, TypedValue func) { + DECLARE_FRAME_POINTER(framePtr); + + ObjectData* obj = nullptr; + HPHP::Class* cls = nullptr; + StringData* invName = nullptr; + + auto ar = (ActRec*)framePtr->m_savedRbp; + if (LIKELY(ar->m_func->isBuiltin())) { + ar = g_vmContext->getOuterVMFrame(ar); + } + const Func* f = vm_decode_function(tvAsVariant(&func), + ar, false, + obj, cls, invName, + false); + if (UNLIKELY(!f)) return false; + CufIter &cit = it->cuf(); + cit.setFunc(f); + if (obj) { + cit.setCtx(obj); + obj->incRefCount(); + } else { + cit.setCtx(cls); + } + cit.setName(invName); + return true; +} + } } // HPHP::Transl diff --git a/hphp/runtime/vm/jit/translator-x64.h b/hphp/runtime/vm/jit/translator-x64.h index e43a2ccae..9f1f3ff03 100644 --- a/hphp/runtime/vm/jit/translator-x64.h +++ b/hphp/runtime/vm/jit/translator-x64.h @@ -445,6 +445,7 @@ private: CASE(IterFree) \ CASE(FPassV) \ CASE(UnsetN) \ + CASE(DecodeCufIter) \ // These are instruction-like functions which cover more than one // opcode. @@ -828,6 +829,7 @@ const size_t kMaxNumTrampolines = kTrampolinesBlockSize / void fcallHelperThunk() asm ("__fcallHelperThunk"); void funcBodyHelperThunk() asm ("__funcBodyHelperThunk"); void functionEnterHelper(const ActRec* ar); +int64_t decodeCufIterHelper(Iter* it, TypedValue func); // These could be static but are used in hopt/codegen.cpp void raiseUndefVariable(StringData* nm); diff --git a/hphp/runtime/vm/jit/translator.cpp b/hphp/runtime/vm/jit/translator.cpp index bd7b75a25..11783c7c2 100644 --- a/hphp/runtime/vm/jit/translator.cpp +++ b/hphp/runtime/vm/jit/translator.cpp @@ -1333,7 +1333,7 @@ static const struct { Stack1, OutArray, -2 }}, { OpCufSafeReturn,{StackTop3|DontGuardAny, Stack1, OutUnknown, -2 }}, - { OpDecodeCufIter,{Stack1, Stack1, OutBoolean, 0 }}, + { OpDecodeCufIter,{Stack1, None, OutNone, -1 }}, /*** 11. Iterator instructions ***/ diff --git a/hphp/runtime/vm/jit/translator.h b/hphp/runtime/vm/jit/translator.h index 96181eeda..638036052 100644 --- a/hphp/runtime/vm/jit/translator.h +++ b/hphp/runtime/vm/jit/translator.h @@ -1021,6 +1021,7 @@ opcodeControlFlowInfo(const Opcode instr) { case OpMIterInitK: // Ditto case OpWIterInit: // Ditto case OpWIterInitK: // Ditto + case OpDecodeCufIter: // Ditto case OpThrow: case OpUnwind: case OpEval: diff --git a/hphp/runtime/vm/verifier/check_func.cpp b/hphp/runtime/vm/verifier/check_func.cpp index f34ebfd81..e7580f11c 100644 --- a/hphp/runtime/vm/verifier/check_func.cpp +++ b/hphp/runtime/vm/verifier/check_func.cpp @@ -730,9 +730,6 @@ bool FuncChecker::checkIter(State* cur, PC pc) { "IterInit* or MIterInit* <%d> trying to double-initialize\n", id); ok = false; } - if (Op(*pc) == OpDecodeCufIter) { - cur->iters[id] = true; - } } else { if (!cur->iters[id]) { verify_error("Cannot access un-initialized iter %d\n", id); diff --git a/hphp/test/slow/builtin_support/setwithref.php b/hphp/test/slow/builtin_support/setwithref.php index 3033b9447..b811ea032 100644 --- a/hphp/test/slow/builtin_support/setwithref.php +++ b/hphp/test/slow/builtin_support/setwithref.php @@ -3,6 +3,7 @@ require_once "support.hhas"; class X { + private $p = true; function __destruct() { var_dump(__METHOD__); } static function s_test($x) { return $x !== true; } function o_test($x) { return $x === $this->p; } @@ -25,12 +26,12 @@ function test() { $a = array(1, 'foo' => 'bar', 0, true, false); $b =& $a['foo']; - var_dump(fast_array_filter($a, null)); - var_dump(fast_array_filter($a, function($a) { return !$a; })); - var_dump(fast_array_filter($a, array('X', 's_test'))); - var_dump(fast_array_filter($a, array(new X, 'o_test'))); - var_dump(fast_array_filter($a, array(new X, 'bar'))); - var_dump(fast_array_filter($a, 'filt')); + var_dump("No function", fast_array_filter($a, null)); + var_dump("Closure", fast_array_filter($a, function($a) { return !$a; })); + var_dump("Static Method", fast_array_filter($a, array('X', 's_test'))); + var_dump("Dynamic Method", fast_array_filter($a, array(new X, 'o_test'))); + var_dump("__call", fast_array_filter($a, array(new X, 'bar'))); + var_dump("string", fast_array_filter($a, 'filt')); var_dump(fast_array_map(function($a) { return $a.$a; }, $a)); var_dump(fast_array_map(function&(&$a) { return $a; }, $a)); diff --git a/hphp/test/slow/builtin_support/setwithref.php.expect b/hphp/test/slow/builtin_support/setwithref.php.expect index bcfcdbcb1..8718996db 100644 --- a/hphp/test/slow/builtin_support/setwithref.php.expect +++ b/hphp/test/slow/builtin_support/setwithref.php.expect @@ -1,6 +1,8 @@ array(1) { ["foo"]=> - object(X)#1 (0) { + object(X)#1 (1) { + ["p":"X":private]=> + bool(true) } } string(13) "X::__destruct" @@ -12,6 +14,7 @@ array(1) { [0]=> &string(3) "bar" } +string(11) "No function" array(3) { [0]=> int(1) @@ -20,12 +23,14 @@ array(3) { [2]=> bool(true) } +string(7) "Closure" array(2) { [1]=> int(0) [3]=> bool(false) } +string(13) "Static Method" array(4) { [0]=> int(1) @@ -37,9 +42,13 @@ array(4) { bool(false) } string(13) "X::__destruct" -array(0) { +string(14) "Dynamic Method" +array(1) { + [2]=> + bool(true) } string(13) "X::__destruct" +string(6) "__call" array(3) { ["foo"]=> &string(3) "bar" @@ -48,6 +57,7 @@ array(3) { [2]=> bool(true) } +string(6) "string" array(1) { [2]=> bool(true) diff --git a/hphp/test/slow/builtin_support/support.hhas b/hphp/test/slow/builtin_support/support.hhas index 7198e3615..97311a888 100644 --- a/hphp/test/slow/builtin_support/support.hhas +++ b/hphp/test/slow/builtin_support/support.hhas @@ -23,39 +23,59 @@ RetC } -.function fast_array_filter($arr, $func) { +.function fast_array_filter($arr = no_args, $func = no_func, $res = entry) { .numiters 2; - NewArray - SetL $res - PopC +# if we get here, a value was supplied for $res + String "array_filter() expects at most 2 parameters" + Jmp warning +no_args: String "array_filter() expects at least 1 parameter, 0 given" + Jmp warning +not_array:String "array_filter() expects parameter 1 to be array" + Jmp warning +bad_func: String "array_filter() expects parameter 2 to be a valid callback" + Jmp warning +warning: Cns "E_USER_WARNING" + FCallBuiltin 2 2 "trigger_error" + PopR + Null + RetC + +entry: IsArrayL $arr + JmpZ not_array IssetL $func - JmpZ null_func + JmpZ no_func CGetL $func - DecodeCufIter 0 - JmpZ bad_func + DecodeCufIter 0 bad_func .try_fault kill_iter_0 { - CGetL $arr - WIterInitK 1 endloop_a $v $k -.try_fault kill_iter_1 { -loop_a: FPushCufIter 1 0 - FPassL 0 $v - FCall 1 - UnboxR - JmpZ skip_a - SetWithRefLM $v -skip_a: WIterNextK 1 loop_a $v $k -} + NewArray + SetL $res + PopC + + CGetL $arr + WIterInitK 1 endloop_a $v $k + .try_fault kill_iter_1 1 { + loop_a: FPushCufIter 1 0 + FPassL 0 $v + FCall 1 + UnboxR + JmpZ skip_a + SetWithRefLM $v + skip_a: WIterNextK 1 loop_a $v $k + } } endloop_a:CIterFree 0 endloop_n:CGetL $res RetC -null_func:CGetL $arr +no_func: NewArray + SetL $res + PopC + CGetL $arr WIterInitK 1 endloop_n $v $k -.try_fault kill_iter_1 { +.try_fault kill_iter_1_only 1 { loop_n: CGetL $v JmpZ skip_n SetWithRefLM $v @@ -63,55 +83,87 @@ skip_n: WIterNextK 1 loop_n $v $k } Jmp endloop_n -bad_func: Null - RetC - kill_iter_0: CIterFree 0 Unwind kill_iter_1: - IterFree 0 + IterFree 1 + Unwind +kill_iter_1_only: + IterFree 1 Unwind } -.function fast_array_map($func, $arr) { +.function fast_array_map($func = no_args, $arr = one_arg, $res = one_array) { .numiters 2; - IssetL $func +# If we get here, a value was supplied for $res +# so we bail out to the c++ implementation + FPushFuncD 1 "__builtin_array_map" + FCallBuiltin 0 0 "func_get_args" + UnboxR + FPassC 0 + FCallArray + UnboxR + RetC + +no_args: String "array_map() expects at least 2 parameters, 0 given" + Jmp warning + +one_arg: String "array_map() expects at least 2 parameters, 1 given" + Jmp warning + +not_array_citer_free: + CIterFree 0 +not_array:String "array_map() expects parameter 2 to be array" + Jmp warning +bad_func: String "array_map() expects parameter 1 to be a valid callback" + Jmp warning +warning: Cns "E_USER_WARNING" + FCallBuiltin 2 2 "trigger_error" + PopR + Null + RetC + +# but if we get here, there was only one array, +# so we use the fast, php version + +one_array:IssetL $func JmpZ ident CGetL $func - DecodeCufIter 0 - JmpZ bad_func - + DecodeCufIter 0 bad_func .try_fault kill_iter_0 { - NewArray - SetL $res - PopC + IsArrayL $arr + JmpZ not_array_citer_free - CGetL $arr - WIterInitK 1 endloop $v $k + NewArray + SetL $res + PopC -.try_fault kill_iter_1 { -loop_x: FPushCufIter 1 0 - FPassL 0 $v - FCall 1 - SetWithRefRM - WIterNextK 1 loop_x $v $k -} + CGetL $arr + WIterInitK 1 endloop $v $k + + .try_fault kill_iter_1 1 { + loop_x: FPushCufIter 1 0 + FPassL 0 $v + FCall 1 + SetWithRefRM + WIterNextK 1 loop_x $v $k + } } endloop: CIterFree 0 CGetL $res RetC -ident: CGetL $arr +ident: IsArrayL $arr + JmpZ not_array + CGetL $arr RetC -bad_func: Null - RetC kill_iter_0: CIterFree 0 Unwind kill_iter_1: - IterFree 0 + IterFree 1 Unwind }