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 verifier #14631

Merged
merged 25 commits into from
Jan 30, 2024
Merged

Conversation

Lord-McSweeney
Copy link
Collaborator

@Lord-McSweeney Lord-McSweeney commented Jan 6, 2024

This is #14563 but with state merging removed, since I couldn't figure it out. It also includes some basic optimizations, and it keeps verification of local registers in (since that's not dependent on the control flow).

TODO:

  • Something in try..catch blocks is broken (I keep losing working reproductions, but FP is throwing verify errors where Ruffle is not) Not relevant anymore
    • Also only verify exception target if there are any potentially error-throwing opcodes present in the from..to Fixed
    • All error-throwing opcodes in the from..to should act as a potential jump to the exception target (just like iftrue, iffalse, etc)- example: verify.zip Fixed, like above
  • More brokenness Not relevant anymore
  • Add many two tests
  • Store parsed_code on the method itself rather than in AbcMethodBody
  • Add a custom Op type in AVM2 that stores already-resolved constant pool entries
  • Storing an entire verified AbcMethodBody seems unnecessary, and is mostly leftover from Adrian's original work

The optimization framework that this PR implements should work well with #7920.

Supersedes #14563.

Future optimizations:

@Lord-McSweeney Lord-McSweeney mentioned this pull request Jan 6, 2024
8 tasks
@Lord-McSweeney Lord-McSweeney force-pushed the avm2-verifier-no-merge branch 3 times, most recently from 86c94d3 to bf58273 Compare January 7, 2024 08:12
core/src/avm2/verify.rs Outdated Show resolved Hide resolved

let mut previous_op = None;
for (i, op) in code.iter_mut().enumerate() {
if let Some(previous_op_some) = previous_op {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we unnest this a bit? (And definitely rename previous_op_some)

e.g.

let previous_op = if let Some(previous_op) = previous_op {
  previous_op
} else {
  previous_op = Some(op.clone());
}

In theory we could even do something like

previous_op = code.first();
for (...) in code.iter_mut().enumerate().skip(1) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't do let Some(previous_op) = previous_op because the optimizer needs to set the Option value of previous_op. I'll try unnesting it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried unnesting it but I realized that there are likely going to be optimizations that we can do that would work despite previous_op being None, and/or those that would work even when the next op is on a jump target.

| Op::Not
| Op::IsType { .. }
| Op::IsTypeLate => {
previous_op = Some(op.clone());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could write this as previous_op = mem::replace(op, Op::Nop), but I am not sure if that is really worth it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this current implementation is more readable.

core/src/avm2/verify.rs Outdated Show resolved Hide resolved
@Lord-McSweeney Lord-McSweeney force-pushed the avm2-verifier-no-merge branch 2 times, most recently from 6c6cd46 to f01c5a7 Compare January 7, 2024 22:44
@torokati44 torokati44 force-pushed the avm2-verifier-no-merge branch from c3b47f3 to f552d34 Compare January 11, 2024 23:23
@Lord-McSweeney Lord-McSweeney force-pushed the avm2-verifier-no-merge branch 3 times, most recently from f7702cd to e187b1b Compare January 15, 2024 03:13
@Lord-McSweeney Lord-McSweeney marked this pull request as draft January 15, 2024 03:27
@Lord-McSweeney Lord-McSweeney marked this pull request as ready for review January 15, 2024 03:35
@Lord-McSweeney Lord-McSweeney force-pushed the avm2-verifier-no-merge branch 2 times, most recently from a5509f4 to 99d9d44 Compare January 16, 2024 01:38
@Lord-McSweeney Lord-McSweeney enabled auto-merge (rebase) January 16, 2024 01:38
@Aaron1011 Aaron1011 disabled auto-merge January 16, 2024 01:40
false
}

fn pool_int<'gc>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we move these to script.rs? It already has a bunch of similar functions, so it'd be more consistent (and decrease verify.rs size a bit too).
Also, any chance this could be unified with abc_int() (and friends)? As they are pretty much duplicates, sans 0 treatment. (this'd also probably make function with invalid default values throw a correct VerifyError "for free"). Can be left for a follow-up though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to leave this for a follow-up to also fix more issues with namespace and string pooling

core/src/avm2/verify.rs Outdated Show resolved Hide resolved
core/src/avm2/verify.rs Outdated Show resolved Hide resolved
core/src/avm2/verify.rs Show resolved Hide resolved
@Lord-McSweeney Lord-McSweeney marked this pull request as draft January 18, 2024 03:19
Lord-McSweeney added 25 commits January 29, 2024 22:25
…s since those are equivalent to the corresponding CoerceX opcodes
…ix `optimize` panicking when out-of-bounds local registers are mentioned
…ing recursively

This fixes a stack overflow on one test.
…::ops_can_throw_error`

This requires a minor change to `Activation::op_lookup_switch`
@Lord-McSweeney Lord-McSweeney enabled auto-merge (rebase) January 30, 2024 06:26
@Lord-McSweeney Lord-McSweeney merged commit dbc1015 into ruffle-rs:master Jan 30, 2024
13 checks passed
@Lord-McSweeney Lord-McSweeney mentioned this pull request Feb 3, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-avm2 Area: AVM2 (ActionScript 3) T-perf Type: Performance Improvements
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants