Support linking against system clang libs #296

infinity0 · 2019-05-21T02:54:14Z

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).

alexcrichton

Looks plausible to me! This seems pretty nontrivial though, so I think it needs to be tested on CI at least in one location.

build.rs

infinity0 · 2019-05-25T20:37:40Z

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 · 2019-05-28T14:15:20Z

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 · 2019-08-19T22:20:58Z

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

alexcrichton · 2019-08-27T22:17:33Z

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 · 2019-08-28T01:25:25Z

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 · 2019-08-28T01:26:32Z

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 · 2019-08-28T14:35:55Z

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 · 2019-08-28T14:56:25Z

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 · 2019-08-28T15:13:06Z

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

infinity0 · 2019-08-28T16:52:09Z

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 · 2019-08-29T10:53:25Z

@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 · 2019-08-29T18:09:22Z

@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 · 2019-08-29T18:15:33Z

@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 · 2019-08-29T18:22:29Z

@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 · 2019-08-29T18:29:43Z

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 · 2019-08-30T01:22:06Z

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 · 2019-08-31T01:31:21Z

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.

alexcrichton

Hm I'm not sure I quite understand, but why is ar needed to extract from the archives? How come linking it in with rustc doesn't work?

ci/check_no-duplicate-symbols.sh

bjorn3 · 2019-09-03T20:25:15Z

build.rs

+                }
+            };
+            let mut entry = orig.jump_to_entry(i).unwrap();
+            // TODO: ar really should have an append_entry to avoid the clone


That clone is cheap (only some int amd the name of the entry)

infinity0 · 2019-09-04T00:59:19Z

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 · 2019-09-04T16:59:29Z

Oh right sorry, I understand now. Makes sense

Better way of conditioning the sanitizer builds Previously the build would take the presence of the LLVM_CONFIG envvar to mean that the sanitizers should be built, but this is a common envvar that could be set for reasons unrelated to the rustc sanitizers. This commit adds a new envvar RUSTC_BUILD_SANITIZERS and uses it instead. This PR or similar will be necessary in order to work correctly with rust-lang/compiler-builtins#296

alexcrichton · 2019-09-10T14:17:57Z

Great! I think though CI may be failing?

infinity0 · 2019-09-12T06:00:05Z

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 · 2019-09-12T07:42:41Z

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

ognevny · 2024-05-10T09:54:52Z

any news on this?

infinity0 · 2024-06-09T19:21:37Z

I never finished this, and have no intention to at the moment. Somebody else is welcome to clone my fork and take it over.

alexcrichton reviewed May 21, 2019

View reviewed changes

build.rs Outdated Show resolved Hide resolved

infinity0 force-pushed the master branch 2 times, most recently from 1e9f88b to cc934bb Compare August 24, 2019 17:17

infinity0 force-pushed the master branch from cc934bb to e1afff0 Compare August 31, 2019 01:29

infinity0 force-pushed the master branch 2 times, most recently from 0e64d5e to baba65d Compare August 31, 2019 17:06

alexcrichton reviewed Sep 3, 2019

View reviewed changes

ci/check_no-duplicate-symbols.sh Outdated Show resolved Hide resolved

bjorn3 reviewed Sep 3, 2019

View reviewed changes

infinity0 mentioned this pull request Sep 5, 2019

Better way of conditioning the sanitizer builds rust-lang/rust#64166

Merged

infinity0 force-pushed the master branch 5 times, most recently from 4b8bacd to 61c966b Compare September 10, 2019 02:34

infinity0 force-pushed the master branch 2 times, most recently from d18bce0 to dd549be Compare September 12, 2019 05:52

infinity0 and others added 4 commits September 11, 2019 23:01

Support linking against system clang libs

2965a3c

Support using clang as well

ec70931

Only link when the arch is actually supported by builtins

940006d

When using system clang libs, filter out duplicate symbols

eeaa5d2

infinity0 force-pushed the master branch from d18bce0 to 69c2a4e Compare September 12, 2019 06:02

Run rustfmt

e4384f6

infinity0 force-pushed the master branch 5 times, most recently from 52caa1e to 3fad51d Compare September 12, 2019 07:06

Test the c-system feature in CI as well

ca5aece

infinity0 force-pushed the master branch from 4e239f1 to ca5aece Compare September 13, 2019 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support linking against system clang libs #296

Support linking against system clang libs #296

infinity0 commented May 21, 2019

alexcrichton left a comment

infinity0 commented May 25, 2019

alexcrichton commented May 28, 2019

bors commented Aug 19, 2019

alexcrichton commented Aug 27, 2019

infinity0 commented Aug 28, 2019

infinity0 commented Aug 28, 2019

alexcrichton commented Aug 28, 2019

infinity0 commented Aug 28, 2019

alexcrichton commented Aug 28, 2019

infinity0 commented Aug 28, 2019

mati865 commented Aug 29, 2019

infinity0 commented Aug 29, 2019 •

edited

Loading

mati865 commented Aug 29, 2019

infinity0 commented Aug 29, 2019

mati865 commented Aug 29, 2019

infinity0 commented Aug 30, 2019

infinity0 commented Aug 31, 2019

alexcrichton left a comment

bjorn3 Sep 3, 2019

infinity0 commented Sep 4, 2019

alexcrichton commented Sep 4, 2019

alexcrichton commented Sep 10, 2019

infinity0 commented Sep 12, 2019

infinity0 commented Sep 12, 2019

ognevny commented May 10, 2024

infinity0 commented Jun 9, 2024

Support linking against system clang libs #296

Are you sure you want to change the base?

Support linking against system clang libs #296

Conversation

infinity0 commented May 21, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

infinity0 commented May 25, 2019

alexcrichton commented May 28, 2019

bors commented Aug 19, 2019

alexcrichton commented Aug 27, 2019

infinity0 commented Aug 28, 2019

infinity0 commented Aug 28, 2019

alexcrichton commented Aug 28, 2019

infinity0 commented Aug 28, 2019

alexcrichton commented Aug 28, 2019

infinity0 commented Aug 28, 2019

mati865 commented Aug 29, 2019

infinity0 commented Aug 29, 2019 • edited Loading

mati865 commented Aug 29, 2019

infinity0 commented Aug 29, 2019

mati865 commented Aug 29, 2019

infinity0 commented Aug 30, 2019

infinity0 commented Aug 31, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

bjorn3 Sep 3, 2019

Choose a reason for hiding this comment

infinity0 commented Sep 4, 2019

alexcrichton commented Sep 4, 2019

alexcrichton commented Sep 10, 2019

infinity0 commented Sep 12, 2019

infinity0 commented Sep 12, 2019

ognevny commented May 10, 2024

infinity0 commented Jun 9, 2024

infinity0 commented Aug 29, 2019 •

edited

Loading