[Runtime][Foundation] Supplement the class_getImageName patch with a patch to +[NSBundle bundleForClass:] on older "embedded" targets #19389
Conversation
…patch to +[NSBundle bundleForClass:] on older "embedded" targets. The patch technique of editing the symbol table doesn't work for +bundleForClass:'s call to class_getImageName when +bundleForClass: is in an embedded shared cache. rdar://problem/44489216
|
@swift-ci please test |
| /// Selectors for the target method, patch method, and helper method. | ||
| #define BUNDLE_FOR_CLASS_SEL @selector(bundleForClass:) | ||
| #define PATCHED_BUNDLE_FOR_CLASS_SEL @selector(_swift_bundleForClass:) | ||
| #define BUNDLE_WITH_EXECUTABLE_PATH_SEL @selector(_swift_bundleWithExecutablePath:) |
There was a problem hiding this comment.
[GitHub ate this comment the first time I submitted it; trying again.]
It seems weird to me to define a normal method for _swift_bundleWithExecutablePath: but patch _swift_bundleForClass: into place with class_addMethod. Why not either
- always provide
+_swift_bundleForClass:, and collapse+_swift_bundleWithExecutablePath:into it (but only swap them conditionally) - collapse
+_swift_bundleWithExecutablePath:intopatchedBundleForClass, since it doesn't actually useself
?
You can also avoid some of the method_exchangeImplementations dance with method_setImplementation, saving the old IMP in a static property instead of another method.
There was a problem hiding this comment.
I organized it this way to keep the dependencies clean(er). The runtime side shouldn't link anything in from Foundation, but _swift_bundleForClass: wants to use the getImageNameFromSwiftClass in the runtime. I could do everything in patchedBundleForClass but it would be inconvenient to look up all of that stuff at runtime.
I used method_exchangeImplementations for thread safety. method_setImplementation would have a race condition if another thread was calling +bundleForClass: when the patch was being installed, since there's a window where the patch is active but the old IMP hasn't yet been stored into the static. Could work around that by fetching it first but then there's potential (albeit unlikely) races with other swizzlers.
There was a problem hiding this comment.
Ooh, good catch. Okay, thanks for the explanation.
There was a problem hiding this comment.
I was this close to committing a version with method_setImplementation when I had a funny thought in the back of my mind about thread safety. I've always done swizzling like that in the past, but I've always done it at startup rather than lazily like this one.
| @@ -251,6 +324,9 @@ static void patchGetImageNameInImage(const struct mach_header *mh, | |||
| const void *newImplementationAddr = | |||
| reinterpret_cast<const void *>(&patchedGetImageNameFromClassForOldOSs); | |||
| patchLazyPointers(mh, "_class_getImageName", newImplementationAddr); | |||
| #if PATCH_NSBUNDLE | |||
| patchNSBundle(); | |||
There was a problem hiding this comment.
Oh, bah. I realized that since libswiftCore links against Foundation anyway it'd be okay to do this eagerly once rather than running it in the "new image loaded" callback.
There was a problem hiding this comment.
I guess we're trying not to rely on linking to Foundation here, so okay.
The patch technique of editing the symbol table doesn't work for
+bundleForClass:'s call to class_getImageName when+bundleForClass:is in an embedded shared cache.rdar://problem/44489216