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

Migrate incr-add-rust-src-component and issue-84395-lto-embed-bitcode run-make tests to rmake #128562

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/tools/tidy/src/allowed_run_make_makefiles.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ run-make/dep-info-spaces/Makefile
run-make/dep-info/Makefile
run-make/emit-to-stdout/Makefile
run-make/extern-fn-reachable/Makefile
run-make/incr-add-rust-src-component/Makefile
run-make/issue-84395-lto-embed-bitcode/Makefile
run-make/jobserver-error/Makefile
run-make/libs-through-symlinks/Makefile
run-make/libtest-json/Makefile
Expand Down
45 changes: 0 additions & 45 deletions tests/run-make/incr-add-rust-src-component/Makefile

This file was deleted.

39 changes: 39 additions & 0 deletions tests/run-make/incr-add-rust-src-component/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// rust-lang/rust#70924: Test that if we add rust-src component in between
// two incremental compiles, the compiler does not ICE on the second.
// Remove the rust-src part of the sysroot for the *first* build.
// Then put in a copy of the rust-src
// component for the second build, in order to expose the ICE from issue #70924.
// See https://github.com/rust-lang/rust/pull/72952

//@ needs-symlink

//FIXME(Oneirical): try on test-various
Copy link
Member

Choose a reason for hiding this comment

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

Remark (for myself): test-various

Also needs to check cross-compile.


use run_make_support::{path, rfs, rustc};

fn main() {
let sysroot = rustc().print("sysroot").run().stdout_utf8();
let sysroot = sysroot.trim();
rfs::create_dir("fakeroot");
symlink_all_entries(&sysroot, "fakeroot");
rfs::remove_file("fakeroot/lib");
Comment on lines +17 to +19
Copy link
Member

@jieyouxu jieyouxu Aug 16, 2024

Choose a reason for hiding this comment

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

Problem [REV1 (2/2)]: unfortunately, there's a whole rabbit hole for symlinks, with different tunnel branches for each platform. The following description is only true for your bog-standard symbolic link ("soft links" that is unofficially called to distinguish against hard links), not junction points, nor hard links.

On Windows

But young padawan (myself included), the API contract for CreateSymlinkW on Windows says:

To remove a symbolic link, delete the file (using DeleteFile or similar APIs) or remove the directory (using RemoveDirectory or similar APIs) depending on what type of symbolic link is used.

Those are the underlying Win32 APIs called by std::fs::remove_file and std::fs::remove_dir respectively.

If we don't know the kind of symlink on Windows beforehand, we can check it with std::os::windows::fs::FileTypeExt with is_symlink_dir and is_symlink_file. Thank you so much @ChrisDenton for pointing this out to me!

This remove_file and several below it is wrong (on Windows) because it's trying to call DeleteFileW on a directory.

On Linux / macOS

But on Linux/macOS, we need to use std::fs::remove_file because that corresponds to unlink on Unix, and there's no distinction for unlink with regard to symbolic links in terms of if the symbolic links point to files or directories.


Ah but wait, there's another caveat. If you want to interrogate the metadata of the symlink itself, you must use std::fs::symlink_metadata and not std::fs::metadata which will follow through symlinks to the linked directory/file.

std::fs::FileType: The underlying Metadata struct needs to be retrieved with the fs::symlink_metadata function and not the fs::metadata function. The fs::metadata function follows symbolic links, so is_symlink would always return false for the target file.

Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: we probably want to implement our very own remove_symlink helper that handles these specialness.

rfs::create_dir("fakeroot/lib");
symlink_all_entries(path(&sysroot).join("lib"), "fakeroot/lib");
rfs::remove_file("fakeroot/lib/rustlib");
rfs::create_dir("fakeroot/lib/rustlib");
symlink_all_entries(path(&sysroot).join("lib/rustlib"), "fakeroot/lib/rustlib");
rfs::remove_file("fakeroot/lib/rustlib/src");
rfs::create_dir("fakeroot/lib/rustlib/src");
symlink_all_entries(path(&sysroot).join("lib/rustlib/src"), "fakeroot/lib/rustlib/src");
rfs::remove_file("fakeroot/lib/rustlib/src/rust");
rustc().sysroot("fakeroot").incremental("incr").input("main.rs").run();
rfs::create_dir_all("fakeroot/lib/rustlib/src/rust/src/libstd");
rfs::create_file("fakeroot/lib/rustlib/src/rust/src/libstd/lib.rs");
rustc().sysroot("fakeroot").incremental("incr").input("main.rs").run();
}

fn symlink_all_entries<P: AsRef<std::path::Path>>(dir: P, fakepath: &str) {
for found_path in rfs::shallow_find_dir_entries(dir) {
rfs::create_symlink(&found_path, path(fakepath).join(found_path.file_name().unwrap()));
}
}
Comment on lines +35 to +39
Copy link
Member

Choose a reason for hiding this comment

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

Problem [REV1 (1/2)]: recall that rfs::create_symlink on Windows actually diverges into two function calls conditioned on if found_path is a file or if it's a directory:

14 changes: 0 additions & 14 deletions tests/run-make/issue-84395-lto-embed-bitcode/Makefile

This file was deleted.

30 changes: 30 additions & 0 deletions tests/run-make/lto-embed-bitcode-clang/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// This test checks that the embed bitcode in elf created with
// lto-embed-bitcode=optimized is a valid llvm bitcode module.
// Otherwise, the `test.bc` file will cause an error when
// `llvm-dis` attempts to disassemble it.
// See https://github.com/rust-lang/rust/issues/84395

//@ needs-force-clang-based-tests
// NOTE(#126180): This test only runs on `x86_64-gnu-debug`, because that CI job sets
// RUSTBUILD_FORCE_CLANG_BASED_TESTS and only runs tests which contain "clang" in their
// name.
Comment on lines +7 to +10
Copy link
Member

Choose a reason for hiding this comment

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

Remark (for myself): x86_64-gnu-debug


use run_make_support::llvm::llvm_bin_dir;
use run_make_support::{cmd, env_var, rustc};

fn main() {
rustc()
.input("test.rs")
.link_arg("-fuse-ld=lld")
.arg("-Clinker-plugin-lto")
.linker(&env_var("CLANG"))
.link_arg("-Wl,--plugin-opt=-lto-embed-bitcode=optimized")
.arg("-Zemit-thin-lto=no")
.run();
cmd(llvm_bin_dir().join("llvm-objcopy"))
.arg("--dump-section")
.arg(".llvmbc=test.bc")
.arg("test")
.run();
cmd(llvm_bin_dir().join("llvm-dis")).arg("test.bc").run();
}
Loading