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

Find a way to get rid of src\tools\lld-wrapper #97352

Closed
petrochenkov opened this issue May 24, 2022 · 19 comments
Closed

Find a way to get rid of src\tools\lld-wrapper #97352

petrochenkov opened this issue May 24, 2022 · 19 comments
Labels
A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

petrochenkov commented May 24, 2022

#89288 added a new wrapper tool the only purpose of which is to call LLD with the right flavor when rust-lld shipped with Rust distribution is used.

I strongly suspect that it is not necessary and the whole setup can be simplified.

The LLD executable itself is already a wrapper that does dispatch on its name and passed -flavor, we should be able to make it do what is necessary for us.
In the worst case we can patch src\llvm-project\lld\tools\lld\lld.cpp, since rust-lld is built and shipped by us using our fork of LLVM, that would still be better than a separate wrapper tool.

@petrochenkov petrochenkov added A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 24, 2022
@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 24, 2022

Also, an advice from lld.cpp that is relevant to the way we are shipping and calling LLD:

// lld can be invoked as "lld" along with "-flavor" option. This is for
// backward compatibility and not recommended.

@petrochenkov
Copy link
Contributor Author

I'd be interested in addressing this myself, but I cannot properly test this on Apple targets with ld64, which was one of the primary motivations for introducing the wrapper.

@bjorn3
Copy link
Member

bjorn3 commented May 24, 2022

The LLD executable itself is already a wrapper that does dispatch on its name

That would require copying lld for every variant. As stated in the description of #89288:

Symbolic links could not be used as they are not supported by rustup and on Windows.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 24, 2022

I don't see why the copy named ld64 is necessary.
Pre-#88250 logic worked mostly fine, clang correctly found the ld file shipped by us, we just could add -Wl,-flavor=ld64 for it to work correctly?

UPD: The -flavor could even be passed unconditionally, like it's done with "naked" LLD (fn command in compiler\rustc_codegen_ssa\src\back\command.rs).

@petrochenkov
Copy link
Contributor Author

Symbolic links could not be used as they are not supported by rustup and on Windows.

I'm not sure whether it's necessary to keep both rust-lld and gcc-ld/ld copies of LLD, may be it is at least for backward compatibility, and at least for some time.
In that case the wrapper can be used just as a "symlink emulation", but in that case it doesn't need to do any dispatching.

@petrochenkov
Copy link
Contributor Author

cc @hkratz

@bjorn3
Copy link
Member

bjorn3 commented May 24, 2022

Each variant only works for some platforms. If we only keep ld, we can't link for example for wasm or macOS. While if we keep ld64, we can link for any non-apple OS.

@petrochenkov
Copy link
Contributor Author

Sorry, I don't understand.
A single LLD executable can (cross-)link to any platform, regardless of how its file named, if rustc passes correct -flavor (which can change depending on rustc ... --target ...).

@hkratz
Copy link
Contributor

hkratz commented May 24, 2022

-flavor <flavor> only works if passed as the first two arguments to lld. We don't call lld directly from rustc and as far as I know there is no way to make gcc/clang insert custom arguments to the lld call at the beginning.

@petrochenkov
Copy link
Contributor Author

Ah, I see, that's really unfortunate.
It means we'll need "symlinks" like ld64 for all LLD flavors, and that wasm-ld and lld-link are not actually yet supported by -Zgcc-ld=lld.
(lld-link doesn't make sense in combination with gcc though, mingw uses slightly hackier flavor detection based on -m options.)

@petrochenkov
Copy link
Contributor Author

This

In the worst case we can patch src\llvm-project\lld\tools\lld\lld.cpp, since rust-lld is built and shipped by us using our fork of LLVM

is still an option though.
We can make -flavor position-independent (under the same or other name).

We can also try upstream the change, reliably passing the flavor through a C compiler is a good motivation, IMO.

@hkratz
Copy link
Contributor

hkratz commented May 24, 2022

We can also try upstream the change, reliably passing the flavor through a C compiler is a good motivation, IMO.

There was patch that was abandoned in 2020 which is why I did not pursue that: https://reviews.llvm.org/D80184

@tschuett
Copy link

https://reviews.llvm.org/D17952

[Clang] Accept absolute paths in the -fuse-ld option

@petrochenkov
Copy link
Contributor Author

#97375 tries to implement something that almost matches #97352 (comment), except that we are using the lld-wrapper for moving the flavor argument from an arbitrary position to the first&second one.

@bjorn3
Copy link
Member

bjorn3 commented May 25, 2022

@tschuett https://reviews.llvm.org/D17952 only affects clang and not gcc. In addition it doesn't help with getting rid of lld-wrapper, just with getting rid of the -B argument in favor of -fuse-ld for specifying the linker location.

@tschuett
Copy link

I know that the project is moving from Python to rust.

Instead of symlinks, you could have super dooper tiny Python scripts forwarding to rust-lld.

@bjorn3
Copy link
Member

bjorn3 commented May 25, 2022

Python has way too long of a startup time for that. For small programs starting python would likely take longer than linking using lld saves.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue May 25, 2022
Simplify implementation of `-Z gcc-ld`

- The logic is now unified for all targets (wasm targets should also be supported now)
- Additional "symlink" files like `ld64` are eliminated
- lld-wrapper is used for propagating the correct lld flavor
- Cleanup "unwrap or exit" logic in lld-wrapper

cc rust-lang#97352
r? `@bjorn3`
@petrochenkov
Copy link
Contributor Author

Closing this issue, lld-wrapper cannot be removed until

  • rustup can support symlinks or hardlinks, or the rust-lld copy of lld can be removed (for now it needs to be kept at least for some backward compatibility)
  • lld supports passing flavor through gcc/clang reliably, e.g. by supporting the -flavor option on any position.

@aganea
Copy link

aganea commented Aug 22, 2023

Hello,

FWIW, not sure if that helps this issue, but LLVM 17.0.0 will support usage of LLD as a library, see https://reviews.llvm.org/D119049

In practice I think that means rust-lld.exe could be merged/embedded into rustc.exe. Most likely the binary size won't increase that much, since most of the code comes from llvm/lib/ and is already included in rustc.exe (or its DLLs).

Overall the merging will improve Rust build execution times on Windows, where launching and starting executables rapidly, like Rust does, is not a good thing, especially on large core count machines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants