AVM1 DefineFunction support #63
Conversation
kmeisthax
force-pushed
the
avm-functiondef
branch
2 times, most recently
from
592d74a
to
e9ddf44
Compare
|
It turns out that registers are easy to implement and are part of the SWF5 action model anyway, so I've decided to pull DF2 into scope. Also, I need to implement |
| /// | ||
| /// # Warnings | ||
| /// | ||
| /// It is incorrect to call this function multiple times in the same stack. |
There was a problem hiding this comment.
Shouldn't this be enforced, then? At least in development builds.
There was a problem hiding this comment.
I'm not sure what the appropriate way to do this. The best I could think of would be to set a flag in the AVM and use that to panic, but I'm not sure if that's a good idea.
There was a problem hiding this comment.
You can detect dev / test builds with if cfg! and have the flag only in those… but I don't know if that's good practice.
Alternatively, you could have a helper function / macro with a dummy implementation when !cfg(debug_assertions), which seems safer.
There was a problem hiding this comment.
I implemented something that just logs an error message if this particular situation occurs for now.
kmeisthax
force-pushed
the
avm-functiondef
branch
from
c6f541d
to
879c13d
Compare
|
I'm going to start prepping this PR to actually be pulled in. In preparation for that, here's an updated set of tests. The specific ones that I added for the branch are the following:
There's also two tests in here for cross-movie function calls. I have not yet implemented the full AVM1 version negotiation algorithm yet, and we can't load movies from other files yet, so these tests are nowhere close to passing. |
kmeisthax
force-pushed
the
avm-functiondef
branch
4 times, most recently
from
1e52bf5
to
33d89c8
Compare
|
It fails some clippy lint that I think we're planning on removing anyway, but I think this is ready to merge now. I had to fix |
kmeisthax
changed the title
kmeisthax
force-pushed
the
avm-functiondef
branch
from
8cedca9
to
6e6051e
Compare
This necessitates the use of a copy of SWF data into the GC arena, along with unsafe (and possibly unsound) pointer manipulation to get the action reader to hold a GC pointer.
Functions that manipulate the stack now run inside of `with_current_reader_mut`, which calls a given function with a Reader for the current stack frame. If the stack frame still exists after that code runs, we update it's PC with the Reader's position.
…nother stack frame to run.
…ly check for slashpaths
1. We no longer implicitly return Undefined unless we're specifically returning from a function (this also keeps actions from filling the stack with Undefined) 2. With scopes are now always inserted behind the current set of locals rather than overriding them 3. `ActionSubtract` now subtracts (instead of adds)
…ust hand the avm an activation object directly
…verwriting them should trigger their setters. Define, since it's intended for setting locals only, always uses force-set and does not trigger setters.
…ate a new scope chain based off the selected active clip.
Herschel
force-pushed
the
avm-functiondef
branch
from
6e6051e
to
38fbb33
Compare
* Remove clone calls from Copy objects * Used Iterator::cloned() instead of manually cloning * Pass swf::Function into AvmFunction2::new() * Use action_clone_sprite
Herschel
force-pushed
the
avm-functiondef
branch
from
38fbb33
to
b6eba80
Compare
| Action(Avm1Function<'gc>), | ||
|
|
||
| /// ActionScript data defined by a previous `DefineFunction2` action. | ||
| Action2(Avm1Function2<'gc>), |
There was a problem hiding this comment.
Is there a benefit to having a separate Avm1Function and Avm1Function2? For DefineFunction1, could we just default all the preload flags to false to get rid of some code duplication?
There was a problem hiding this comment.
Hmm, you might be right. DF2 is a strict superset of DF1, I think...
| } | ||
| } | ||
|
|
||
| impl<'gc> Activation<'gc> { |
There was a problem hiding this comment.
This is bikeshedding, but the ECMAScript spec technically calls this object an "Execution Context", of which the "Activation object" is a single part. The Execution Context contains "this + locals + scope chain", while activation object is just the "locals" part. I don't think the distinction is significant, but it might be worth putting a note in the comments.
There was a problem hiding this comment.
Right. Actually, we're even a little further off from ECMAScript spec: Locals are handled by creating a blank object and sticking it on the end of the current scope chain rather than keeping them in the execution context as a separate concern. I don't believe there is any particular observable change from the viewpoint of running code, but I always have to keep that in mind when the spec is talking about scope chains.
| /// | ||
| /// Registers are stored in a `GcCell` so that rescopes (e.g. with) use the | ||
| /// same register set. | ||
| local_registers: Option<GcCell<'gc, Vec<Value<'gc>>>>, |
There was a problem hiding this comment.
Eventually it'd be nice to use SmallVec here, since most functions will only use a few registers and it could avoid an allocation.
| pub fn new_closure_scope( | ||
| mut parent: GcCell<'gc, Self>, | ||
| mc: MutationContext<'gc, '_>, | ||
| ) -> GcCell<'gc, Self> { | ||
| let mut closure_scope_list = Vec::new(); | ||
|
|
||
| loop { | ||
| if parent.read().class != ScopeClass::With { | ||
| closure_scope_list.push(parent); | ||
| } | ||
|
|
||
| let grandparent = parent.read().parent; | ||
| if let Some(grandparent) = grandparent { | ||
| parent = grandparent; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| let mut parent_scope = None; | ||
| for scope in closure_scope_list.iter().rev() { | ||
| parent_scope = Some(GcCell::allocate( | ||
| mc, | ||
| Scope { | ||
| parent: parent_scope, | ||
| class: scope.read().class, | ||
| values: scope.read().values, | ||
| }, | ||
| )); | ||
| } | ||
|
|
||
| if let Some(parent_scope) = parent_scope { | ||
| parent_scope | ||
| } else { | ||
| GcCell::allocate( | ||
| mc, | ||
| Scope { | ||
| parent: None, | ||
| class: ScopeClass::Global, | ||
| values: GcCell::allocate(mc, Object::bare_object()), | ||
| }, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
My gut feels like we could avoid the Vec and GcCell allocations here, since this is basically a clone of the scope chain with certain scope classes filtered out, if I understand correctly. For example, I think the Vec could be avoided by constructing the Scope objects in the initial loop and fixing up the parent of the previous one as you go along.
Maybe longer term Scope can use an iterator-like API for the iteration in resolve etc., and these just could ignore certain ScopeClasses as appropriate while iterating. Then the scope chains will only be appended to and never have to be copied?
There was a problem hiding this comment.
The iterator approach seems difficult, because we might have a function defined in a with block that calls another function with another with block, etc. So it wouldn't be as simple as "resolve without with blocks", since we only want those defined in the current function.
Removing the extra allocations is an easy perf win though.
This PR constitutes an implementation of activation objects, scope chains, and user-defined functions for ActionScript 1/2 (AVM1) code. User functions are defined as a reference to some part of the SWF they were loaded from, as well as the version of that SWF (so we can emulate version-specific behavior).
TODOs
ActionDefineFunctionimplActionCallFunctionimplthisargumentsobjectActionWithimplActionCallimpl (call a timeline frame as if it was a function)ActionDelete/ActionDelete2implActionEnumerateimplActionStoreRegisterimplActionGetMember/ActionSetMemberimplActionDefineFunction2impl