MacOS: add linker flag "-undefined dynamic_lookup" for shared libs. #66204
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
jeff-davis
force-pushed
the
master
branch
2 times, most recently
from
fa490e1 to
9ac2d43
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
After asking on Discord, someone suggested that @alexcrichton was the right person to look at this kind of fix. |
|
Changing the default set of linker flags for a target is unfortunately no small task. Linkers are very finnicky and are used all the time so we don't tend to know what we're relying on when calling linkers. I say this in the sense that this has an unusually high risk of causing accidentally regression due to tweaking how the linker is being invoked. I do not know of possible regressions, but they almost always happen when changing linker arguments. I also don't really know what we should be doing here about undefined symbols. This runs the risk of papering over real issues in addition to fixing your original issue. For example maybe a symbol is accidentally undefined? At this time I don't think we have a real great way of evaluating this change to be confident in it one way or another. This sort of requires someone with deeper knowledge of linking on macOS to know whether we should be passing this by default and what our possible risks of regressions are. Unfortunately I'm not that person. |
Neither am I, unfortunately. Any idea who I might reach out to? How are linker changes normally made and what kind of fallout have we seen in the past? Are we worried about compilation failures, or some subtle performance regression due to a slightly different linking process? Does the CI test suite have reasonable platform coverage to boost confidence, and if so, how do I kick off a run that has my patch in it?
I understand why you want some more expertise involved in fixing this; but it's pretty clear to me that this is a bug and should be fixed. Take a look at libssl: Notice that libssl references, but does not define, The point here is that it's fundamental and expected that a shared library might reference symbols in other libraries or the host program. Apparently, that is not common in the rust world, but I don't see a reason it shouldn't be supported in a low-level language like rust for a crate type called If we want to catch mistakes, relying on platform-specific linking details is the wrong way to do it anyway. It would be much more reliable to simply try loading the library, which would catch unresolved symbols on any platform. If there is a use case beyond that, we could expose a feature on all platforms that will explicitly check for unresolved symbols. |
|
@aleksijuvani are you the right person to take a look here? |
|
I'm not familiar with this linker flag, sorry! |
|
@froydnj any thoughts? |
|
Unfortunately I do not know of someone myself with sufficient experience to be able to review this. We historically just basically haven't ever changed the linker flags for tier 1 targets (unless it only applies in niche situations). Also unfortunately our test suite would not provide much coverage here in terms of regression testing. I am personal wary of this change because of how big this hammer is. It basically says "stop ever reporting undefined symbol errors on macOS". Some questions I would have would be:
I personally feel that just knowing more about this change will help boost confidence in it. At this time it, to me at least, still feels like too much of an unknown "whack this thing with that hammer" fix for me to personally have confidence in it. |
On Linux, the default for undefined symbols in executables is to error; the default for undefined symbols in shared libraries is to allow them. I believe, but am not certain, that the default on Windows is to reject undefined symbols always.
Yes, this is expected: the runtime loader on OS X will load What is the situation where
The linker can already determine that all the symbols in your shared library are defined in the library itself, or defined in other shared libraries that your shared library links to/depends on. This behavior is just as reliable as loading the library itself. Given that |
PostgreSQL allows the loading of extensions, which are some combination of SQL and a shared library. For non-trivial extensions, it's common for the shared library to call internal postgres APIs, which are defined in the PostgreSQL executable. I am developing https://github.com/jeff-davis/postgres-extension.rs to make it possible to develop extensions in rust, but this is one of the roadblocks I'm hitting.
My crate is just meant to make it possible for others to develop extensions in rust, using my crate as a dependency. So I think any build hack needs to be done by all users of my crate rather than just in my crate. Perhaps there's a way around that or maybe I should try to document that everyone needs to do that. But at least right now, it seems like that build hack would spill over to users of my crate. |
Thank you for the additional context! I can see how undefined symbols are definitely the expected behavior here. I suppose that building Postgres extensions on Mac in C/C++ also requires
This is absolutely a reasonable concern! But I think if you have a fn main() {
// Permit undefined symbols to Postgres internal symbols in the `postgres` binary.
// https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script
println!("cargo:rustc-cdylib-link-arg=-undefined");
println!("cargo:rustc-cdylib-link-arg=dynamic_lookup");
}I think that any crate that depends on your crate magically inherits the above flags when a shared library is created. |
Third party extensions, like PostGIS, use Extensions that come with postgres, like hstore, use
I tried that, but it seems like it needs to be in the shared library's crate. If I move it up to the postgres-extension.rs crate, it doesn't seem to be working. |
Hmmm, interesting! I would have expected it to, but IIUC, this bit in cargo only triggers for the current crate being compiled: Rather than changing the defaults for every Rust program, maybe it's worth exploring whether cargo could be changed to propagate linker args upwards in the dependency graph by moving that block outside of the Alternatively, I guess |
|
Thanks for the comments here @froydnj! I think I agree that exploring a Cargo/build system solution here is probably best rather than changing the default behavior in the compiler. |
|
It looks like |
Necessary for shared libraries where not all symbols are resolved until the library is loaded at runtime. Fixes rust-lang#62874
|
I created rust-lang/cargo#7612 to represent the cargo change @froydnj suggested. I think that's independently useful as a way to enable linker hacks when necessary, so I support that change. I'm not 100% sure we should give up on this rustc change, though. I've updated this PR here so that the linker flag only applies when creating a |
|
The other PR ( rust-lang/cargo#7612 ) seems unsatisfying and is not necessarily making progress. We have a clear use case, problem, and fix on the table with this current PR. My expectation for a supported platform is that we should be able to fix platform bugs, or at least do some investigation to learn why we can't fix it or why the proposed fix is wrong. I haven't received any feedback on what's wrong with this PR other than a general discomfort with making any changes to the linking process at all. If we see some evidence that this fix is a minefield, then I guess we need to resort to hacks and workarounds. But everything I've seen so far suggests that this flag is a routine solution to this problem. I have made some attempts to boost our confidence by looking at other projects and build tools, and I have also changed my PR to narrow the scope and hopefully put the flag in a more suitable place. More looking around shows that others have run into this problem as well, such as:
...and solved it by hacking the build to use the same Sure, I can hack around the problem in similar ways, but I don't see why any of it is necessary. It's just a way for a developer to make a mistake, forget to test on MacOS, and then their crate breaks. |
|
I'm sorry this isn't a great situation, and I agree it's definitely not great! I will stand by what I said above though, in that I do not personally have the confidence to r+ this change as is, nor do I know who can. That's basically just the fact-of-the-matter right now, and while it'd be great if that could change I don't have the time and energy right now to build up all the necessary context to sign off on a change like this. Doing a conservative change is going to be difficult in Cargo, as I've listed on that PR. If you want to land this change as-is you'll likely want to get in touch with the compiler team on Zulip and see if someone there can help take over review. Someone else may have the time to help confirm and/or investigate more here to see if this is a reasonable change to make. |
|
r? @parthsane |
|
Sorry, I am not familiar with linking in mac. I don't think I can help. |
|
r? @Centril |
|
r? @michaelwoerister cc @Zoxc |
|
Sorry, my knowledge about dynamic linking on macOS is rather limited. |
Centril
added
I-nominated
T-compiler
|
Nominating for T-Compiler to find a suitable reviewer. |
|
This seems to me like a niche platform specific use case which should not be the default for rustc. If you want this behavior you should have to opt in for it. |
|
The use case is certainly not platform-specific. I only found it when I later tried to build my crate on Mac. I think a better description is that the problem is platform-specific. I posted links to other projects that also need to do this hack (e.g. creating a loadable module for python in rust), so I'm not completely alone. I don't understand your comment that it "should not be the default for rustc". Right now it does exactly what I want on linux: it allows undefined symbols in shared libraries. Are you suggesting that we should change linking on linux so that it also rejects undefined symbols unless you opt-in? I would understand the point better if rustc generally didn't take much ownership of the linking process (like a C compiler). But it already does -- my PR just adds a new flag next to another MacOS-specific linker flag. |
|
Hmm. So we discussed this in the compiler meeting. Speaking personally, I would say a few things: This is a tricky issue! I appreciate @jeff-davis's frustration but I also feel like there is good reason to be cautious. Adding this flag to the default for Mac OS is not something we can undo, after all, and for the most part I believe our linker flags are fairly "conservative" (i.e., they stick to the defaults from the linker itself). One thing that I think would be useful is trying to collect some of the information in this thread. I found I had to read it 2 or 3 times to extract out the various points. I think the summary looks something like:
Did I miss anything? Looking at the above summary, a couple of questions arise:
I think what we're missing is more feedback from people deeply familiar with the Mac platform, who might be able to say if this is a pretty standard thing to do or what. I guess it feels to me like changes of this kind, even if they are only one line of code, might be more appropriately handled with an RFC, since there are potentially ramifications that are hard for us to fully assess. |
|
Thank you @nikomatsakis for an excellent summary. I will attempt an RFC for "Defining the behavior of unresolved symbols in cdylib crates". |
|
triage: @nikomatsakis 's comment summarized the T-compiler discussion nicely. I'm going to remove nomination (since we have discussed this) and close the PR (since we think this, at the very least, requires an RFC.) |
Necessary for shared libraries where not all symbols are resolved
until the library is loaded at runtime.
Fixes #62874