Skip to content

Comments

Issue 14211 - Compiler should devirtualize calls to members of final class#4427

Merged
braddr merged 1 commit intodlang:masterfrom
9rnsr:fix14211
Feb 21, 2015
Merged

Issue 14211 - Compiler should devirtualize calls to members of final class#4427
braddr merged 1 commit intodlang:masterfrom
9rnsr:fix14211

Conversation

@9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Feb 21, 2015

https://issues.dlang.org/show_bug.cgi?id=14211

Add CallExp::directcall to handle devirtualizing of virtual function call in front-end.
It can also handle a direct call with DotTypeExp by the same logic.

@yebblies
Copy link
Contributor

You could test this by corrupting the vtable of a class and checking the call succeeds anyway.

eg

void main()
{
    auto c = new Class();
    *cast(void**)c = null;
    c.func();
}

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 21, 2015

@yebblies Unfortunately the test does not work for non-release build, because a class method asserts invariant and it requires valid this instance reference.

class B
{
    void func()
    {
        // _d_invariant() call will cause Access Violaton
    }
}
final class C : B
{
}
void main()
{
    auto c = new C();
    *cast(void**)c = null;
    c.func();
}

@yebblies
Copy link
Contributor

Urrgh. Why are we calling invariants on classes that don't have invariants?

@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 21, 2015

Class B is not final, so its derived class may define invariants.

@yebblies
Copy link
Contributor

Ah, that makes sense. extern(C++) classes don't/can't call invariants, so this should work.

extern(C++)
class B
{
    void func()
    {
    }
}
extern(C++)
final class C : B
{
}
void main()
{
    auto c = new C();
    *cast(void**)c = null;
    c.func();
}

…nal class

Add `CallExp::directcall` to handle devirtualizing of virtual function call in front-end.
It can also handle a direct call with `DotTypeExp` by the same logic.
@9rnsr
Copy link
Contributor Author

9rnsr commented Feb 21, 2015

Good idea! Added test.

@yebblies
Copy link
Contributor

I'm not 100% sure this should be in the frontend, but it doesn't look like it will do any harm.

@yebblies
Copy link
Contributor

Auto-merge toggled on

braddr added a commit that referenced this pull request Feb 21, 2015
Issue 14211 - Compiler should devirtualize calls to members of final class
@braddr braddr merged commit fdafca9 into dlang:master Feb 21, 2015
@yebblies
Copy link
Contributor

win32 just went red on the autotester. Any idea how this could have caused it?

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.

3 participants