Skip to content

Commit

Permalink
Fix Rust linking on component and debug Windows builds
Browse files Browse the repository at this point in the history 
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 <brucedawson@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Adrian Taylor <adetaylor@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1139572}
NOKEYCHECK=True
GitOrigin-RevId: e917deabc983569acfd001b06fa71ea3471d8ff3
  • Loading branch information
@danakj @copybara-github
danakj authored and copybara-github committed May 4, 2023
1 parent aa6d19f commit 5311fc0
Show file tree
Hide file tree
Showing 19 changed files with 273 additions and 253 deletions.
1 change: 0 additions & 1 deletion config/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,6 @@ config("executable_config") {

if (is_win) {
configs += _windows_linker_configs
configs += [ "//build/config/win:exe_flags" ]
} else if (is_mac) {
configs += [ "//build/config/mac:mac_dynamic_flags" ]
} else if (is_ios) {
Expand Down
20 changes: 0 additions & 20 deletions config/rust.gni
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,6 @@ declare_args() {
# Use the Rust toolchain built in-tree. See //tools/rust.
use_chromium_rust_toolchain = true

# Build libstd locally with GN and use that instead of the prebuilts, where
# applicable. If this is false the prebuilt libstd will always be used. If
# true, the local build is only used with the Chromium Rust toolchain and only
# on supported platforms and GN targets.
enable_local_libstd = true

# Chromium currently has a Rust toolchain for Android and Linux, but
# if you wish to experiment on more platforms you can use this
# argument to specify an alternative toolchain.
Expand Down Expand Up @@ -87,13 +81,6 @@ declare_args() {
# Use a separate declare_args so these variables' defaults can depend on the
# ones above.

# When true, uses the locally-built std in all Rust targets.
#
# As an internal implementation detail this can be overridden on specific
# targets (e.g. to run build.rs scripts while building std), but this
# generally should not be done.
use_local_std_by_default = enable_local_libstd && use_chromium_rust_toolchain

# Individual Rust components.

# Conversions between Rust types and C++ types.
Expand Down Expand Up @@ -286,13 +273,6 @@ assert(!use_thin_lto || !enable_rust || use_chromium_rust_toolchain ||
use_unverified_rust_toolchain,
"Must use Chromium Rust toolchain for LTO")

# Determine whether the local libstd can and should be built.
local_libstd_supported = enable_local_libstd && use_chromium_rust_toolchain

# Determine whether the prebuilt libstd can be used
prebuilt_libstd_supported = !use_chromium_rust_toolchain ||
(target_os == "linux" && target_cpu == "x64")

# Arguments for Rust invocation.
# This is common between gcc/clang, Mac and Windows toolchains so specify once,
# here. This is not the complete command-line: toolchains should add -o
Expand Down
33 changes: 28 additions & 5 deletions config/win/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,11 +329,6 @@ config("common_linker_setup") {
}
}

# Flags that should be applied to building .exe files but not .dll files.
config("exe_flags") {
rustflags = [ "-Ctarget-feature=+crt-static" ]
}

config("default_cfg_compiler") {
# Emit table of address-taken functions for Control-Flow Guard (CFG).
# This is needed to allow functions to be called by code that is built
Expand Down Expand Up @@ -506,11 +501,25 @@ config("dynamic_crt") {
if (is_debug) {
# This pulls in the DLL debug CRT and defines _DEBUG
cflags = [ "/MDd" ]

# /MDd specifies msvcrtd.lib as the CRT library. Rust needs to agree, so
# we specify it explicitly.
# Once https://github.com/rust-lang/rust/issues/39016 is resolved we should
# instead tell rustc which CRT to use (static/dynamic + release/debug).
rustflags = [ "-Clink-arg=msvcrtd.lib" ]

if (use_custom_libcxx) {
ldflags = [ "/DEFAULTLIB:msvcprtd.lib" ]
}
} else {
cflags = [ "/MD" ]

# /MD specifies msvcrt.lib as the CRT library. Rust needs to agree, so
# we specify it explicitly.
# Once https://github.com/rust-lang/rust/issues/39016 is resolved we should
# instead tell rustc which CRT to use (static/dynamic + release/debug).
rustflags = [ "-Clink-arg=msvcrt.lib" ]

if (use_custom_libcxx) {
ldflags = [ "/DEFAULTLIB:msvcprt.lib" ]
}
Expand All @@ -521,11 +530,25 @@ config("static_crt") {
if (is_debug) {
# This pulls in the static debug CRT and defines _DEBUG
cflags = [ "/MTd" ]

# /MTd specifies libcmtd.lib as the CRT library. Rust needs to agree, so
# we specify it explicitly.
# Once https://github.com/rust-lang/rust/issues/39016 is resolved we should
# instead tell rustc which CRT to use (static/dynamic + release/debug).
rustflags = [ "-Clink-arg=libcmtd.lib" ]

if (use_custom_libcxx) {
ldflags = [ "/DEFAULTLIB:libcpmtd.lib" ]
}
} else {
cflags = [ "/MT" ]

# /MT specifies libcmt.lib as the CRT library. Rust needs to agree, so
# we specify it explicitly.
# Once https://github.com/rust-lang/rust/issues/39016 is resolved we should
# instead tell rustc which CRT to use (static/dynamic + release/debug).
rustflags = [ "-Clink-arg=libcmt.lib" ]

if (use_custom_libcxx) {
ldflags = [ "/DEFAULTLIB:libcpmt.lib" ]
}
Expand Down
12 changes: 0 additions & 12 deletions rust/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,7 @@ if (toolchain_has_rust) {
# Depending on the C++ bindings side of cxx then requires also depending
# on the Rust bindings, since one calls the other. And the Rust bindings
# require the Rust standard library.
# Normally the Rust stdlib is brought in as a dependency by depending
# on any first-party Rust target. But in this case, it's conceivable
# that pure-C++ targets will not depend on any 1p Rust code so we'll add
# the Rust stdlib explicitly.
deps = [ ":cxx_rustdeps" ]

if (use_local_std_by_default) {
deps += [ "//build/rust/std:link_local_std" ]
} else {
assert(prebuilt_libstd_supported,
"Prebuilt Rust stdlib is not available for this target")
deps += [ "//build/rust/std:link_prebuilt_std" ]
}
}

# The required dependencies for cxx-generated bindings, that must be included
Expand Down
26 changes: 14 additions & 12 deletions rust/cargo_crate.gni
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,13 +266,12 @@ template("cargo_crate") {
":${_build_script_name}($host_toolchain_no_sanitizers)"
deps = [ build_script_target ]

# The build script output is always in the name-specific output dir. It
# may be built with a different toolchain when cross-compiling (the host
# toolchain) so we must find the path relative to that.
_build_script_target_out_dir =
get_label_info(build_script_target, "target_out_dir")
_build_script_exe =
"$_build_script_target_out_dir/$_orig_target_name/$_build_script_name"
# The build script may be built with a different toolchain when
# cross-compiling (the host toolchain) so we must find the path relative
# to that.
_build_script_root_out_dir =
get_label_info(build_script_target, "root_out_dir")
_build_script_exe = "$_build_script_root_out_dir/$_build_script_name"
if (is_win) {
_build_script_exe = "${_build_script_exe}.exe"
}
Expand Down Expand Up @@ -328,11 +327,14 @@ template("cargo_crate") {
deps = invoker.build_deps
}

# An rlib's build script may be built differently for tests and for
# production, so they must be in a name specific to the GN target. The
# ${_build_script_name}_output target looks for the exe in this
# location.
output_dir = "$target_out_dir/$_orig_target_name"
# The ${_build_script_name}_output target looks for the exe in this
# location. Due to how the Windows component build works, this has to
# be $root_out_dir for all EXEs. In component build, C++ links to the
# CRT as a DLL, and if Rust does not match, we can't link mixed target
# Rust EXE/DLLs, as the headers in C++ said something different than
# what Rust links. Since the CRT DLL is placed in the $root_out_dir,
# an EXE can find it if it's also placed in that dir.
output_dir = root_out_dir
rustenv = _rustenv
forward_variables_from(invoker,
[
Expand Down
33 changes: 7 additions & 26 deletions rust/rust_target.gni
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,6 @@ template("rust_target") {
_visibility = invoker.visibility
}

_use_local_std = use_local_std_by_default
if (defined(invoker.use_local_std)) {
_use_local_std = invoker.use_local_std
}

_rustflags = []
if (defined(invoker.rustflags)) {
_rustflags += invoker.rustflags
Expand All @@ -133,6 +128,10 @@ template("rust_target") {
_configs += invoker.proc_macro_configs
_test_configs += [ "//build/rust:proc_macro_extern" ]
}
} else if (invoker.target_type == "rust_proc_macro") {
if (defined(invoker.shared_library_configs)) {
_configs += invoker.shared_library_configs
}
} else {
if (defined(invoker.library_configs)) {
_configs += invoker.library_configs
Expand Down Expand Up @@ -223,7 +222,6 @@ template("rust_target") {
"_rustflags",
"_support_use_from_cpp",
"_testonly",
"_use_local_std",
"_visibility",
])
} else {
Expand Down Expand Up @@ -262,23 +260,11 @@ template("rust_target") {
# target that depends on a rust target directly may need access to Cxx
# as well, which means it must appear in public_deps.
public_deps += [ "//build/rust:cxx_cppdeps" ]

# cxx_cppdeps pulls in the default libstd, so make sure the default was
# not overridden.
assert(
_use_local_std == use_local_std_by_default,
"Rust targets with cxx bindings cannot override the default libstd")
} else if (!defined(invoker.no_std) || !invoker.no_std) {
# If C++ depends on and links in the library, we need to make sure C++
# links in the Rust stdlib. This is orthogonal to if the library exports
# bindings for C++ to use.
if (_use_local_std) {
deps = [ "//build/rust/std:link_local_std" ]
} else {
assert(prebuilt_libstd_supported,
"Prebuilt Rust stdlib is not available for this target")
deps = [ "//build/rust/std:link_prebuilt_std" ]
}
deps = [ "//build/rust/std:stdlib_for_clang" ]
}
}

Expand All @@ -298,13 +284,7 @@ template("rust_target") {
}

if (!defined(invoker.no_std) || !invoker.no_std) {
if (_use_local_std) {
_rust_deps += [ "//build/rust/std:local_std_for_rustc" ]
} else {
_rust_deps += [ "//build/rust/std:prebuilt_std_for_rustc" ]
}
} else {
not_needed([ "_use_local_std" ])
_rust_deps += [ "//build/rust/std:stdlib_for_rustc" ]
}

# You must go through the groups above to get to these targets.
Expand Down Expand Up @@ -449,5 +429,6 @@ template("rust_target") {
set_defaults("rust_target") {
executable_configs = default_executable_configs
library_configs = default_compiler_configs
shared_library_configs = default_shared_library_configs
proc_macro_configs = default_rust_proc_macro_configs
}
Loading

0 comments on commit 5311fc0

Please sign in to comment.