Support linking against system clang libs

[infinity0]

Some platforms already have clang runtime libs available. In this case we can link against them directly, rather than rebuilding them (which requires the sources to be available, which has been removed recently).

[infinity0]

Thanks for the comments, I'll get around to them at some point. But first I need to understand this crate better before proceeding with this PR, since I'm not entirely sure if this approach is viable.

In particular, LLVM does not built this static library (libclang_rt.builtins-$arch.a) for some architectures, e.g. s390x Fedora and sparc64 Debian. However the equivalent Rust build on those architectures does build the C code as rust's own libcompiler-rt.a as with all architectures.

Furthermore I am not sure how to actually meaningfully test if any of what I'm doing is actually working. Even if I comment out the c_system::compile line to disable system linking, both the build, the tests, and the tests in testcrate/ all pass successfully. So I guess they are falling back to the Rust implementation or something? Also if I change the sources map to have bogus entries, e.g. renaming __clzdi2 to __clzdi2XXX, everything also still works.

[alexcrichton]

Yeah the best way to test this would be to run it through rustc's CI or something similar, the CI here in this repository has been really difficult to get right for that reason becaues it's so easy to accidentally pull in the system and/or Rust version. I don't know of a great way to test this.

[bors]

:umbrella: The latest upstream changes (presumably #309) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

This looks reasonable enough to me, @infinity0 is this ready for landing?

I'm not really reviewing the pieces here that closely, only that it's not affecting what's already there too much

[infinity0]

This is being tested on Debian's CI right now across about 10-13 architectures, we should have an answer in about 24 hours or less.

One issue is that the LLVM_CONFIG envvar is already being used by the rustc build in a very specific way, which force-enables various LLVM sanitizers - not a default behaviour, and fails with Debian's system LLVM. So I patched Debian's rustc to use a different envvar for that purpose. If you'd rather keep the existing behaviour then we'll need to change the envvar in this PR to something different, to avoid the clash with rustc's usage.

[infinity0]

Oh and I didn't get around to your comment about not shelling out to ls yet but I'll do that when everything else is ready and agreed upon.

[alexcrichton]

I don't have any preference really around handling LLVM_CONFIG, the build process of the sanitizers has always been ad-hoc and never ready for stabilization, so tweaking that upstream or leaving as-is seems fine by me so long as it works for your use case.

[infinity0]

Actually, looking again at newer versions of rustc, it seems that it no longer needs the C versions of the intrinsics - feature "rustc-dep-of-std" no longer depends on feature "c", since rustc 1.36.0 (with compiler_builtins 0.1.14). Is my interpretation correct? If so then this PR is actually unnecessary for rustc in Debian, but might be useful for other use-cases in the future.

[alexcrichton]

The standard library can be built without C symbols yeah but by default for releases the C symbols are enabled

[infinity0]

Sorry I'm a bit confused. When I said "no longer needs the C versions of the intrinsics" I meant the versions implemented in C by LLVM compiler-rt, that the "c" feature would bring in. But it seems that rustc no longer needs these, because:

• If I run rg compiler_builtins Cargo.toml src/*/Cargo.toml for rustc 1.37.0 I see that the only dependencies of rustc on compiler_builtins either uses the latter's default feature, or the "rustc-dep-of-std" feature, and neither of these now include the "c" feature of compiler_builtins.

Because of this, I'm not sure what you mean by "by default for releases the C symbols are enabled", since the C implementations are not included by default (as far as I can tell).

If OTOH you meant "the Rust implementations are exposed as C ABI functions" then yes I agree, but I'm confused on how this is relevant to what I was talking about before - the C ABI functions remain exposed with or without this PR, and for older and newer versions of rustc, and I'm not sure how it would be possible to disable them.

[mati865]

@infinity0 Rust ships with C code compiled from llvm-project submodule, see https://github.com/rust-lang/rust/pull/60981/files

Also see last line here:

$ rg "compiler_builtins" src/*/Cargo.toml 
src/liballoc/Cargo.toml
15:compiler_builtins = { version = "0.1.10", features = ['rustc-dep-of-std'] }
35:compiler-builtins-mem = ['compiler_builtins/mem']
36:compiler-builtins-c = ["compiler_builtins/c"]

[infinity0]

@mati865 Yes I know the source C files are shipped as part of the source release, but they don't appear to be enabled. The "compiler-builtins-c" feature you indicated is not enabled by default, and none of the other features depending on it within rustc are enabled by default either:

$ rg "compiler-builtins-c" src/*/Cargo.toml
src/liballoc/Cargo.toml
36:compiler-builtins-c = ["compiler_builtins/c"]

src/libstd/Cargo.toml
67:compiler-builtins-c = ["alloc/compiler-builtins-c"]

edit: to be clear, in previous versions of rustc <= 1.35, they were enabled as the older versions of the compiler_builtins crate defined feature "c" as a feature of "rustc-dep-of-std". This is no longer the case.

For current versions of rustc, it seems that one has to enable the feature "compiler-builtins-c", but I don't see that this is done by default anywhere. For example I don't see this in https://github.com/rust-lang/rust-forge or https://github.com/rust-lang/rust-central-station, and I don't know where else to look. I'm not even sure how one could enable those features - e.g. ./x.py build doesn't appear to take a --features flag.

As I said I can still proceed with this PR since I think it's useful, I'm just trying to avoid getting confused by various comments since I only half-know what I'm doing here.

[mati865]

@infinity0 look closely at https://github.com/rust-lang/rust/pull/60981/files#diff-fa9668c926a2788f7849514fe3ed66a9. It will enable compiler-builtins-c feature if src/llvm-project/compiler-rt path exists.

[infinity0]

@mati865 Ah ok thanks, that was the missing link, sorry for the confusion. Then I will finish this PR as discussed above and deploy it in Debian as well. Do you know of a good way to test (even manually) whether the optimised C versions are actually being used? Presumably I could objdump libcompiler_builtins.rlib and examine the assembly?

[mati865]

Do you know of a good way to test (even manually) whether the optimised C versions are actually being used?

I don't know the whole process here, sorry.

[infinity0]

OK, so I looked into the generated rlib in a bit more detail and I think the current approach won't work, because rustc-link-lib ends up including (for some symbols) both the C version and the Rust version, which will cause a duplicate symbol error at link time. I'll need to filter out the library first, probably using the ar crate. This should be straightforward though.

[infinity0]

OK, this version should work and I've added a script to manually check for duplicate symbols. We are waiting on mdsteele/rust-ar#14 however.

[infinity0]

why is ar needed to extract from the archives? How come linking it in with rustc doesn't work?

clang's builtins contains all the symbols, but this crate defines rust versions of some symbols. If we link in clang's builtins directly, we'll link in both the C versions and the rust versions, leading to duplicate symbols errors.

[alexcrichton]

Oh right sorry, I understand now. Makes sense

[alexcrichton]

Great! I think though CI may be failing?

[infinity0]

I tried to fix it in the latest commit but it seems now CI is not running at all? Are you able to see what's wrong with it?

[infinity0]

I just had to rebase to the latest master, but having trouble figuring out the current failure. Will keep trying.