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.

Sign up for GitHub

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

Jump to bottom

Windows MSVC CRT linking #211

Closed
ChrisDenton opened this issue Apr 17, 2023 · 2 comments
Closed

Windows MSVC CRT linking #211

ChrisDenton opened this issue Apr 17, 2023 · 2 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@ChrisDenton
Copy link
Member

Proposal

Problem statement

With the windows-msvc targets, linking the C runtime can have a number of different configurations. For example, linking the CRT libraries compiled with debug symbols. However, currently the libc crate (when compiled as a dependency of std) does not allow this. It hardcodes a choice of only two configurations (static or dynamic), controllable with the target-feature compiler flag.

Motivation, use-cases

When integrating with C/C++ code, it's essential that the same CRT is used for the Rust parts as the C/C++ parts. Mixing CRTs is unsupported. Currently this requires ensuring the C/C++ part of the build to match Rust. The cc crate can take care of automatically selecting the static or dynamic CRT but this means it can't offer to use debug libraries or other CRTs (because they would be incompatible with Rust).

Even in a pure Rust build, linking the debug CRT may be useful for debugging.

Solution sketches

Add a cfg option to the libc crate so that when compiled with rustc-dep-of-std the right CRT can be selected. Users can then set rustflags in config.toml to set the options for a build.

[target.'cfg(all(windows, target_env="msvc"))']
rustflags = [
    "-C", "target-feature=+crt-static",
    "--cfg", 'msvc_crt_link="debug"',
]

This cfg can also be detected in build scripts using CARGO_CFG_MSVC_CRT_LINK so that crates such as cc can adjust other native builds accordingly. Options include "debug" for linking the debug CRT the and "nocrt" for taking manual control of linking the CRT (essentially equivalent to using nostd).

Links and related work

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.
@ChrisDenton ChrisDenton added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Apr 17, 2023
@danakj
Copy link

danakj commented Apr 20, 2023

AIUI this proposal would be sufficient to allow Debug Windows builds of rustc-linked targets in the Chromium project.

As we build the stdlib separately for release or debug configurations, and the choice applies to all build targets, the choice of CRT when building stdlib would allow us to link C++ into Rust.

aarongable pushed a commit to chromium/chromium that referenced this issue May 4, 2023
@danakj
Fix Rust linking on component and debug Windows builds
e917dea 
1. The CRT can be specified from rustc but only static/dynamic,
not release/debug. This is tracked upstream in
rust-lang/libs-team#211

Because of this, we must not use the CRT command line options
from rustc, and instead specify the library directly. This is
done at the same location where the CRT is chosen for C++
targets via the "/MD" and friends flags.

Then, we must not have rustc try to also link the CRT and we
disable that by setting -Zlink-directives=false for the libc
crate.

2. After the above, we have two sets of libraries that must be
linked when using the rust stdlib.

Internal dependencies, which are #[link] directives from the
libc crate (other than the CRT), which must be linked any time
the rust lib is used.

Public dependencies, which are linked automatically by rustc
but are not linked by C++ targets that depend on Rust targets.

So we split these into two configs, and expose the latter on
the BUILD rule for C++ targets that depend on Rust.

3. The local build of stdlib is here to stay and is always used
with the Chromium toolchain, so remove a bunch of configuration
levers around it and just hardcode the stdlib in when using the
Chromium toolchain. And hardcode the prebuilt stdlib path when
not using the Chromium toolchain. This removes a bunch of
branches elsewhere in our GN rules as we can simply depend on
the "rust stdlib target" instead of choosing between them.

Then drop the test build target that was opting into the local
stdlib as all our Rust build tests do that now.

4. Don't use  __attribute__((visibility("default"))) on Windows,
use __declspec(dllexport) instead.

I don't know that this fixes anything, but that attribute doesn't
exist on Windows, so yeah.

5. In the component build, C++ targets are linked with the dynamic
CRT. In order to have mixed targets, Rust needs to as well. Then
Rust build script exes are linked with the dynamic CRT and they are
unable to find the DLL on the bots (which don't have Visual Studio)
installed. The CRT DLL is placed in root_out_dir, so we move build
script exes to the root_out_dir in order to have them find the DLL
and be able to execute.

6. Ensure the CRT DLLs are present on the bots when running the
tests by adding a data_deps edge to them for build_rust_tests.
Normally this edge comes from a dependency on //base which basically
everything has, but our Rust build tests don't.

7. Disable rust_shared_library unit test on Windows component build
while we figure out how to get the right `data_deps` edge from the
test to the library so that it's included in the isolate. When
building a library's tests, the library itself is recompiled under
cfg=test as part of the test exe. So the rlib or shared library
itself is not used. But we still need to depend on any transitive
deps of that library when building the unit tests, so the unit tests
add a deps edge onto the library.

For shared libraries this deps edge doesn't do anything on Linux, or
on Windows non-component builds. But on Windows component builds, the
unit test EXE acquires a dependency on the DLL (as shown by dumpbin)
and thus the DLL needs to be present when running the test.

Bug: 1442273, 1434719

Change-Id: I157a0728ec25f636d0b78973b8ab81205e7b25ef
Cq-Include-Trybots: luci.chromium.try:win-rust-x64-rel,win-rust-x64-dbg,linux-rust-x64-rel,linux-rust-x64-dbg,android-rust-arm64-rel,android-rust-arm64-dbg,android-rust-arm32-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4499979
Reviewed-by: Bruce Dawson <[email protected]>
Commit-Queue: danakj <[email protected]>
Reviewed-by: Adrian Taylor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1139572}
UI-RayanWang pushed a commit to ubiquiti/ubnt_libjingle_component_src_build that referenced this issue Aug 16, 2023
@danakj @copybara-github
Fix Rust linking on component and debug Windows builds
5311fc0 
1. The CRT can be specified from rustc but only static/dynamic,
not release/debug. This is tracked upstream in
rust-lang/libs-team#211

Because of this, we must not use the CRT command line options
from rustc, and instead specify the library directly. This is
done at the same location where the CRT is chosen for C++
targets via the "/MD" and friends flags.

Then, we must not have rustc try to also link the CRT and we
disable that by setting -Zlink-directives=false for the libc
crate.

2. After the above, we have two sets of libraries that must be
linked when using the rust stdlib.

Internal dependencies, which are #[link] directives from the
libc crate (other than the CRT), which must be linked any time
the rust lib is used.

Public dependencies, which are linked automatically by rustc
but are not linked by C++ targets that depend on Rust targets.

So we split these into two configs, and expose the latter on
the BUILD rule for C++ targets that depend on Rust.

3. The local build of stdlib is here to stay and is always used
with the Chromium toolchain, so remove a bunch of configuration
levers around it and just hardcode the stdlib in when using the
Chromium toolchain. And hardcode the prebuilt stdlib path when
not using the Chromium toolchain. This removes a bunch of
branches elsewhere in our GN rules as we can simply depend on
the "rust stdlib target" instead of choosing between them.

Then drop the test build target that was opting into the local
stdlib as all our Rust build tests do that now.

4. Don't use  __attribute__((visibility("default"))) on Windows,
use __declspec(dllexport) instead.

I don't know that this fixes anything, but that attribute doesn't
exist on Windows, so yeah.

5. In the component build, C++ targets are linked with the dynamic
CRT. In order to have mixed targets, Rust needs to as well. Then
Rust build script exes are linked with the dynamic CRT and they are
unable to find the DLL on the bots (which don't have Visual Studio)
installed. The CRT DLL is placed in root_out_dir, so we move build
script exes to the root_out_dir in order to have them find the DLL
and be able to execute.

6. Ensure the CRT DLLs are present on the bots when running the
tests by adding a data_deps edge to them for build_rust_tests.
Normally this edge comes from a dependency on //base which basically
everything has, but our Rust build tests don't.

7. Disable rust_shared_library unit test on Windows component build
while we figure out how to get the right `data_deps` edge from the
test to the library so that it's included in the isolate. When
building a library's tests, the library itself is recompiled under
cfg=test as part of the test exe. So the rlib or shared library
itself is not used. But we still need to depend on any transitive
deps of that library when building the unit tests, so the unit tests
add a deps edge onto the library.

For shared libraries this deps edge doesn't do anything on Linux, or
on Windows non-component builds. But on Windows component builds, the
unit test EXE acquires a dependency on the DLL (as shown by dumpbin)
and thus the DLL needs to be present when running the test.

Bug: 1442273, 1434719

Change-Id: I157a0728ec25f636d0b78973b8ab81205e7b25ef
Cq-Include-Trybots: luci.chromium.try:win-rust-x64-rel,win-rust-x64-dbg,linux-rust-x64-rel,linux-rust-x64-dbg,android-rust-arm64-rel,android-rust-arm64-dbg,android-rust-arm32-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4499979
Reviewed-by: Bruce Dawson <[email protected]>
Commit-Queue: danakj <[email protected]>
Reviewed-by: Adrian Taylor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1139572}
NOKEYCHECK=True
GitOrigin-RevId: e917deabc983569acfd001b06fa71ea3471d8ff3
@ChrisDenton
Copy link
Member Author

ChrisDenton commented Apr 15, 2024 •
edited
Loading

Linking other CRTs is now possible as the CRT is linked using /DEFAUTLIB (in latest nightly) so it's easy to disable (e.g. using /NODEFAULTLIB:msvcrt) and then override. I feel that's sufficient for build systems to set their own CRT even if not the most ergonomic.

Rust having more built-in options would be nice but it's not something I have the bandwidth to pursue atm. If someone else wants to then I can reopen (or feel free to make a different proposal).

@ChrisDenton ChrisDenton closed this as not planned Won't fix, can't repro, duplicate, stale Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@danakj @ChrisDenton