The late static bound class was being used for the context. This
was wrong, but before this change there was no context at all, so
it would have taken newly written code to expose the bug.
However, a given continuation can be instantiated from a large number
of different lsb classes (but only one context class) which meant that
we got an explosion of unnecessary translations.
Fixing various bugs all over the VM that make assumptions about RefData
and TypedValue layout. Here are the assumptions fixed by this diff:
offsetof(RefData, m_tv) == 0. Both JIT's assumed this in many subtle
ways, by punning RefData* as TypedValue* without adding an offset.
This assumption also causes RefData._count to overlap TypedValue.m_aux,
which constraints TypedValue layout.
offsetof(TypedValue, m_data) == 0. gen_ext_hhvm.php assumes you
can cast TypedValue* to Value*; the JITs often weren't using
offsetof(TypedValue, m_data) in their addressing calculations. HHIR
assumed return-by-value TV's have m_data/m_type in rax/rdx, which
can change when TV layout changes.
offsetof(TypedValue, m_type) > 8 is an assumption baked into the
pass-by-value register assignment logic in HHIR's codegen.cpp; if
the type is in the low word, register assignment is swapped.
sizeof(TypedValue::m_type) == 4. We used dword-sized operations
in both JIT's when accessing m_type. Now, we use helper functions
that are sensitive to sizeof(DataType)
Configuration:
DEBUG=: (opt) same layouts as trunk for RefData & TypedValue
DEBUG=1: (dbg) new RefData layout (m_tv doesn't overlap RefData::_count)
PACKED_TV=1, DEBUG=*: new RefData and TypedValue layout.
I've found several bugs recently due to the TypedValue plausible
check being too loose. Our DataType enum values are now sparse,
and we miss garbage that happens to fall inside the [-10, 127]
range, which is >50% of possible garbage values.
Merge ContDone+ContExit into ContRetC with added support of passing results. This variable passing mechanism is not exposed to the PHP as the ReturnStatements in generators do not contain result expression. However, this is exposed by restored hphp_continuation_done() built-in to allow experimentation.
The idea is that once we introduce ContYield opcode (merge of all opcodes used by YieldExpression), we could change ContRetC and ContYield to leave result and done-status on the stack and leave it up to the caller (ContNext/ContSend/ContRaise) to fill in Continuation fields. This will make these opcodes more generic and useful for other things, while allowing us to move some properties to the VM and kill opcodes like ContCurrent.
Continuation::send() uses m_received field to transfer the value inside
the continuation. This field remains set until the continuation is
iterated the next time.
Unset this field early by ContReceive so that the memory can be
reclaimed.
Of the four horsemen of the SmartAllocator, ArrayData was the only virtual
call. This meant an extra layer of indirection when coming from the TC
to allow the c++ compiler to emit its virtual call, and slightly larger
callsites when using non-generic paths.
While we're moving in this direction, consolidate ArrayData introspection
on its type enum. isSharedMap() was previously implemented with a vtable
slot, and we had no way of asking if an ArrayData was a NameValueTable.
They both returned the late static bound class, not the context
class. This meant that eg "constant('self::FOO')" was actually
returning what "constant('static::FOO')" should have done.
In addition, we often want the Class*, not its name, so
change them to return Class*. The remaining places that then
read the name from the Class* should be fixed to use the Class*
directly (in a later diff).
Finally, noticed that while "defined()" was recently fixed to
support "static::", "constant()" was not. Pulled out a common
function to find the correct Class*.
Avoid punning TypedValue* to String&/Array&/Object& in FCallBuiltin
(all three implementations). Our native function calling conventions
require passing pointers into a TypedValue for these types, and
pointers-to-scratch for return values.
In the HHIR case, I removed the optional "return pointer" argument
from the IR CallBuiltin instruction. The C++ value-passing ABI details
are now handled in cgCallBuiltin and are no longer exposed in the IR.
The argument types are still PtrTo*, but we handle the address fixups
in CodeGenerator.
The interpreter fix was different than the jit/ir fix because ##translateFPushObjMethodD()##'s ##i.inputs[0]->rtt.valueClass()## is null (with a TODO in the code to make it not null). It then goes into the slowpath.
Another one like this and I think I should have a different attribute for static closures :(
The only places where ReturnStatement is constructed are:
- onReturn(check_yield=true) -> not allowed in generator
- onReturn(check_yield=false) -> coming from transform_yield_break, right after creating hphp_continuation_done()
- MethodStatement, end of function call -> hphp_continuation_done() is created at end of generator in prepare_generator()
Emitter is emitting ContExit in ReturnStatements used in generators. As
can be seen from the analysis above, it's always preceded by emitting
ContDone from hphp_continuation_done(). Let's emit ContDone inside the
ReturnStatement directly and kill usage of hphp_continuation_done().
transform_yield_break() becomes a simple onReturn(check_yield=false), so
let's inline it into onYield and create ReturnStatement directly. After
this change, check_yield flag is always true and can be killed.
ContExit was also used after emitting a generator method in case the end
of method is still reachable. ContDone is added so that the generator is
properly closed. I believe this is never actually used, as MethodStatement
creates ReturnStatement at the end of method anyway.
Unhack the parser and introduce YieldExpression that emits the
equivalent set of opcodes that were emitted by bunch of
expressions/statements generated by parser before.
YieldExpression expects evaluation stack to contain just the value
being yielded, so {,List}AssignmentExpression need to evaluate RHS
first. The previous code had the same behavior.
This will let us consolidate continuation-related opcodes and make
them less tied with continuation objects.
Most of this is pretty boring and mechanical. I added
VectorProp and VectorElem flags to help deal with the increasing
number of vector-related opcodes.
I added the check for this in the interpreter but ##f_array_map## re-enteres the VM via a different path than FCall. Here is the equivilent check for the VM.
PHP added this in 5.4 so that you can say your closure shouldn't capture ##$this##. https://wiki.php.net/rfc/closures
Should we add it? Many of their unit tests use it.
Instead of putting a boolean somewhere I used the same attr framework and set the static bit there. Thoughts? It only needs one change in the ##FPushFunc##.
In Zend 5.3 they decided that closures should inherit the ##$this## from the containing scope. This brings us close to paraity with them. The remaining thing is to make
function() use ($this) {}
fatal after we purge it from WWW.
This diff adds a relativeOffsets option to Disasm, which will print
code addresses as relative offsets from the beginning of the tracelet instead
of absolute addresses. This format makes it much easier to compare the relative
sizes of the instructions selected by the two jits. I also changed the runtime
option to be a double instead of a bool, which is used as the cutoff ratio for
which tracelets to print. Setting it to 1 will print tracelets where the ir
made larger code than tx64, setting it to 2 will print tracelets where the ir's
code is at least twice as big as tx64's, etc...
Replace "collection" with "collections" in various file names since
we typically use the plural form in conversation and documentation.
Add set() and removeAt() methods needed for collection interfaces. Also
add the KeyedIterable and KeyedIterator interfaces.
Add __construct() methods for collections
This diff removes initializing stores to TypedValue._count, renames
_count to m_aux, and makes m_aux a union with members typed
and named according to their specialized uses. The few remaining
uses of that field for random tweaks are more obvious and easy to
grep for.
TypedValue no longer extends Value, (allowing m_data to move to a
different offset in the future), and Variant now extends TypedValue,
so we only have to maintain one definition.
HphpArray now explicitly uses TypedValue.m_pad instead of overlapping
TypedValue with an anonymous struct, again so we don't have to maintain
another structure to match TypedValue's layout.
The JIT's were using offsetof(TypedValue, _count) all over the place
for access to String/Array/Object/RefData::_count. Instead, use
FAST_REFCOUNT_OFFSET.
This actually was pretty broken already. If you defined a new function in the ##create_function## string it would return that function but it wouldn't move it to the right unit so it can't execute. A big mess. For example:
$ cat a.php
<?php
$a = create_function('', 'function b() { return 3; }');
$a();
var_dump(b());
$ php a.php
HipHop Fatal error: Undefined function: b in /data/users/ptarjan/hphp/hphp/a.php on line 4
$ hhvm a.php
int(3)
Support == and != operators for collections. Also fix some bugs the <, <=,
>, and >= operators when comparing two objects or when comparing an array
with an object.
We only used it to get the values of certain class constants,
and to define the ObjectStaticCallbacks for every class.
We can put the class constants directly into the class_map
(we should have done that before for perf reasons), and then
the only remaining use of ObjectStaticCallbacks is to proxy
the Class* for each builtin class. So just use the Class*
directly.
Once this is in, Im just a small step away from eliminating
make -C hphp/system - so Im leaving a lot of dead code here.
Its going to be easier to delete it en masse, rather than
try to pick and chose now.
This basically targetted symbols.php, and Globals, but ended up
killing a lot more. I could keep adding more and more, but
this seems like a good point to stop and continue with
another diff.
The logic for unwinding generator frames was assuming that the previous
frame was always a VM frame. This used to be a correct assumption, but now
we have invokeContFunc which allows native code to call generator functions.
This diff fixes the unwinder to correctly computer the bottom of the eval
stack in the case where the previous frame is a native frame. It also
updates invokeContFunc to properly decref the return value.
Instead of having the body of the closure be in the ##__invoke()## on the ##Closure## class, instead we make an anonymous function on the real class and put the body there. The signature for this function is:
function methodForClosure$1234($arg1, $arg2, ..., $use1, $use2, ...)
and then ##__invoke## now just takes all the params that were passed to it, puts them as the first args to the anonymous function, then takes all the use variables it had saved up and passed them in as the next params.
I tried to not have an ##__invoke## at all, but I ended up basically doing the same parameter and use var repacking in iopFCall (and would have had to do it in x86 code too). I opted for doing the rejiggering in bytecode. If I did it in raw PHP I think it would have been much slower with many ##func_get_args()## and array operations.
We don't display the stack frame for ##isNoInjection()## functions, but the backtrace does set the previous call lines wrong.
This came up when I made the Closure::__invoke to be hidden, but the line numbers were wrong.
I was debugging this method and jumping into a macro that is 20 lines is much harder than a method. Shouldn't the compiler inline it if that is the right thing to do anyways?
There also was a hidden variable ##numArgs## which is what was messing me up.
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