Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVM2 interpreter #404

Merged
merged 189 commits into from
Jul 20, 2020
Merged

AVM2 interpreter #404

merged 189 commits into from
Jul 20, 2020

Conversation

kmeisthax
Copy link
Member

@kmeisthax kmeisthax commented Feb 22, 2020

This implements an AVM2 interpreter. AVM2 is the virtual machine that ActionScript 3.0 programs compile into; it's completely separate from AVM1 and almost became part of the web platform itself in the form of ECMAScript 4 (no published standard exists) and Adobe Tamarin (FOSS but MPL trilicensed instead of Apache).

Notable features include packages, namespaces, access control, E4X, type assertions, a very explicit scope stack, no primitive values (if the one ES4 overview PDF I could find is accurate), and more. Coming from modern JavaScript, or even AS2; this feels like a completely different programming language and it took a while to understand very basic things about the VM. (This is probably why non-Flash web developers outright rejected ES4.) Nevertheless, it's part of Flash, so we need to implement it as it gates a whole bunch of interesting content.

In this PR

  • Avm2 interpreter state
  • Values, objects, activations, functions, namespaces, properties, and slots
    • Interpreter uses class and trait definitions separate from the ones in swf
    • Interfaces, including their effect on istype instructions
  • Trait instantiation
    • Re-do trait instantiation because traits exist on instance objects directly
    • Make sure traits actually don't get instantiated on prototypes
  • Control flow instructions
    • jump
    • iftrue
    • iffalse
    • ifstricteq
    • ifstrictne
    • Other control flow instructions are future work
  • Object iteration
  • Object, Function builtins
  • Class class
  • AVM2 StageObject - see future work
  • SymbolClass tag - see future work
  • FileAttributes tag & VM isolation - see future work
  • Tests
  • We can't implement Function.prototype.apply without working Arrays, which is outside the scope of this PR
  • We can't test closures because the test I wrote for them uses coerce.

PR Roadmap

The following are features out of scope of this PR. Due to the fact that this PR's scope has more or less been completed, I'm working on them in separate branches. If they're relatively "complete" before this PR is merged in, they may be merged in.

  • Primitive type coercion, abstract comparisons, and abstract equality - avm2-primitives on my user's repo.
    • Numerical calculations - some implementations exist on the above but will be given their own grandchild PR
    • String manipulation - not yet started
    • Primitive boxing - not yet started
  • Native builtins - avm2-nativeclasses on my user's repo. Covers defining ES4 classes in Rust.
    • AVM2 StageObject - TObject implementation that holds stage objects, plus native implementations of the builtin display object classes.

@kmeisthax kmeisthax force-pushed the avm2-interpreter branch 2 times, most recently from 6079a48 to b71bdf6 Compare February 24, 2020 23:34
@kmeisthax kmeisthax changed the title [DRAFT] AVM2 interpreter AVM2 interpreter Mar 9, 2020
@kmeisthax
Copy link
Member Author

I am releasing the draft lock on this PR and requesting review and merge. Hopefully it passes CircleCI.

I'm deliberately walling off a whole bunch of future AVM2 features for future PRs, as mentioned in the PR description. This PR already has 143 commits and makes object_prototyping, a PR I had to cut up into four painful-to-rebase pieces, look minimal. I've specifically selected future work that isn't likely to have interdependencies between future PRs, and thus can be developed separately on top of the work I've done bootstrapping a basic AVM2 interpreter.

There are approximately 31 tests, a significant number of which challenged my notions about how this interpreter should work. I think I at least have the object model locked, though I wasn't able to write a test that uses slots. In fact, clippy seems to think Slot::Occupied is unreachable, even though there's a function constructing it. I disabled that warning for now.

@Justin-CB
Copy link
Contributor

Is there any ETA on this pull request?

@jfmherokiller
Copy link

Is there any ETA on this pull request?

Out of my own curositiy i tried to merge this into master (current as of today) and it seems this branch is way behind. Just based off a cursory observation (im no rust master) we might need to create an avm super type that encases both avm1 and avm2 and cast based upon the information extracted from the swf.

@kmeisthax
Copy link
Member Author

@jfmherokiller There's no need to do that, I just need to resolve the merge conflicts. AVM1 and AVM2 are supposed to be entirely separate; the conflicts are in other parts of the player where I added AVM2 stuff to.

@kmeisthax
Copy link
Member Author

I should also point out that this does not contain the capability to play AS3 games. It can't even add numbers yet - it's purely the AVM2 interpreter loop, object model, and just enough builtins to get tests to run.

@jfmherokiller
Copy link

jfmherokiller commented Apr 21, 2020

the question then i have is how will the player distinguish between an avm1 and avm2 swf?
Edit: just reread your discription and checked my own research yep its the fileattibs tag

@kmeisthax
Copy link
Member Author

As you can tell by that massive list of new commits I have rebased the AVM2 PR and resolved the merge conflicts.

Given @Dinnerbone's recent work with init actions I'm wondering if ABC files should also execute immediately rather than resolving on the action queue.

@jfmherokiller
Copy link

jfmherokiller commented Apr 21, 2020

I will say I did succed in adding a kind of standin for FileAttributes (I just started using rust today so code will be very basic) in movie_clip.rs.
mydiff.txt
file attached is the git diff showing the changes

@kmeisthax
Copy link
Member Author

So, that's great, but kinda out of scope for this PR. I specifically excluded anything to do with either FileAttributes, or tying display objects into the AVM2 interpreter, because this PR is already fairly sprawling and I want @Herschel's eyes on it and this merged before I start doing other related work.

Whenever this actually gets merged, and we start work on an "AVM2 DisplayObjects" PR, then we'd want to start parsing FileAttributes so we'd know how to instantiate a given movie. The relevant flags would both have to set a bit on the containing SwfMovie (so that contained movieclips can't mix VMs) and instantiate the entire movie hierarchy as either AVM1 or AVM2. Furthermore, I'm a little wary of parsing FileAttributes in preload because there are script actions and instantiations that can happen before a FileAttributes tag has been encountered which do need to know what VM is allowed to run for a given movie. Ideally, we'd scan the SWF's root movie clip before preload (or even before making the initial MovieClip) so we could get at those attributes and use that to instantiate the correct VM's Object.

@kmeisthax
Copy link
Member Author

Turns out this thing had been sitting unmergeable for a good week or two 'cause I didn't notice the fact that beta/nightly clippy tests are now being run.

Also, given that #675 landed I'm now considering adjusting the AVM2 Value and Object to match...

@kmeisthax kmeisthax force-pushed the avm2-interpreter branch 2 times, most recently from 45bbf03 to c7cd509 Compare June 25, 2020 21:49
…hat pulls strings from the ABC file now uses `AvmString`s.
…enting us from making multiple copies of the same string.

For good measure, most of the other methods in `value` for retrieving pool primitives now also use `TranslationUnit` instead of `AbcFile`. This is the result of a handful of cascading changes throughout the project, and itself caused a few more.
Holding a `Ref` on a garbage-collected object inherently extends any borrow locks on that object. Since ABC files are references already, taking a `Ref` to them only helps to skip the refcount update. This is less useful than expected: in most situations, using `abc_ref` causes double-borrow panics. The few methods that can use it are going to be fragile in the face of future refactors, so I'm nipping the problem in the bud now.
Copy link
Member

@Herschel Herschel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this! I think we should have avm2::AvmString and avm2::PropertyMap (at least aliased or copy+pasted from the Avm1 versions for now), as these will almost certainly have differences than the avm1 versions, and we should at least have types in place to make it easier for us to modify these later.

There are also some unimportant naming questions that I feel strongly don't feel strongly about (how closely should we try to mirror the avmplus naming conventions?)

Otherwise I think this is ready to merge, and I will merge this whenever you're ready.

swf/src/avm2/read.rs Outdated Show resolved Hide resolved
Comment on lines +17 to +25
pub enum Value<'gc> {
Undefined,
Null,
Bool(bool),
Number(f64),
String(AvmString<'gc>),
Namespace(Namespace<'gc>),
Object(Object<'gc>),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed: Should we call this Atom (since this is what avmplus calls it?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong opinion either way.

I did use Value specifically because it matches the AVM1 Value. I doubt we're going to have people used to hacking on avmplus wondering where Atom is here, as much as we're going to have people used to hacking on Ruffle's AVM1 interpreter wondering where Value is.

core/src/avm1/string.rs Show resolved Hide resolved
core/src/avm2/script_object.rs Outdated Show resolved Hide resolved
core/src/avm2/names.rs Show resolved Hide resolved
core/src/avm2/method.rs Outdated Show resolved Hide resolved

#[derive(Copy, Clone, Debug, Collect)]
#[collect(no_drop)]
pub struct TranslationUnit<'gc>(GcCell<'gc, TranslationUnitData<'gc>>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TranslationUnit feels like a weird name to me. avmplus calls this PoolObject which I don't think is much better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the name came about while I was switching the AVM2 interpreter away from pulling ABC traits and data directly to storing them in other structures that could be dynamically created (which we need for avm2-nativeclasses.) I can't call it AbcFile because that's already in use by swf, so TranslationUnit (a term used by C/C++ compiler specifications to refer to a single source file) seemed like a good option, since, as far as I could tell I was getting one DoABC per source file in my tests...

If you have a better option I'll definitely change the name.

core/src/avm2/script.rs Outdated Show resolved Hide resolved
Comment on lines 79 to 93
if let Some(method) = write.methods.get(&method_index) {
return Ok(method.clone());
}

drop(write);

let method: Result<Gc<'gc, BytecodeMethod<'gc>>, Error> =
BytecodeMethod::from_method_index(self, Index::new(method_index), mc)
.ok_or_else(|| "Method index does not exist".into());
let method: Method<'gc> = method?.into();

self.0
.write(mc)
.methods
.insert(method_index, method.clone());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like we should be able to use the entry API with or_insert_with, although I guess the issue is that from_method_index does some txunit.read().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timing of when GcCell locks are held and released is important here (which is why I used the "wrapped GcCell pattern" here). Due to the way traits work, all four of these methods can recurse into one another on the same stack at any time, so I'm being highly conservative with how long locks are actually being held.

BytecodeMethod never needs a write lock on the translation unit, and it only ever reads it to get an ABC. In fact, it reads it three times too many because this used to be abc_ref (which caused other double-borrow problems). I'm going to change this particular constructor into taking both a TranslationUnit and an AbcFile, so it doesn't need to hold either read or write locks and we can use entries.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that won't work, but not for the reason you'd think: or_insert_with is infallible. We need to generate a verification error if the method index doesn't exist, which we can't return inside of the or_insert_with callable. Ergo, I'll be leaving this as-is.

(This seems to be a problem with a lot of Rust methods that take closures...)

core/src/avm2/script.rs Outdated Show resolved Hide resolved
@Herschel
Copy link
Member

Ok, let's get this in! Thanks for the patience while this PR sat around for a long time. I'm excited to make progress here.

(BTW, I meant to say that I don't feel strongly about the name stuff in my earlier post, sorry if that came off the wrong way).

@Herschel Herschel merged commit 1a5d7fe into ruffle-rs:master Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants