[lld][MachO] Support for -interposable #131813
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-macho Author: John Holdsworth (johnno1962) ChangesAs discussed in #53680, add support for ld64's -interposable flag on Apple platforms to lld. Full diff: https://github.com/llvm/llvm-project/pull/131813.diff 4 Files Affected:
diff --git a/lld/MachO/Config.h b/lld/MachO/Config.h
index f8dcc84e4ee1b..629fa2bfd0cb3 100644
--- a/lld/MachO/Config.h
+++ b/lld/MachO/Config.h
@@ -183,6 +183,7 @@ struct Configuration {
bool deadStripDylibs = false;
bool demangle = false;
bool deadStrip = false;
+ bool interposable = false;
bool errorForArchMismatch = false;
bool ignoreAutoLink = false;
// ld64 allows invalid auto link options as long as the link succeeds. LLD
diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 4f6c9b4ddc798..a335b5750b3de 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -1676,6 +1676,7 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
// Must be set before any InputSections and Symbols are created.
config->deadStrip = args.hasArg(OPT_dead_strip);
+ config->interposable = args.hasArg(OPT_interposable);
config->systemLibraryRoots = getSystemLibraryRoots(args);
if (const char *path = getReproduceOption(args)) {
diff --git a/lld/MachO/Options.td b/lld/MachO/Options.td
index 9001e85582c12..abc7697fde873 100644
--- a/lld/MachO/Options.td
+++ b/lld/MachO/Options.td
@@ -875,7 +875,6 @@ def setuid_safe : Flag<["-"], "setuid_safe">,
Group<grp_rare>;
def interposable : Flag<["-"], "interposable">,
HelpText<"Indirects access to all to exported symbols in a dylib">,
- Flags<[HelpHidden]>,
Group<grp_rare>;
def multi_module : Flag<["-"], "multi_module">,
Alias<interposable>,
diff --git a/lld/MachO/SymbolTable.cpp b/lld/MachO/SymbolTable.cpp
index a61e60a944fb4..f6ac2019533ca 100644
--- a/lld/MachO/SymbolTable.cpp
+++ b/lld/MachO/SymbolTable.cpp
@@ -204,8 +204,9 @@ Defined *SymbolTable::addDefined(StringRef name, InputFile *file,
// With -flat_namespace, all extern symbols in dylibs are interposable.
// FIXME: Add support for `-interposable` (PR53680).
- bool interposable = config->namespaceKind == NamespaceKind::flat &&
- config->outputType != MachO::MH_EXECUTE &&
+ bool interposable = ((config->namespaceKind == NamespaceKind::flat &&
+ config->outputType != MachO::MH_EXECUTE) ||
+ config->interposable) &&
!isPrivateExtern;
Defined *defined = replaceSymbol<Defined>(
s, name, file, isec, value, size, isWeakDef, /*isExternal=*/true,
|
|
|
|
Looks like I've wrangled the git/CI problems now. Anything else you'd like let me know. Rest assured I've been testing this version of the linker in the field on a reasonably sized project and it and the new -interposable functionality are working well. |
|
HI Folks, how about landing this PR before it collects any conflicts? It's a very limited change to unlock a latent capability of your code and as such possess very limited risk of causing a collateral regression. Interposing is an very useful thing for a linker to be able to do which I use in the project InjectionIII which is motivated here. I'm interested in ldd as it has started turning up in Apple's daily build of their developer toolchain so it looks like they are evaluating it. It would be a shame if it were to not include the -interposable option given how easily it could. |
|
Could you verify that Apple's linker has the same behavior around:
If so, this looks good to me. |
|
Hi @smeenai, I can confirm your first question by observation of the linker in use in a large-ish project that requires interposing for dynamic updates. By chance Apple's Xcode allows you to choose an executable or dynamic library for your "main" image containing all code and both cases definitely work with "my" ldd and Apple's linker so indirecting symbols in an executable is more faithful. This is the reason the code I'm proposing is different from your original suggestion last week. Your second question about |
|
I'm sensing a reluctance to take my word for Apple's linker applying -interposable for symbols inside executables. To close this out how about I go with @smeenai's original suggestion in the mentioned issue as below, update the test and not implement this. With Xcode 16 an app's "main image" is a .dylib now anyway so it's not important to me to fight this battle. |
|
Sorry, not doubting your word at all, just got busy yesterday :) Give me a bit to look at this a bit more in depth. |
|
If you're checking for lazy symbol bindings, targeting newer OS versions uses chained fixups by default, which gets rid of lazy bindings. The difference between interposable and non-interposable calls is whether they go through a stub, i.e. The stub is what enables the interposing to work, and the binding for the stub can be lazy or non-lazy. You can repeat the same experiment with an executable and confirm that |
|
Thanks for the info. Modifying the test to use Apple's linker so: Yields: So it is setting up a stub for a symbol in an executable. Does that answer your first question? Without the -interposable flag it looks like this: |
Ah sorry, by this I meant that you were correct in what you'd said, not that I was asking you to run that experiment to verify it :) Either way though, the behavior change here looks good; we should just update the test to verify the stub instead of the lazy bind. |
|
Modifying the linking to force the platform version: Yields: But how to force the platform version for lld directly in an llvm specific test? I'm thinking the existing test proves the option is working. |
|
You can add |
|
How about this test (before I commit it): objdump -d output is: |
|
That's using a numeric substitution block to capture the hex value, and it's also taking advantage of FileCheck doing partial matches on lines by default (so you don't need to specify the entire contents of a line to match against, just the bits you care about). I'd also suggest renaming the functions to |
|
Thanks for all your patience @smeenai, I think the test is updated and the PR squashed now. Anything else you can think of? |
|
I'll land this for you when all tests have passed. |
|
@johnno1962 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
Thanks @smeenai, Two weeks turnaround for my first llvm PR - Not bad all told. Perhaps one day I'll pursue my theory that all object files should be stored in zip format for faster disk I/O but that'll keep for now. |
As discussed in #53680, add support for ld64's -interposable flag on Apple platforms to lld.