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

Add a run-make test that checks our core::ffi::c_* types against Clang #133058

Open
tgross35 opened this issue Nov 15, 2024 · 6 comments
Open

Add a run-make test that checks our core::ffi::c_* types against Clang #133058

tgross35 opened this issue Nov 15, 2024 · 6 comments
Assignees
Labels
A-FFI Area: Foreign function interface (FFI) A-testsuite Area: The testsuite used to check the correctness of rustc E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@tgross35
Copy link
Contributor

tgross35 commented Nov 15, 2024

It would be good to have a test that our C interop types are always compatible with C (c_char, c_long, c_longlong all vary to some extent). This will more or less be what @taiki-e did at #129945, just rolled into a run-make test rather than a shell script.

The test will basically need to do the following:

  1. Loop through each target string in the output of rustc --print target-list

  2. Map the rust target to a LLVM target if they aren't the same

  3. Loop through a list of each c_* type available in core::ffi

  4. Query clang -E -dM -x c /dev/null -target LLVM_TARGET to get a list of builtin definitions. Of note will be something like the following (comments added by me)

    #define __CHAR_BIT__ 8 // c_char
    #define __CHAR_UNSIGNED__ 1 // char signedness
    
    #define __SIZEOF_DOUBLE__ 8 // c_double
    #define __SIZEOF_FLOAT__ 4 // c_float
    #define __SIZEOF_INT__ 4 // c_int
    #define __SIZEOF_LONG_DOUBLE__ 16 // c_longdouble
    #define __SIZEOF_LONG_LONG__ 8 // c_longlong
    #define __SIZEOF_LONG__ 4 // c_long
    #define __SIZEOF_POINTER__ 4 // *const c_void
    #define __SIZEOF_PTRDIFF_T__ 4 // c_ptrdiff_t
    #define __SIZEOF_SHORT__ 2 // c_short
    #define __SIZEOF_SIZE_T__ 4 // c_size_t
    
    #define __BOOL_WIDTH__ 8 // bool
    #define __INTPTR_WIDTH__ 32 // isize
    #define __INT_WIDTH__ 32 // c_int
    #define __LLONG_WIDTH__ 64 // c_longlong
    #define __LONG_WIDTH__ 32 // c_long
    #define __POINTER_WIDTH__ 32 // *const c_void
    #define __PTRDIFF_WIDTH__ 32 // c_ptrdiff_t
    #define __SHRT_WIDTH__ 16 // c_short
    #define __SIZE_WIDTH__ 32 // c_size_t
    #define __UINTPTR_WIDTH__ 32 // usize
  5. Use the above to construct a simple Rust program that can verify the sizes and (when applicable) signedness line up at compile time. Probably do an assignment that will catch mismatched types plus a const assert for anything that doesn't have literals.

    // input to check `c_short`
    const C_SHORT: u{c_sizeof_short} = 0;
    const RUST_SHORT: core::ffi::c_short = C_SHORT;
    
    ```rust
    // input to check pointer width
    const _: () = assert!(size_of::<*const core::ffi::c_void>() * 8 == {c_pointer_width});
  6. Run rustc -Z no-codegen and check success/failure against a list of xfail targets.

run_make_support has clang available, example usage: https://github.com/rust-lang/rust/blob/e84902d35a4d3039c794e139eb12fba3624c5ff1/tests/run-make/cross-lang-lto-clang/rmake.rs. There is probably some way we could check this on MSVC targets too.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 15, 2024
@tgross35 tgross35 added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-libs Relevant to the library team, which will review and decide on the PR/issue. A-FFI Area: Foreign function interface (FFI) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 15, 2024
@tgross35
Copy link
Contributor Author

One tricky thing is that this would need to build-std for every target in CI, which is unfortunate. It might be better to have the test do something like copy core/ffi/mod.rs.html and preprocess to remove everything besides type aliases, then test against that rather than core::ffi (making the compiled file #![no_core]).

@jieyouxu jieyouxu added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it A-testsuite Area: The testsuite used to check the correctness of rustc and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Nov 15, 2024
@jieyouxu
Copy link
Member

Remark: I don't think we would want to -Z build-std for every target in CI because that sounds like it's going to take a while

@tgross35
Copy link
Contributor Author

Remark: I don't think we would want to -Z build-std for every target in CI because that sounds like it's going to take a while

Yeah, I realized that limitation after writing this. Just copying core::ffi to its own crate seems reasonable enough to avoid this since it's a pretty simple module - do we do this for any other test? (i.e. use the library by path rather than the built version).

@jieyouxu
Copy link
Member

Kind of? There's

cargo()
.args(&[
"build",
"--manifest-path",
"Cargo.toml",
"-Zbuild-std=core",
"--target",
&target(),
])
.env("RUSTFLAGS", "-Copt-level=0 -Cdebug-assertions=yes")
.env("CARGO_TARGET_DIR", &target_dir)
.env("RUSTC_BOOTSTRAP", "1")
// Visual Studio 2022 requires that the LIB env var be set so it can
// find the Windows SDK.
.env("LIB", std::env::var("LIB").unwrap_or_default())
.run();
let rlibs_path = target_dir.join(target()).join("debug").join("deps");
let compiler_builtins_rlib = read_dir(rlibs_path)
.find_map(|e| {
let path = e.unwrap().path();
let file_name = path.file_name().unwrap().to_str().unwrap();
if file_name.starts_with("libcompiler_builtins") && file_name.ends_with(".rlib") {
Some(path)
} else {
None
}
})
.unwrap();
which kinda already does -Z build-std=core which makes the test slow. The problem is I'm not sure we can trivially share the cache for that if the tests have different requirements.

@tgross35
Copy link
Contributor Author

tgross35 commented Nov 15, 2024

I think we could totally skip build-std but still get all targets tested. You could have the test duplicate library/core/src/ffi/mod.rs to a temporary directory and remove:

  • Every (pub )?use line
  • impl fmt::Debug ...
  • extern "C" {}

And wind up with a standalone file that works with #![no_core] as long as cfg_if is provided somewhere, so we can test it on all targets quickly without rebuilding core or even doing any codegen (it's just checking the result of the cfg at that point).

Fixing up ffi/mod.rs would be the most unreliable bit here, but that module doesn't change too much (and I guess we could move things to ffi/primitives.rs to make things easier if it winds up being a problem in the future).

@ricci009
Copy link

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-FFI Area: Foreign function interface (FFI) A-testsuite Area: The testsuite used to check the correctness of rustc E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants