Fix linker errors in CQ when compiling with starlark. There are several issues here: * rust needs to know that we're using custom clang instead of system compiler * rust needs to be passed the link args so it's aware than asan is enabled. * Apparently libraries need to be passed in *reverse* dependency order to the linker * We need to configure --target correctly in order to cross-compile * The version of rust in CIPD has a different output directory layout to the version I'm using locally. We need to support both. With these changes applied, the code (and tests) successfully compile and link on linux, mac, and windows CQ bots. https://ci.chromium.org/ui/p/gn/builders/try.shadow/linux/b8674332519420178241/overview https://ci.chromium.org/ui/p/gn/builders/try.shadow/mac/b8674332512389917857/overview https://ci.chromium.org/ui/p/gn/builders/try.shadow/win/b8674325382197679105/overview Bug: 528225104 Change-Id: Ie430dfac5f8860e1af83eb2cc8907b8f6a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/24880 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/build/gen.py b/build/gen.py index c7d03f7..bf4e5db 100755 --- a/build/gen.py +++ b/build/gen.py
@@ -20,6 +20,35 @@ SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) REPO_ROOT = os.path.dirname(SCRIPT_DIR) +def rust_arch(): + """Determines the target CPU architecture normalized for Rust.""" + cflags = os.environ.get('CFLAGS', '').split() + ldflags = os.environ.get('LDFLAGS', '').split() + + target_triple = None + for flag in cflags + ldflags: + if flag.startswith('--target='): + target_triple = flag.split('=', 1)[1] + break + elif flag.startswith('-target='): + target_triple = flag.split('=', 1)[1] + break + + if target_triple: + arch = target_triple.split('-')[0].lower() + else: + import platform as py_platform + arch = py_platform.machine().lower() + + if arch in ('amd64', 'x86_64'): + return 'x86_64' + elif arch in ('arm64', 'aarch64'): + return 'aarch64' + elif arch == 'riscv64': + return 'riscv64' + return arch + + class Platform(object): """Represents a host/target platform.""" def __init__(self, platform): @@ -103,6 +132,21 @@ def exe_suffix(self): return '.exe' if self.is_windows() else '' + def rust_triple(self): + """Returns the Rust target triple for this platform.""" + arch = rust_arch() + + if self.is_linux(): + return f"{arch}-unknown-linux-gnu" + elif self.is_darwin(): + return f"{arch}-apple-darwin" + elif self.is_msvc(): + return f"{arch}-pc-windows-msvc" + elif self.is_mingw() or self.is_msys(): + return f"{arch}-pc-windows-gnu" + else: + raise NotImplementedError(f"Unsupported Rust target platform: {self._platform}") + class ArgumentsList: """Helper class to accumulate ArgumentParser argument definitions @@ -399,6 +443,7 @@ 'crate_dir': ninja.source_file('src/gn/starlark'), 'target_dir': 'starlark', 'cxxflags': ' '.join(cflags), + 'ldflags': ' '.join(ldflags), } ninja.CargoLibTarget( library_to_a('gn_starlark'), @@ -1030,7 +1075,9 @@ libs.extend([ 'advapi32.lib', 'dbghelp.lib', + 'gdi32.lib', 'kernel32.lib', + 'ntdll.lib', 'ole32.lib', 'shell32.lib', 'user32.lib', @@ -1044,7 +1091,9 @@ libs.extend([ '-ladvapi32', '-ldbghelp', + '-lgdi32', '-lkernel32', + '-lntdll', '-lole32', '-lshell32', '-luser32',
diff --git a/build/ninja_file.py b/build/ninja_file.py index 58e0e10..f7fcb6c 100644 --- a/build/ninja_file.py +++ b/build/ninja_file.py
@@ -148,8 +148,8 @@ ninja_file=self, command=python( run_cargo_rel_path, - '$target_type $out $cargo_out_dir $cxx "$cxxflags"' - ' cargo build --color=always' + '$target_type $out $cargo_out_dir $cxx "$cxxflags" "$ldflags" $target_triple $ld' + ' cargo build --color=always --target=$target_triple' ' --manifest-path=$manifest_path $cargo_target_dir $cargo_flags' + ('' if self.debug else ' --release'), ), description='CARGO build $out', @@ -174,6 +174,7 @@ return 'debug' if self.debug else 'release' def CargoLibTarget(self, name, *, crate_dir, target_dir, cargo_flags='', **kwargs): + target_triple = self.platform.rust_triple() return self.Cargo( name, inputs=self.directory(crate_dir, ['target', 'testdata']), @@ -181,12 +182,14 @@ cargo_target_dir=f'--target-dir={target_dir}', cargo_flags=cargo_flags + ' --lib', target_type='lib', - cargo_out_dir=f'{target_dir}/{self.rust_profile}', + target_triple=target_triple, + cargo_out_dir=f'{target_dir}/{target_triple}/{self.rust_profile}', depfile=f'{name}.d', **kwargs, ) def CargoTestTarget(self, name, *, crate_dir, target_dir, cargo_flags='', **kwargs): + target_triple = self.platform.rust_triple() return self.Cargo( name, inputs=self.directory(crate_dir, ['target']), @@ -194,7 +197,8 @@ cargo_target_dir=f'--target-dir={target_dir}', cargo_flags=cargo_flags + ' --tests', target_type='test', - cargo_out_dir=f'{target_dir}/{self.rust_profile}', + target_triple=target_triple, + cargo_out_dir=f'{target_dir}/{target_triple}/{self.rust_profile}', depfile=f'{name}.d', **kwargs, )
diff --git a/build/run_cargo.py b/build/run_cargo.py index 6c9cb2c..a5ba52a 100755 --- a/build/run_cargo.py +++ b/build/run_cargo.py
@@ -17,12 +17,14 @@ build directory. """ +import contextlib import os from pathlib import Path import re import shutil import subprocess import sys +import tempfile _ESC = '\u001e' @@ -93,23 +95,54 @@ def process_test_target(out_path: Path, cargo_out_dir: Path) -> list[Path]: - """Generates a test runner script and returns all test binary source depfiles.""" + """Generates a test runner script. + + Returns all test binary source depfiles. + """ # When cargo builds tests, it builds one test binary per crate. # So we find all those test binaries, then make the generated "test binary" # just a script that invokes each of those binaries one by one. + + # Note that the layout of the output directory of cargo has changed. Thus, we + # must match both: + # Old layout: deps/<crate>-<hash>(.exe) + # New layout: build/<crate>/<hash>/out/<crate>-<hash>(.exe) + layout_pattern = re.compile( + r'^(?:' + r'deps' + r'|' + r'build/[^/]+/[0-9a-f]{16}/out' + r')/(?P<crate_name>[a-zA-Z0-9_-]+)-[0-9a-f]{16}(?:\.exe)?$' + ) + groups = {} - for c in (cargo_out_dir / 'deps').iterdir(): - is_executable = c.suffix.lower() == '.exe' if sys.platform == 'win32' else os.access(c, os.X_OK) - if c.is_file() and is_executable and c.suffix.lower() not in ('.so', '.dylib', '.dll'): - parts = c.name.split('-') + for root, _, files in os.walk(cargo_out_dir): + root_path = Path(root) + for name in files: + c = root_path / name + rel_path = c.relative_to(cargo_out_dir).as_posix() + m = layout_pattern.search(rel_path) + if not m: + continue + + # Ensure it is actually an executable test binary + if ( + c.suffix.lower() != '.exe' + if sys.platform == 'win32' + else not os.access(c, os.X_OK) + ): + continue + + crate_name = m.group('crate_name') # Cargo can cache your test binary under different configurations. - # Eg. The test binary might be called `mytest-hash1`, then after updating your lockfile, - # it might keep that binary but future tests would be called `mytest-hash2`. - # When this happens, we should use the newest one. - if len(parts) >= 2: - crate_name = '-'.join(parts[:-1]) - if crate_name not in groups or c.stat().st_mtime > groups[crate_name].stat().st_mtime: - groups[crate_name] = c + # E.g. the test binary might be called `mytest-hash1`, then after + # updating your lockfile, it might keep that binary but future tests + # would be called `mytest-hash2`. When this happens, we should use the + # newest one. + existing = groups.get(crate_name) + if not existing or c.stat().st_mtime > existing.stat().st_mtime: + groups[crate_name] = c + newest_binaries = list(groups.values()) out_path.parent.mkdir(parents=True, exist_ok=True) @@ -148,30 +181,103 @@ def main(): - if len(sys.argv) < 7: + if len(sys.argv) < 10: print( - 'Usage: run_cargo.py <test|lib> <out> <cargo_out_dir> <cxx> <cxxflags> <command...>', + 'Usage: run_cargo.py <test|lib> <out> <cargo_out_dir> <cxx> ' + '<cxxflags> <ldflags> <target_triple> <ld> <command...>', file=sys.stderr, ) sys.exit(1) - target_type, out_path_str, cargo_out_dir_str, cxx, cxxflags, *cmd_args = sys.argv[1:] + ( + target_type, + out_path_str, + cargo_out_dir_str, + cxx, + cxxflags, + ldflags, + target_triple, + linker, + *cmd_args, + ) = sys.argv[1:] out_path = Path(out_path_str) cargo_out_dir = Path(cargo_out_dir_str) os.environ['CXX'] = cxx os.environ['CXXFLAGS'] = cxxflags - # Since Ninja runs commands from the build output directory, CWD is the ninja out dir. + # Since Ninja runs commands from the build output directory, CWD is the + # ninja out dir. ninja_out_dir = os.getcwd() os.environ['NINJA_OUT_DIR'] = ninja_out_dir - os.environ['RUSTFLAGS'] = f"-L {ninja_out_dir}" + if sys.platform == 'win32': + # Linker flags in GN can contain relative paths (e.g. /MANIFESTINPUT) + # which are relative to the Ninja output directory. Since Cargo runs the + # linker from different build directories, we must normalize these paths + # to be absolute. + def replace_path(match): + prefix = match.group('prefix') + path_val = Path(match.group('path')) + if not path_val.is_absolute(): + abs_path = os.path.normpath(Path(ninja_out_dir) / path_val) + return f"{prefix}{abs_path}" + return match.group(0) + + pattern = re.compile( + r'(?i)(?P<prefix>[-/](?:manifestinput|natvis|pdb|def|implib|libpath):)' + r'(?P<path>[^\s]+)' + ) + ldflags = pattern.sub(replace_path, ldflags) + + target_rustflags = [f"-C linker={linker}", f"-L {ninja_out_dir}"] + [ + f"-C link-arg={flag}" for flag in ldflags.split() + ] + + if sys.platform == 'win32': + # GN compiles C++ code on Windows with the static C runtime (/MT or /MTd) by + # default. We must configure rustc to also link statically to prevent linker + # runtime library mismatch errors (LNK2038). + target_rustflags.append("-C target-feature=+crt-static") + + # Optimize the ffi crate in debug mode to strip unused cxx shims and + # resolve MSVC link failures. + os.environ['CARGO_PROFILE_DEV_PACKAGE_FFI_OPT_LEVEL'] = '1' + + # Compiling the cxx crate requires symlink support. + # Cargo's internal Git client (libgit2) does not respect environment + # variables like GIT_CONFIG_COUNT. Instead, we override HOME/USERPROFILE + # to point to a temporary directory containing a .gitconfig with + # core.symlinks = true. + with contextlib.suppress(RuntimeError): + os.environ.setdefault('CARGO_HOME', str(Path.home() / '.cargo')) + + temp_home = tempfile.mkdtemp() + with open(os.path.join(temp_home, '.gitconfig'), 'w') as f: + f.write('[core]\n\tsymlinks = true\n') + + os.environ['USERPROFILE'] = temp_home + os.environ['HOME'] = temp_home + os.environ['HOMEDRIVE'] = '' + os.environ['HOMEPATH'] = '' + + # When linking C++ objects instrumented with ASan/UBSan, we must allow the + # linker to link its default libraries so it pulls in the sanitizer runtimes. + if '-fsanitize=' in ldflags: + target_rustflags.append("-C default-linker-libraries=yes") + + env_var = f"CARGO_TARGET_{target_triple.upper().replace('-', '_')}_RUSTFLAGS" + os.environ[env_var] = ' '.join(target_rustflags) + # The link flags are specifically for the target. When cross-compiling, the + # target and host platforms are the same, but we don't want the link flags + # when building build tools. + os.environ['CARGO_TARGET_APPLIES_TO_HOST'] = 'false' # Now we run the `cargo build` command res = subprocess.run(cmd_args) if res.returncode != 0: sys.exit(res.returncode) - # Cargo build doesn't output files in a format ninja can use. So we now need to convert them. + # Cargo build doesn't output files in a format ninja can use. So we now + # need to convert them. if target_type == 'lib': src_depfiles = process_lib_target(out_path, cargo_out_dir) elif target_type == 'test':
diff --git a/src/gn/starlark/crates/ffi/build.rs b/src/gn/starlark/crates/ffi/build.rs index ae666d1..b29213f 100644 --- a/src/gn/starlark/crates/ffi/build.rs +++ b/src/gn/starlark/crates/ffi/build.rs
@@ -5,5 +5,5 @@ include!("../build_helper.rs"); fn main() { - require_libs(&["base", "gn_lib"]); + require_libs(&["gn_lib", "base"]); }