It's not ok for the unwinder to use a reference to elements
living in the m_faults array, since the unwinder can re-enter the VM
when calling destructors (or the FunctionExit hook). If one of those
re-entries does exception handling, it can modify m_faults.
Additionally, gets rid of VMPrepareThrow and instead just throw Object
and use the same case as we do when exceptions came from an extension.
I had to fix an assembler test catch handler to actually catch to keep
the assertion about m_faults on re-entry correct.
When interp'ing an N-instruction tracelet, we ask the interpreter
to step through N instructions. The interpreter treats FCall as one of
those instructions, and happily starts interp'ing into the callee. This can
cause us to form weird tracelets in the callee that have no business
existing. Instead, break on any control-flow we encounter.
Array is already a SmartPtr, wrapping it in another is pointless.
Except it turns out that there are include dependencies preventing
us from using Array here. Use boost::intrusive_ptr to wrap an
ArrayData instead.
This diff refactors some of the VM's logic for iterators (with a focus on
mutable iteration), delivering several improvements:
1) MIterCtx was renamed to MArrayIter, and the m_key and m_val fields
were eliminated.
2) Eliminated the need for MArrayIter to dynamically allocate a
MutableArrayIter object, and removed other layers of indirection as
well.
3) Reduced the size of HPHP::VM::Iter from 64 bytes down to 32 bytes.
4) Removed the "if (siPastEnd())" check when adding a new element to an
HphpArray or a ZendArray.
5) Moved all of the iterator logic into a single .cpp file.
This diff reworks FullPos's to point to current element instead of pointing
to the next element. It also splits up the IterFree instruction into two
instructions (IterFree and MIterFree). These changes allowed various logic
to be simplified and data structures to be reduced in size. There is
definitely more opportunity for refactoring, but I know the JIT helpers for
iteration have been carefully tuned and so I'll leave further refactoring
for future diffs.
Finally, I spent a little time cleaning up the bytecode spec a bit, mostly
with respect to iteration.
When we skip EHEnts for catch blocks that we aren't going to
enter, we still need to count them in the handledCount so that we run
the appropriate fault funclet (see the unit test). Before this
change, it would fail to account for the catch handler and end up
running the fault funclet twice, leading to an assertion about
IterFree trying to free an already-freed iterator.
Since we now use the original throw location to figure out
which EHEnt applies when propagating from a fault funclet, we need to
check whether we've already unwound the ActRec before doing it again.
Doing it again could cause issues with nested FPI regions (hitting an
assert in sandboxes).
... is that once you put them into the codebase, the odds of them
showing up where you dont want them go up dramatically.
With a gcc 4.7.1 -O0 build I hit a consistent crash in production
where debugBacktrace had stepped too far, thinking that the
first non-vm frame was in fact a vm frame. It did so because
it looked at the word above the frame, and saw that it contained
c_Continuation::kMagic.
It turns out that fixupWork's isVMFrame left kMagic in $rdx if the
frame really was a generator ($rdx was dead, but still). We then
return from the tc via a serviceReq that doesnt need $rdx (so doesnt
set it), and then (with a following wind, so that $rdx hasnt been
smashed yet) enterTCHelper stores $rdx in info.args[1].
By another staggering coincidence, info.args[1] is at exactly the
right address to make the call into enterTCHelper /look/ like a
a continuation (based on kMagic). So if we then have a catch
which re-enters the TC there's a good chance kMagic is still there
and the next debugBacktrace (or uncaught exception) will crash.
This diff rewrites everything in terms of the C++ stack; we
now say that its a VM frame if its not on the C++ stack.
In the interpreter, neither ContEnter nor ContExit checked the
surprise flags, resulting in the time being attributed to the
callers. In the jit, ContEnter checked the surprise flags, but
ContExit did not, resulting in what appeared to be highly
recursive profiles.
This change is mostly for FB internal organizational reasons.
Building is not effected beyond the fact that the target now
lands in hphp/hhvm/hhvm rather than src/hhvm/hhvm.
mwilliams