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
It turned out a lot of the namespace stuff still worked. The biggest thing for the first pass is that we don't fallback to the global function or constant if there isn't a namespaced one.
Also, when a constant has a ##\## anywhere in it it throw an error when it isn't defined, instead of assuming the string.
PolicyArray splits the ArrayData implementation in two parts. ArrayShell implements the baroque ArrayData implementation in terms of a much smaller statically-bound core that concerns itself exclusively with the storage strategy for the array. This way the two aspects can be worked on separately, and different stores can be easily plugged into the given ArrayShell.
To give a better perspective, the featured SimpleArrayStore has about 340 lines all told (including solid documentation), whereas ArrayShell has some 1100 lines. The shell can be reused with other stores of unbounded sophistication. The store needs to implement only 22 primitives, some of which are trivial. Basically a new store implementation saves 1100 of difficult-to-get-right lines of code right off the bat, and only needs to focus on implementing 22 well-defined primitives.
Things to watch for when reviewing:
- Is the store API small/expressive enough? What should be added/removed?
- Are various forms of duplication present? If so, how could code be factored better?
- Any subtle change in semantics from HphpArray and friends that should be minded?
Known issues:
* Currently ArrayShell is defined as:
class ArrayShell : public ArrayData, private SimpleArrayStore { ... };
when in fact it should be:
template <class StorePolicy>
class ArrayShell : public ArrayData, private StorePolicy { ... };
Extenuating circumstances related to allocator design prevent use of templates at this point. I'll work on these in parallel with the review.
* growNoResize is almost always prefaced by a test-and-grow. This sequence should be encoded in a function.
* The growth policy is spread all over the place in a subtle form of duplication.
* The current store design makes almost no effort to be particularly efficient.
This func took 2 params and ignored the second one. While I was there I made the functions call eachother. Hopefully inlining is smart enough to let me clean this code.
I got a bit carried away, but I think this is better.
In order to ensure that we can continue to interpret code when setting up a step out, even if we're stepping out to jitted code, we set the saved RIP in each ActRec on the stack to the retFromInterpretedFrame() helper. However, this is wrong for generator functions. There is a different variant of the function for generators, so use that when appropriate. Also added a check to ensure we only changed return addresses to jitted code. Mark thought of a rare case where we could have a return address to a C++ helper in there, so leave those alone.
The debugger's Instrument command does not work with the JIT; any instrumentation requests will be ignored for jitted code. We believe that no one is using this at this time. I'm starting with a small diff to disable the command and remove the impact it has on the interpreter for now, just in case we're wrong. Once the command has been disabled for a few weeks I'll come back and remove all of the code (task 2376711).
InstHelpers was just pure dead code, so I nuked it.
Rather than test RuntimeOption::EvalJit and 5 thread locals to determine
whether or not to run the jit on each re-entry, maintain one thread local.
Make various RequestInjectionData fields private to ensure that the jit
flag is kept in sync.
This cleans up the code a lot, and takes it out of various hot
paths. It will impact perf for requests where intercepts are used,
but no longer penalizes requests that don't use intercepts.
Herman Venter