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

ccinfo: when providing ccinfo, optionally include libstd and alloc #624

Merged
merged 5 commits into from
Mar 9, 2021

Conversation

durin42
Copy link
Contributor

@durin42 durin42 commented Mar 3, 2021

The new attribute on RustToolchain is the label of a target that
provides __rust_realloc et al, which allows ld(1) to use the .rlib
files directly without needing to involve rustc in the linking
step. This means Rust and C++ can be mixed in a cc_binary freely
without needing any staticlib-type crates, which avoids problems if
you have a cc_binary -> rust_library -> cc_library -> rust_library
situation.

@google-cla google-cla bot added the cla: yes label Mar 3, 2021
@durin42
Copy link
Contributor Author

durin42 commented Mar 3, 2021

(I expect this to be broken, please don't review just yet.)

@durin42 durin42 force-pushed the extralibs branch 3 times, most recently from 9aee1d0 to 43f5664 Compare March 3, 2021 20:20
@durin42
Copy link
Contributor Author

durin42 commented Mar 3, 2021

(Still some more to sort out, please don't merge yet.)

@hlopko hlopko self-requested a review March 4, 2021 12:31
rust/private/rustc.bzl Outdated Show resolved Hide resolved
rust/toolchain.bzl Outdated Show resolved Hide resolved
rust/toolchain.bzl Outdated Show resolved Hide resolved
rust/toolchain.bzl Outdated Show resolved Hide resolved
rust/toolchain.bzl Outdated Show resolved Hide resolved
@hlopko
Copy link
Member

hlopko commented Mar 4, 2021

This looks reasonable so far. WDYT about also submitting your kludge cc_library? It is currently rustc version dependent, but we can make it work now, see how often it breaks, and we can get feedback on it from the community. It is also good to be able to show what we do now when discussing rust-lang/rust#73632 with folks.

rust/toolchain.bzl Show resolved Hide resolved
@durin42
Copy link
Contributor Author

durin42 commented Mar 4, 2021

Still pending: a test, and I think this ends up providing the rust_lib libraries in the wrong order, causing linker problems. But close.

More later today, I hope.

@durin42 durin42 force-pushed the extralibs branch 2 times, most recently from c64fdfd to d0a304b Compare March 5, 2021 04:18
@durin42
Copy link
Contributor Author

durin42 commented Mar 5, 2021

Progress, mostly: for some reason linking is failing to find the hello_from_rust symbol. The linker invocation looks vaguely okay, so I'll stop here for now.

@durin42
Copy link
Contributor Author

durin42 commented Mar 5, 2021

Okay, closer. Lingering problems:

  1. I'm not sure how to configure the toolchain in the test to tell it about the allocator library

  2. core needs__muloti4, which is missing during linking.

I'm really stopping for the night this time.

@durin42
Copy link
Contributor Author

durin42 commented Mar 5, 2021

Looked into that linker failure more. As far as I can tell, that's a subtle incompatibility between the libcore Rust produces and libgcc (libcore is assuming compiler-rt) when doing 128 bit overflow checks. rust-lang/rust#8449 is related-ish, at least. It sounds to me like we need some work done in LLVM to make the overflow checks compatible with libgcc again, which will then have to percolate through the Rust ecosystem next time they pick up an LLVM release.

Between that and the fact that MSVC can't seem to cope with the allocator library sources, I'm out of ideas for ways to test this in-tree. I can add a test that validates the presence of the libstd CcInfo on a simple rust_library, but that seems to be about it. Is that enough for folks to be happy?

#include <stddef.h>
#include <stdlib.h>

void* __rdl_alloc(size_t, size_t);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put this file into generated rust toolchains for linux when cc rules detected clang, but let's do it in a followup PR.

test/unit/native_deps/cc_bin_uses_rust_library.cc Outdated Show resolved Hide resolved
rust/toolchain.bzl Show resolved Hide resolved
@hlopko
Copy link
Member

hlopko commented Mar 9, 2021

Regarding testing, I'd be fine with excluding the failing test on windows (also mac?) CI tasks (.bazelci/presubmit.yml). For linux, this still doesn't work with gcc right? In that case I think we can add new tasks for clang on linux (CI workers should have clang installed, I'm not sure if they use libc++ though)

durin42 and others added 4 commits March 9, 2021 13:31
The new attribute on RustToolchain is the label of a target that
provides __rust_realloc et al, which allows ld(1) to use the .rlib
files directly without needing to involve rustc in the linking
step. This means Rust and C++ can be mixed in a cc_binary freely
without needing any staticlib-type crates, which avoids problems if
you have a cc_binary -> rust_library -> cc_library -> rust_library
situation.
@durin42 durin42 force-pushed the extralibs branch 2 times, most recently from e93f789 to 799d8dd Compare March 9, 2021 19:47
This only works on clang today, and can only work on clang for the
near-term. It's not clear how to write a clang-only test in this case,
so we just give up for now.

"I promise it works!" etc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants