[irgen] Force emission of objc class refs for non-foreign clang objc classes whose metadata we use. #28174
Conversation
|
@rjmccall what do you think of the query here? Would you change anything? |
|
@swift-ci test |
|
I am probably going to add to the FileCheck pattern in the IRGen test that the class ref is marked no dead strip (which it is today). |
|
but I am going to do that in a follow on commit. |
|
*I mean I am going to do it after the full test passes so I can just smoke test. |
|
It's quite unfortunate that we have to emit a bunch of symbol references which are likely to have real load-time costs just to get something linked in. If this is just a short-term solution, okay, but can we figure out a longer-term solution that would force the library to be linked? Actually, that has immediate implications as well, since we can't always a class ref for an imported class (e.g. if it's a runtime-only class). |
lib/IRGen/GenDecl.cpp
Outdated
| @@ -1252,13 +1252,34 @@ void IRGenerator::noteUseOfTypeGlobals(NominalTypeDecl *type, | |||
| return; | |||
|
|
|||
| // Force emission of ObjC protocol descriptors used by type refs. | |||
| if (auto proto = dyn_cast<ProtocolDecl>(type)) { | |||
| if (auto *proto = dyn_cast<ProtocolDecl>(type)) { | |||
lib/IRGen/GenDecl.cpp
Outdated
| @@ -1252,13 +1252,34 @@ void IRGenerator::noteUseOfTypeGlobals(NominalTypeDecl *type, | |||
| return; | |||
|
|
|||
| // Force emission of ObjC protocol descriptors used by type refs. | |||
| if (auto proto = dyn_cast<ProtocolDecl>(type)) { | |||
| if (auto *proto = dyn_cast<ProtocolDecl>(type)) { | |||
There was a problem hiding this comment.
Just a small thing I noticed. If it really bugs you I can remove it.
There was a problem hiding this comment.
It just doesn't seem like an improvement in any way. Also, I have several times had to go through and fix a bunch of places where somebody needlessly recorded that something was a pointer only for a refactor to start encapsulating it in a class.
|
@rjmccall For a longer term solution, we would have to change the guarantees that the static linker provides around autolinking. We would need autolinking to not be a suggestion, but a hard requirement. |
|
Build failed |
|
Just for anyone listening in, @rjmccall and I talked about this offline and we agreed to wait a second to talk with @DougGregor when he is back in the office tomorrow. |
|
@rjmccall, @DougGregor, and I talked about this offline and we agree that in the short term this is the appropriate step to take. |
…classes whose metadata we use. Today in far more cases we are using mangled strings to look up metadata at runtime. If we do this for an objc class but for whatever reason we do not have any other references to the class, the static linker will fail to link in the relevant framework. The reason why this happens is that autolinking is treated by the static linker as a hint that a framework may be needed rather than as a "one must link against the framework". If there aren't any undefined symbols needed by the app from that framework, the linker just will ignore the hint. Of course this then causes the class lookup to fail at runtime when we use our mangled name to try to lookup the class. I included an Interpreter test as well as IRGen tests to make sure that we do not regress here in the future. NOTE: The test modifications here are due to my moving the ObjCClasses framework out of ./test/Interpreters/Inputs => test/Inputs since I am using it in the IRGen test along side the interpreter test. rdar://56136123
gottesmm
force-pushed
the
pr-7bbd01cc86ac04db4e31ad2128d842d7205d3314
branch
from
949de9e to
8247525
Compare
|
@swift-ci test |
Today in far more cases we are using mangled strings to look up metadata at
runtime. If we do this for an objc class but for whatever reason we do not have
any other references to the class, the static linker will fail to link in the
relevant framework. The reason why this happens is that autolinking is treated
by the static linker as a hint that a framework may be needed rather than as a
"one must link against the framework". If there aren't any undefined symbols
needed by the app from that framework, the linker just will ignore the hint. Of
course this then causes the class lookup to fail at runtime when we use our
mangled name to try to lookup the class.
I included an Interpreter test as well as IRGen tests to make sure that we do
not regress here in the future.
NOTE: The test modifications here are due to my moving the ObjCClasses framework
out of ./test/Interpreters/Inputs => test/Inputs since I am using it in the
IRGen test along side the interpreter test.
rdar://56136123