Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect linker command line on MacOS with LLD when using a shared object named ".so" #18464

Open
tkoeppe opened this issue May 21, 2023 · 14 comments
Assignees
Labels
help wanted Someone outside the Bazel team could own this P3 We're not considering working on this, but happy to review a PR. (No assignee) platform: apple team-Rules-CPP Issues for C++ rules type: bug

Comments

@tkoeppe
Copy link
Contributor

tkoeppe commented May 21, 2023

Description of the bug:

On MacOS, the default linker is LD64, which is tolerant to whether a dynamic library uses the native .dylib or the anatopic .so file extension from the ELF world: the flag -lfoo considers both libfoo.dylib and libfoo.so to match.

Bazel leans on this behaviour when a cc_binary has a precompiled source, srcs = [":libfoo.so"], and emits the linker flag -lfoo, both on Linux and on MacOS (together with an -L flag specifying the mangled solib directory into which the precompiled library is symlinked). However, on MacOS this is not always correct:

With LLVM's LLD linker, this setup is broken, as that linker strictly requires the .dylib file extension. With LLD, linking simply fails.

The reason this matters is that sometimes the shared library is part of our build rules, and so we have to pick a name for it; that way we can end up with something named .so on MacOS. That setup works with LD64, but not with LLD.

There is also a related issue where both LD64's and LLD's behaviour deviates from what Bazel expects, namely when the filename is not of the form lib<name>.so or lib<name>.dylib, e.g. x.dylib: in that case Bazel emits the flag -l:x.dylib, but the : syntax is not recognized by by either linker.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

With some trivial source files foo.cc, bar.cc, and demo.cc:

cc_binary(linkstatic=true, linkshared=true, name="libfoo.so", srcs=["foo.cc"])

cc_binary(linkstatic=true, linkshared=true, name="bar.dylib", srcs=["bar.cc"])

cc_binary(name="demo", srcs=["demo.cc", ":libfoo.so", ":bar.dylib"])

Linking with libfoo.so will fail with LLD:

  • ld64.lld: error: library not found for -lfoo

Linking with bar.dylib will fail with both linkers:

  • LD64: ld: library not found for -l:bar.dylib
  • LLD: ld64.lld: error: library not found for -l:bar.dylib

Which operating system are you running Bazel on?

MacOS 12 and 13

What is the output of bazel info release?

release 6.2.0 (via bazelisk)

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

This appears to be a straight-forward behavioural difference between LD64 and LLD. It seems like Bazel should be made aware of the effective linker.

The use of the -l:... syntax should probably be avoided entirely on MacOS.

@Pavank1992 Pavank1992 added the team-Rules-Java Issues for Java rules label May 22, 2023
@Pavank1992 Pavank1992 added team-Rules-CPP Issues for C++ rules and removed team-Rules-Java Issues for Java rules labels May 22, 2023
@keith
Copy link
Member

keith commented May 22, 2023

As far as macOS lld not supporting .so I submitted a patch for that here https://reviews.llvm.org/D151147 which I think is the easiest way to solve that piece. I agree with your assessment for -l: that we should just not do that on macOS. After a brief look I think the issue logic is here

} else if (CppFileTypes.SHARED_LIBRARY.matches(name)
|| CppFileTypes.VERSIONED_SHARED_LIBRARY.matches(name)) {
librariesToLink.addValue(LibraryToLinkValue.forVersionedDynamicLibrary(name));
} else {
// Interface shared objects have a non-standard extension
// that the linker won't be able to find. So use the
// filename directly rather than a -l option. Since the
// library has an SONAME attribute, this will work fine.
librariesToLink.addValue(
LibraryToLinkValue.forInterfaceLibrary(inputArtifact.getExecPathString()));
}
, if the library was instead passed through the else statement here, it would end up getting added directly to the link command line, which should work with both linkers.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented May 23, 2023

if the library was instead passed through the else statement here, it would end up getting added directly to the link command line, which should work with both linkers.

I think so, too, yes -- I don't know what the benefit of the -l: syntax is, even for ELF builds. Why not just always pass the library name as a direct input argument?

@keith
Copy link
Member

keith commented May 23, 2023

I think you're probably right, worth a try

@tkoeppe
Copy link
Contributor Author

tkoeppe commented May 23, 2023

@trybka: Any thoughts on why we use -l:?

@trybka
Copy link
Contributor

trybka commented May 23, 2023

-l: is a compatibility workaround for how ELF linkers work.

-l: allows you to pass a filename directly, (e.g. in the case that your lib name does not start with lib and end with a supported extension (e.g. .so or .dylib).

So you could ask, "why not pass the filename directly?"
On Windows (COFF) and macOS (MachO), this works fine. What actually gets put into the NEEDED section is the basename of that path, e.g. x.dylib.

When building for ELF, the whole filename is there, which makes runtime path searching usually not work.

So this code in Bazel should probably ask the toolchain (via a feature), "do you need legacy ELF -l:namespec behavior?"
If not (on Windows, mac), just emit the full path. On ELF, use this janky -l: business.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented May 23, 2023

Ah yes, the NEEDED name in ELF depends on how you link... but could that be fixed by specifying a SONAME explicitly?

@trybka
Copy link
Contributor

trybka commented May 23, 2023

I mean, sure, but Bazel has no way to know if you did this or not.

In this stage, it is dealing with libraries that it may not have built (you can just have a checked in .so that you reference with cc_library.srcs). And it cannot inspect the file as part of the analysis phase (or, at all).

@tkoeppe
Copy link
Contributor Author

tkoeppe commented May 23, 2023

Yes, makes sense, thanks!

keith added a commit to llvm/llvm-project that referenced this issue May 23, 2023
While not the recommended extension on macOS .so is supported by ld64.
This mirrors that behavior.

Related report: bazelbuild/bazel#18464

Differential Revision: https://reviews.llvm.org/D151147
@oquenchil oquenchil self-assigned this Jul 3, 2023
@oquenchil oquenchil added P3 We're not considering working on this, but happy to review a PR. (No assignee) help wanted Someone outside the Bazel team could own this and removed untriaged labels Jul 3, 2023
@oquenchil
Copy link
Contributor

Can this be closed?

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 3, 2023

@oquenchil The issue hasn't been fixed yet, has it?

@oquenchil
Copy link
Contributor

I wasn't sure if the comments from Keith and Tom were enough here. I will leave it open then.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 3, 2023

There were several issues under discussion, but the main problem has not been addressed as far as I'm aware.

@MaskRay may have a change to LLD in the pipeline that would make this issue moot, though, without changing Bazel, as long as people are able to upgrade to a sufficiently modern linker.

@keith
Copy link
Member

keith commented Jul 5, 2023

Yea the issue around ld: library not found for -l:bar.dylib is yet to be fixed. But as far as supporting .so files in ld64.lld my change for that landed for the future https://reviews.llvm.org/D151147

@keith
Copy link
Member

keith commented Jan 12, 2024

-l: is fixed on main and bazel 6.5.0 to just not be used instead on macOS

veselypeta pushed a commit to veselypeta/cherillvm that referenced this issue Aug 23, 2024
While not the recommended extension on macOS .so is supported by ld64.
This mirrors that behavior.

Related report: bazelbuild/bazel#18464

Differential Revision: https://reviews.llvm.org/D151147
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Someone outside the Bazel team could own this P3 We're not considering working on this, but happy to review a PR. (No assignee) platform: apple team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

No branches or pull requests

6 participants