Make -Clinker-plugin-lto compatible with ld64

[cormacrelf]

Fixes (partially) #60059. Works with ld64 and ld64.lld. There are other linkers that also need some attention here.

There are also a few warnings we could emit that could make -Clinker-plugin-lto easier to use correctly, in particular for Apple toolchains:

• warning: ignoring LTO plugin path in -Clinker-plugin-lto=<path to libLTO>, because using LLD, which will use its built-in LLVM
• warning: using -Clinker-plugin-lto with Apple ld64, but no path to libLTO.dylib provided. LLVM versions likely incompatible
  • .help: Use -Clinker-plugin-lto=path/to/libLTO.dylib from an appropriate LLVM toolchain. See [version compatibility table]

This PR keeps it to what's necessary to make linker plugin LTO work, but I'm happy to deliver some warnings, seeing as I've just messed around with this particular code.

[rustbot]

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

[petrochenkov]

r? compiler
I'm overloaded with reviews.

[petrochenkov]

r? compiler
b-naber doesn't do reviews.

[cjgillot]

r? compiler

[davidtwco]

I'm not especially familiar with LTO, but these changes seem reasonable to me from what I can surmise from the description and linked issue. I presume a test isn't possible for this?

cc @michaelwoerister @lqd who know more about linkers

[michaelwoerister]

Thanks for the PR, @cormacrelf! Taking a closer look at #60059 (comment) has been on my todo list for a while. I'll try to make some progress here this week. I don't have access to a macOS system these days, but we can try to make sure that CI tests cover this sufficiently.

[michaelwoerister]

(I've assigned myself, but don't let this discourage you from also taking a look, @lqd 🙂)

[michaelwoerister]

First of all, thanks a lot for looking into this, @cormacrelf! The kind of research done in #60059 (comment) is really helpful.

The changes look good to me -- but I have no way of testing this locally. I think we should try to add regression tests for this. We have a test that invokes Clang + LLD, but none that uses ld or ld64. A test similar to this one but with -Clinker-plugin-lto instead of -Clto should do the trick. However, I'm wondering how we can mitigate the fact, that ld64's libLTO.dylib is probably too old for rustc's LLVM version...

[michaelwoerister]

Is this documented somewhere? If so, it would be good to add a link.

I'm not sure if we should emit a warning. It could be super helpful -- but it could also be annoying if you know it's fine but for some reason cannot get rid of the commandline argument.

[cormacrelf]

No worries! I have started doing that on long-standing issues with lots of people butting their head against the problem. It has been good.

I agree about regression tests. These flags are going to be load-bearing for some big codebases.

As for libLTO, it looks like needs-matching-clang can ensure we have a clang, and we can inject LLVM_LIBLTO to the makefile environment the same way we inject CLANG and LLVM_BIN_DIR. Something something in the compiletest codebase. I'll have a look.

[michaelwoerister]

As for libLTO, it looks like needs-matching-clang can ensure we have a clang, and we can inject LLVM_LIBLTO to the makefile environment the same way we inject CLANG and LLVM_BIN_DIR. Something something in the compiletest codebase. I'll have a look.

Excellent idea! So, do we basically expect that -Clinker-plugin-lto will never work unless providing a rustc-compatible libLTO.dylib? I'm wondering if we could ship it with rustc. It looks rather big though (~100MB for libLTO.dylib in LLVM 17)

[bjorn3]

It will work if you use an LLVM/clang version at least as new as rustc for linking. https://doc.rust-lang.org/rustc/linker-plugin-lto.html#toolchain-compatibility lists the compatibility table, but it seems like it hasn't been updated for a while.

[michaelwoerister]

It will work if you use an LLVM/clang version at least as new as rustc for linking.

Yes, this observation makes me think that that's rarely the case 🙂:

By the way the LLVM shipped with XCode is almost always too old to load rustc produced bitcode files, so if it seems to work without using the linker plugin bundled with rustc, you aren't actually doing linker plugin LTO.

[cormacrelf]

That's a consideration for the docs and the real world users, but what version of clang is used for compiletest? If we know it to be always recent, then using its libLTO in the integration test is on the cards.

[bjorn3]

but what version of clang is used for compiletest?

If RUSTBUILD_FORCE_CLANG_BASED_TESTS is set (which we do in CI), we build clang too when building LLVM. And I believe we only enable the linker plugin lto tests when a clang version matching the LLVM used by rustc is available. We don't ship this clang we build with RUSTBUILD_FORCE_CLANG_BASED_TESTS though. And it is only set for the x86_64-gnu-debug CI job.

[michaelwoerister]

@rust-lang/infra, how realistic is it that we build clang (or at least libLTO.dylib) on macOS, so that we can have integration tests that make sure rustc-driven linker-plugin-LTO works on that platform? I think the clang build would be cached in the docker image alongside the rest of LLVM, right?

[Kobzol]

macOS jobs don't run inside (our) Docker images, so we'd have to figure out some caching on Github Actions directly (AFAIK). I have created a Zulip topic for this.

[michaelwoerister]

According to the Zulip discussion, our macOS builders will download their own version of clang and it seems reasonable that we can match that to the LLVM version of the rustc being built. @cormacrelf, do you know if that would help? If we can verify that that downloaded clang will invoke ld64 (instead of e.g. LLD), we should be in a position to regression test that rustc passes the correct arguments to the linker, right?

[michaelwoerister]

If we can verify that that downloaded clang will invoke ld64

I guess that should be part of the test: make rustc print the linker invocation and check that it's ld64

[cormacrelf]

We can write some great tests if we get a recent clang and libLTO, that would be fantastic. Clang 16 and 17 should both work, I think, but 17 to be on the safe side. If there are complications upgrading clang, we could at least test the flags work against rust-lld's Darwin mode without it, but we wouldn't be able to do a smoke test for Rust + C cross language inlining, and actual ld64 support would be untested.

make rustc print the linker invocation and check that it's ld64

Rustc will invoke clang++. And while the linker itself is known as "ld64" to distinguish from the GNU linker, it is invoked by clang as ld, so this technique doesn't go very far in general.

I suppose you want to guard against clang switching its default linker to LLD? I doubt that it will, but we can also just tell clang++ to use an absolute path to a specific linker and pin that down. I forget the incantation for that, it's --ld-path or something.

And may as well also run it with ld64.lld from the same clang build. Heck, you could even run it with a script that invokes rust-lld -flavor darwin "$@". Both of those will work even without a libLTO.

[michaelwoerister]

We'll need to find out how we can get access to that version of clang within run-make tests. This looks promising:

rust/src/tools/compiletest/src/lib.rs

Line 232 in 767453e

run_clang_based_tests_with: matches.opt_str("run-clang-based-tests-with"),

[michaelwoerister]

I think the way to go here is to make the clang version used for these kinds of tests configurable via the compiler's config.toml (i.e. add an option to bootstrap). The option would then look something like:

enum ClangUsedForCrossLangTesting {
    /// Don't run the tests (or fail if RUSTBUILD_FORCE_CLANG_BASED_TESTS is set)
    None,
    /// An externally provided clang version. (we could try to do some sanity checking of versions)
    External(Path),
    /// Build clang from the LLVM sources used by the compiler
    BuildMatching, 
}

Then the various builders could define what they want to do. If we allow an externally provided clang version, we should be able to run the tests on more builders.

@cormacrelf, are you up for looking into this?

[Congyuwang]

According to LLVM@17 ld64.lld --help:

lto_library is obsolete.

-lto_library <path> Obsolete. LLD supports LTO directly, without using an external dylib.

As of the optimization levels, there are multiple options for different purposes.

--lto-CGO<cgopt-level>  Set codegen optimization level for LTO (default: 2)
--lto-O<opt-level>      Set optimization level for LTO (default: 2)

-O <value>  Optimize output file size

-O * is only for file size optimization.

Perhaps, -plugin-opt=O* should be converted to --lto-CGO*.

[bors]

☔ The latest upstream changes (presumably #127216) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@cormacrelf any updates on this? thanks

[Dylan-DPC]

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks