[objc][codegen] Fixed bug with calling fake override interface methods #4588
Conversation
runtime/src/main/cpp/ObjCExport.mm
Outdated
| @@ -943,8 +945,21 @@ static void throwIfCantBeOverridden(Class clazz, const KotlinToObjCMethodAdapter | |||
| insertOrReplace(methodTable, adapter->nameSignature, const_cast<void*>(adapter->kotlinImpl)); | |||
| RuntimeAssert(adapter->vtableIndex == -1, ""); | |||
|
|
|||
| if (adapter->itableIndex != -1 && superITable != nullptr) | |||
| if (adapter->itableIndex != -1 && superITable != nullptr) { | |||
| // Because of fake overrides [adapter->interfaceId] might not be equal to [typeInfo->classId_]. | |||
There was a problem hiding this comment.
This is not entirely correct. Yes, the particular bug we've encountered features a fake override. But
- It is probably triggered by a bug in ObjCExportCodeGenerator: the latter doesn't "resolve fake overrides" when computing
functionName, so the machinery doesn't find the adapter among the inherited ones and generates an adapter for super interface's method. - Generally that machinery doesn't guarantee that it would only emit adapters matching the type. Avoiding emitting adapters that are inherited from super types is only an optimization. We are free to remove or rework it. And even if we don't, there might be cases when it doesn't guarantee that adapters match type.
There was a problem hiding this comment.
a bug in ObjCExportCodeGenerator: the latter doesn't "resolve fake overrides" when computing functionName, so the machinery doesn't find the adapter among the inherited ones and generates an adapter for super interface's method.
JFTR: I've rechecked the ObjCExportCodeGenerator and am not sure anymore that it has such a bug.
Apparently our virtual call machinery calls interface fake override methods using .functionName of a fake override, not of its "real" super method. This means that ObjCExportCodeGenerator generally has to generate additional "reverse adapter" for the fake override, using its own functionName. While itablePlace is reused from super interface.
It can use itablePlace = null though, but this would be just an optimization (see also above).
runtime/src/main/cpp/ObjCExport.mm
Outdated
| } else { | ||
| auto const& interfaceVTable = interfaceVTablesIt->second; | ||
| RuntimeAssert(interfaceVTable.size() == static_cast<size_t>(interfaceVTableSize), ""); | ||
| } |
There was a problem hiding this comment.
Looks pretty much like a copy-paste from the code above. It should be possible to extract a (lambda) function for this code.
| if (adapter->itableIndex != -1 && superITable != nullptr) | ||
| if (adapter->itableIndex != -1 && superITable != nullptr) { | ||
| // In general, [adapter->interfaceId] might not be equal to [typeInfo->classId_]. | ||
| auto interfaceId = adapter->interfaceId; |
There was a problem hiding this comment.
Can also be used in addToITable below.
homuroll
force-pushed
the
objc_fake_override_interface_call_fix
branch
from
62cccc5 to
03ad015
Compare
No description provided.