If a function was called via (re)enterVM, and it didn't yet
have a prolog, and it was unable to get the write lease, and it was
intercepted, and the intercept handler asked for the original function
to be skipped, fcallHelper didn't return correctly, typically resulting
in the original function being re-executed, and then re-intercepted -
except that it was one level deeper in the vm-nesting hierarchy.
Eventually, it crashed.
Hopefully a little bit simpler, and duplicating less logic
with Stack::toString. Contains some fixes to visitStackElems to make
it work for this (it is printing some stacks slightly incorrectly
right now, too ... :)
Move all the stack unwinding code to its own module, delete
some redundant enumerations, document a few things. Gets rid of most
of the remnants of the old setjmp/longjmp-based implementation at the
enterVM level but doesn't do much to the unwinder itself (coming in
separate diff to hopefully be easier to review).
This stuff almost is an abstraction layer, but it's in a
strange place. Also, the layout of TypedValue isn't something we've
been using through an abstraction layer, and if we want to later I
think we won't want to do so using this one.
Remove unnecessary optimization that can be achieved in a more general
way and simplify continuation creation code.
VMExecutionContext::createContinuationHelper was renamed to
VMExecutionContext::createCont{Func,Meth}. The createContFunc no longer
takes this/class arguments, the createContMeth transfers them in one
Type::Ctx pointer that is used natively by ActRec's m_this. Since the
whole logic of this function is to set this single field, the logic is
now simpler.
Interpreter: iopCreateCont() just passes the m_this field of the parent
ActRec.
Translator: CreateCont opcode loads m_this field using LdCtx opcode.
This opcode optimizes into LdThis, which optimizes into
DefInlineFP->SpillFrame->object and allows frame to be eliminated, a
case previously covered by InlineCreateCont.
This diff uncovered a bug in trace builder, where LdCtx in static
methods could be optimized into LdThis, if the method was called thru
object. Fixed.
Save 8 bytes of m_args and its initialization for Continuations without
func_get_args() call (does not save real memory due to 16-byte alignment).
Store variable arguments in optional local.
Vector statements ending with a SetProp now properly free
memory when run with JIT enabled. Additionally, in both
JIT and interpreted mode memory will still be freed if
exceptions are thrown during evaluation of vector operations.
There were multiple issues with flow control when exceptions occur. Fixed these by ditching the reliance on the exception thrown interrupt and introduce an exception handler interrupt, which indicates control is about to pass to a catch clause. This gives us much better insight into how execution is flowing and how we might need to adjust an in-flight stepping operation.
Users of m_localsOffset used this offset to calculate pointer where
local variables starts. Since they are stored in the reverse order, the
equation was:
cont_ptr + m_localsOffset + num_locals - local_id - 1
We are already storing pointer to ActRec (m_arPtr), which is equal to:
cont_ptr + m_localsOffset + num_locals
This diffs kills m_localsOffset and uses m_arPtr to simplify
calculations. Continuation fields were rearranged so that
sizeof(c_Continuation) is reduced by 16 bytes.
I need to walk a live eval stack, handling FPI regions and
generators, and duplicating that logic in yet another place seems
wrong. This makes the Stack::toString stuff use a reusable one---at
some point I'll see if the unwinder can use it as well, but it works
also for my use case. Also, Stack::toString was broken for mutable
array iterators---fixes that. And use a union now for struct Iter.
And don't include bytecode.h from func.h.
m_obj in c_Continuation is redundant. It is only used at the time
Continuation is created. First, it is set in init() call. Few
instructions later, it is read so that $this in the Continuation's
ActRec can be populated.
Kill it.
nvSet() only casts the value from TypedValue* to const Variant&; do it
at callsites. Inlined array_setm_ik1_v0() and array_setm_s0k1_v0() into
their only remaining callsites in translator-runtime.cpp.
This is the first results from profiling callsites of
StringData::initLiteral. This diff converts a handful more
string literals to StaticString, removes overloaded Variant
comparison operators (operator==, etc), and avoids
constructing new strings in a few cases in tvCastToString
and tvCastToStringInPlace.
We had a lot of odd behavior with both Next and Out. Previously the debugger would interpret the world until it saw that it was no longer on a specific line, or at a specific stack depth. I changed it recently to let the program run normally, and use an internal breakpoint to control step outs. Next's become like a step out temporarily when you descend into a function and need to get back out to the original line, so some bugs with out showed as bugs with Next, too.
Specifically, any time more PHP code was executed as a "side-effect" of a non-fcall instruction, step out would get lost. In these cases the stack trace gives us the offset of the instruction causing the PHP to run, not the instruction that control will return to as is the case with a Fcall. A breakpoint set there would get missed. This changes the step out logic to recognize such cases (via the fact that a nested VM state was pushed to execute the code) and step out more intelligently. We look at the instruction in question, and determine where execution may go, which might be multiple places. I also made a small change to ensure that we don't stop in generated functions when stepping out, which cleans up the iterator experience quite a bit, and sets us up for proper generator stepping, which will come next.
runtime/eval is a relic of a bygone era. As long as we're cleaning up
our directory structure, let's move FileRepository (the only remaining
thing in runtime/eval/runtime) to where it makes sense.
runtime/eval still contains the debugger, which would probably make more
sense as runtime/debugger, but I don't want to throw a wrench in the
works for @mikemag and @hermanv unnecessarily.
I was going to #include translator.h in a header I had for
talking to the region selector thing and decided to just get this over
with instead. (It shouldn't need to #include that.) Found a few
other unused things to remove while at it.
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.
This was barely a demonstrable win in sandbox mode when I first wrote
it and it's not even used in hhir. It's probably been bitrotting and is causing
crashes for some people. Time to say goodbye.
Most of the SpillStacks we do are just to keep the in-memory VM stack
clean in case a helper call throws an exception. Many of these exceptions are
vanishingly rare, so let's stop making the fast path slower for them. This diff
adds support for catch traces, which are just like normal exit traces with one
major exception: they're never jumped to from translated code. If we're
unwinding a TC frame and discover that the current rip has a catch trace
registered, the unwinder will execute it before resuming unwinding.
The unwinder parts were fairly straightfoward, then I ran into a bunch of
issues with SetM. The SetElem instruction sometimes consumes a reference to one
of its inputs, which is bad news for our optimizations. To make everything work
again, I changed SetElem to throw an exception in cases where it would've
previously decreffed the value input. This is a special exception that the new
unwinding code recognizes. When one of these is caught, the catch trace
executed for that TC frame will finish up the vector instruction and push the
value provided by the exception on the stack. This allows us to optimize most
traces under the assumption that SetM's output is the same as its input,
letting the catch trace clean things up if the helper decides that's not the
case and throws.
There is one common case where SetM's output isn't the same as its input:
setting an offset in a string base returns a new String. Luckily, we can detect
most of these at compile time so SetElem returns a new StringData* when the
base is known to be a string. If the base might be a string, SetElem will
return nullptr if the base wasn't a string, and a StringData* if it was. We
test the output in these cases and side exit if the return value of SetElem is
non-null (I have yet to see this side exit happen outside of artificially
created test cases).
Once I got things working in the vector translator, I went through every other
call to spillStack and exceptionBarrier, replacing them with catch traces as
appropriate.
Functions like array_filter and array_map need "withRef" semantics,
and while it can be simulated in php via copy-on-write, doing so
is very inefficient.
This adds
SetWithRefLM
SetWithRefRM
- similar to SetM but binds the value if it was a reference. L reads a local, R reads a return value
WIterInit
WIterInitK
WIterNext
WIterNextK
- essentially the same as the corresponding opcodes without W, but the value local is set by reference if the array element was a reference.
Will be needed for array_filter/array_map etc
This sets things up so that if we define a builtin in systemlib, we rename
the corresponding c++ builtin with the prefix __builtin_, so its still available
(in case the php builtin wants to delegate some edge cases, and to make
it easy to run comparisons between the php and c++ implementations).
Also did a little reorganization to get rid of Func::isPHPBuiltin,
and use an Attr to identify functions as builtins. C++ builtins can
still be identified by checking the Func::info() method. This is needed
to allow builtin methods defined in php (such as array_map) to lookup their
arguments in the correct context.
The change to stop zeroing locals during RetC was too
aggressive---we stopped doing it during unwinding also. When
unwinding, we need to zero locals and $this because another
destructing object may still run debug_backtrace, and we won't have
the *pc == OpRet{C,V} trick to tell us to ignore the junk.
Make sure m_received is always null at ContSend, ContRaise,
and ContNext, and this information to generate better code for these
bytecode instructions.
This diff changes HHVM so that arrays are considered to implement the
Traversable and KeyedTraversable interfaces (which have no methods).
The idea is that these interfaces will be useful for parameter type
constraints for PHP code that wants to be compatible with both arrays and
collections (and possibly other objects that implement these interfaces).
The Func*'s in the native func unit are all persistent, which means
that we can strip them, and skip the Unit::merge call on every
request. But we didnt set the flag to compact the unit if the only
things that needed stripping were Func*'s. Fixed that, and also fixed
it so all the builtins are marked persistent even in non-repo-auth
mode (funcs are not if rename function is enabled, because we implement
fb_rename_function by swapping target cache entries).
This adds a new trace macro to allow tracing to the ring buffer in release builds. It also performs normal tracing, too, like TRACE(n, ...). Converted a number of trace/log messages in the debugger to the new macro so we have this data when we get a core file. I've converted things that we've found useful lately, but this will be adjusted over time quite a lot as we discover new things that help us find problems more quickly, or find messages that turn our to be useless or spammy.
I'm committing my work on HphpArray in bite-sized pieces for easier review. This piece replaces calls to NEW with a factory method. There are many problems with operator new, starting with the fact that the allocator cannot communicate properly with the constructor.
The newly introduced factory method should return ArrayData but that causes many issues right now, so I left that step to a future diff.
This diff introduces a new "eager" vmreganchor, which we call
from some of the heaviest users of vmreganchor. For an eager
reganchor, we save the rbp, pcOff and spOff at the call site
in the MInstrState, and the reganchor itself reads it off
from the MInstrState (instead of following the rbp chain
and looking up the hash table). Currently, the set of functions
which use eager vmreganchor is based on profiling.
Fix a sigfault when we eval a snippet of PHP for the debugger. If there is no activation record (between the end of a request and PSP, before first function execution of the request, etc.) we'd segfault setting up the invocation. Fixed to tolerate that case.
I was learning from @jdelong and he said that you should use
double quotes for local includes and angle brackets for library
includes. I asked why our code was the way it was, and he said he wanted
to clean it up. I beat him to it :)
Conflicts:
hphp/runtime/base/server/admin_request_handler.cpp
hphp/runtime/vm/named_entity.h
Owen Yamauchi