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: Make optimizer lookup callee return type #17495

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Aaron1011
Copy link
Member

This provides us with more optimization opportunities. Since we optimize methods one at a time, we need to look up the return type in the domain rather than relying on the resolved return type in the target BytecodeMethod

@Lord-McSweeney
Copy link
Collaborator

Lord-McSweeney commented Aug 14, 2024

I don't think this works; for example

class A {
    public function someMethod():MyType;
}
class B extends A {
    public override function someMethod():OtherType;
}

If a type the optimizer knows is of type A's someMethod is called, the optimizer assumes the return type is of type MyType but it could also be of type OtherType. We need to verify that overriding methods have the same return type as their supermethods.

@adrian17
Copy link
Collaborator

adrian17 commented Aug 14, 2024

To be clear: it is correct and works on real SWFs, we just don't validate them as strictly as FP does (so it can break on invalid SWFs)?

@Lord-McSweeney
Copy link
Collaborator

The classes

class A {
    public function someMethod():MyType;
}
class B extends A {
    public override function someMethod():OtherType;
}

throws a VerifyError in FP. I'm working on getting it to do so in Ruffle too.

@danielhjacobs danielhjacobs added A-avm2 Area: AVM2 (ActionScript 3) T-refactor Type: Refactor / Cleanup labels Sep 17, 2024
This provides us with more optimization opportunities.
Since we optimize methods one at a time, we need to look up
the return type in the domain rather than relying on the
resolved return type in the target `BytecodeMethod`
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-refactor Type: Refactor / Cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants