[swift-dev] [Swift 2.2] Request to merge pull request: Perform a dynamic method call if a class has objc ancestry

[Joe Groff]

> On Feb 2, 2016, at 10:06 AM, Arnold Schwaighofer <aschwaighofer at apple.com> wrote:
> 
> Yes we could do something like this as an optimization I imagine. That should be done separately from this correctness bug, though.

Agreed. The change LGTM as is.

-Joe

> 
> 
>> On Feb 2, 2016, at 10:02 AM, Joe Groff <jgroff at apple.com> wrote:
>> 
>> Could we use the method implementation pointer as the speculation key instead of the isa pointer? That makes the load chain a little longer, but should give you a guaranteed exhaustive key for Swift methods even with mixed ObjC heritage, since artificial ObjC subclasses still aren't allowed to override Swift methods that aren't explicitly `dynamic`.
>> 
>> -Joe
>> 
>>> On Feb 2, 2016, at 9:54 AM, Arnold Schwaighofer via swift-dev <swift-dev at swift.org> wrote:
>>> 
>>> https://github.com/apple/swift/pull/1159
>>> 
>>> Perform a dynamic method call if a class has objc ancestry in specula
>>> tive devirt as fallback.
>>> 
>>> If a class has an @objc ancestry this class can be dynamically overridden and
>>> therefore we don't know the default case even if we see the full class
>>> hierarchy.
>>> 
>>> rdar://23228386
>>> 
>>> Explanation:
>>> 
>>> Before this change we would devirtualize a method call to static calls of the potential call targets without a fallback to a class method lookup if we believed to have the full class hierarchy e.g in WMO mode. But during runtime this assumption can be violated because an objective-c class can be dynamically extended and so we would end up calling through the wrong method.
>>> 
>>> private class A : NSObject {
>>> func foo() {...}
>>> }
>>> private class B : A { 
>>> override foo() {...}
>>> }
>>> 
>>> Before:
>>> 
>>> callAnA(a : A) {
>>> if (a isa A) {
>>> A.foo(a)
>>> } else {
>>> B.foo(a)
>>> }
>>> }
>>> 
>>> After:
>>> 
>>> callAnA(a : A) {
>>> if (a isa A) {
>>> A.foo(a)
>>> } else if (a isa B) {
>>> B.foo(a)
>>> } else a.foo(a) // call through class method table.
>>> }
>>> 
>>> Scope:
>>> 
>>> The change only effects whether we emit a default case that calls through the class method table. Emitting the call through the class method table is always safe. This risk is low.
>>> 
>>> Testing:
>>> 
>>> There is a unit test testing the change, furthermore the change was tested in the project reported in rdar://23228386 and only with this change the test scenario in the project works.
>>> 
>>> Reviewed by:
>>> Roman, the author of the speculative virtualization pass, and Slava also took a look at it.
>>> _______________________________________________
>>> swift-dev mailing list
>>> swift-dev at swift.org
>>> https://lists.swift.org/mailman/listinfo/swift-dev
>> 
>