Run-make test to check `core::ffi::c_*` types against clang #133944

ricci009 · 2024-12-06T01:32:58Z

Hello,

I have been working on issue #133058 for a bit of time now and would love some feedback as I seem to be stuck and not sure if I am approaching this correctly.

I currently have the following setup:

Get the rust target list
Use rust target to query the llvm target
Get clang definitions through querying the clang command with llvm target. I only save the necessary defines. Here is an example of the saved info (code can easily be modified to store Width as well):

Target: riscv64-unknown-linux-musl
CHAR_BIT = 8
CHAR_UNSIGNED = 1
SIZEOF_DOUBLE = 8
SIZEOF_INT = 4
SIZEOF_LONG = 8
SIZEOF_PTRDIFF_T = 8
SIZEOF_SIZE_T = 8
SIZEOF_FLOAT = 4
SIZEOF_LONG_LONG = 8
SIZEOF_SHORT = 2
Target: riscv64-unknown-fuchsia
CHAR_UNSIGNED = 1
SIZEOF_SHORT = 2
CHAR_BIT = 8
SIZEOF_INT = 4
SIZEOF_SIZE_T = 8
SIZEOF_FLOAT = 4
SIZEOF_LONG = 8
SIZEOF_DOUBLE = 8
SIZEOF_PTRDIFF_T = 8
SIZEOF_LONG_LONG = 8

I then save this into a hash map with the following format: <LLVM TARGET, <DEFINE NAME, DEFINE VALUE>>
Ex: <x86_64-unknown-linux-gnu, <SIZEOF_INT, 4>>

This is where it gets a bit shaky as I have been brainstorming ways to get the available c types in core::ffi to verify the size of the defined types but do not think I have the expertise to do this.

For the current implementation I specifically focus on the c_char type (unsigned vs signed). The test is currently failing as there are type mismatches which are expected (issue #129945 highlights this). I just do not know how to continue executing tests even with the type mismatches as it creates an error when running the run-make test. Or maybe I am doing something wrong in generating the test? Not too sure but would love your input. Thanks

r? @tgross35 @jieyouxu

try-job: x86_64-gnu-debug

rustbot · 2024-12-06T01:33:07Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

workingjubilee · 2024-12-06T01:51:19Z

tests/run-make/core-ffi-typecheck/rmake.rs

+    ("c_char", "__CHAR_BIT__"), // Character width
+    ("char_signedness", "__CHAR_UNSIGNED__"),
+    ("c_double", "__SIZEOF_DOUBLE__"), // Double precision floating-point
+    ("c_float", "__SIZEOF_FLOAT__"),   // Single precision floating-point
+    ("c_int", "__SIZEOF_INT__"),       // Signed integer
+    ("c_long", "__SIZEOF_LONG__"),     // Signed long integer
+    ("c_longlong", "__SIZEOF_LONG_LONG__"), // Signed long long integer
+    ("c_schar", "__SIZEOF_CHAR__"),    // Signed char
+    ("c_short", "__SIZEOF_SHORT__"),   // Signed short integer
+    ("c_uchar", "__SIZEOF_CHAR__"),    // Unsigned char
+    ("c_uint", "__SIZEOF_INT__"),      // Unsigned integer
+    ("c_ulong", "__SIZEOF_LONG__"),    // Unsigned long integer
+    ("c_ulonglong", "__SIZEOF_LONG_LONG__"), // Unsigned long long integer
+    ("c_ushort", "__SIZEOF_SHORT__"),  // Unsigned short integer
+    ("c_size_t", "__SIZEOF_SIZE_T__"), // Size type
+    ("c_ptrdiff_t", "__SIZEOF_PTRDIFF_T__"), // Pointer difference type];


We should either prefer the standard macros of CHAR_WIDTH and so on where available (which will be in bits instead of bytes, but eh, just hit it with >> 3), or simply use the actual sizeof operator within the generated C code.

jyn514 · 2024-12-06T06:21:06Z

This is where it gets a bit shaky as I have been brainstorming ways to get the available c types in core::ffi to verify the size of the defined types but do not think I have the expertise to do this.

try a program like this:

    println!("{}", std::mem::size_of::<core::ffi::c_long>())

(and so on for the other types.) From there you can parse them into a map like you have for LLVM.

to get a list of the available types you could use something like grep 'type c_[^ ]*' library/core/src/ffi/mod.rs -o | cut -d' ' -f2 | sort -u. on my machine that prints

c_char
c_int
c_long
c_ptrdiff_t
c_size_t
c_ssize_t
c_uint
c_ulong

tgross35 · 2024-12-09T06:20:34Z

Oh hey, thanks for working on this!

There are two interesting bits here:

There is value in running this for all targets, not just the ones that get native tests run. We can't reasonably build core because of the time that takes.
Because of this, we can't actually check what core::ffi::... looks like

What I mentioned in #133058 (comment) works, I was able to prototype this out relatively easily.

Solution to #1

Rust has the internal no_core feature which lets you skip building core and bring only the features you need. It's pretty weird to work with since you need to redefine every little thing you want to use (we provide minicore.rs to help with some of that), but it's pretty useful for running tests. Here, we need to hook a few additional lang items and intrinsics:

// smallcore.rs (within this run-make directory)
#![allow(internal_features)]
#![feature(decl_macro)]
#![feature(intrinsics)]
#![feature(lang_items)]
#![feature(link_cfg)]
#![feature(no_core)]
#![feature(rustc_attrs)]

#![no_core]
#![no_std]

// This is in `tests/auxiliary/minicore.rs`
extern crate minicore;
use minicore::Sized;

// Copy the body from https://github.com/rust-lang/rust/blob/df5b8e39b7def660696340b199ae395a869b3064/library/core/src/internal_macros.rs#L149
macro_rules! cfg_if { /* ... */ }

macro_rules! panic {
    ($msg:literal) => { $crate::panic(&$msg) };
}

/* Intrinsics are function signatures that `rustc` does something magic with.
 * The body doesn't matter. */

#[rustc_intrinsic]
#[rustc_intrinsic_const_stable_indirect]
#[rustc_intrinsic_must_be_overridden]
pub const fn size_of<T>() -> usize {
    loop {}
}

#[rustc_intrinsic]
#[rustc_intrinsic_must_be_overridden]
pub const fn abort() -> ! {
    loop {}
}

/* Lang items are similar to intrinsics but aren't limited to function signatures */

// This is what `panic!()` usually eventually expands to
#[lang = "panic"]
#[rustc_const_panic_str]
const fn panic(_expr: &&'static str) -> ! {
    abort();
}

/* We need to reimplement some basic traits so rustc knows what to do with `==` and `!` */

#[lang = "eq"]
pub trait PartialEq<Rhs: ?Sized = Self> {
    fn eq(&self, other: &Rhs) -> bool;
    fn ne(&self, other: &Rhs) -> bool {
        !self.eq(other)
    }
}

#[lang = "not"]
pub trait Not {
    type Output;
    fn not(self) -> Self::Output;
}

impl PartialEq for usize {
    fn eq(&self, other: &usize) -> bool {
        (*self) == (*other)
    }
}

impl Not for bool {
    type Output = bool;
    fn not(self) -> Self {
        !self
    }
}

(@jieyouxu any interest in just putting some of this in minicore?)

Solution to #2

We can't build core so that means we can't just access the types via core::ffi. This module is simple enough that it can be cleaned up into something that doesn't depend on the rest of core, I just quickly with perl for experimentation but you can just use regex from run_make_support.

# Start with core/src/ffi copied to a temporary directory

# Remove stability attrbutes since they can't be used outside of std
perl -i -0777 -pe 's/#!?\[(un)?stable.*?\]//gs' ffi/mod.rs

# Remove doc attributes since we'll be removing some items they apply to
perl -i -0777 -pe 's/#\[doc.*?\]//gs' ffi/mod.rs

# Remove lang item indicators so they don't conflict if `minicore` gets updated
perl -i -0777 -pe 's/#\[lang.*?\]//gs' ffi/mod.rs

# Remove non-inline modules since we don't need `cstr` or `va_list`
perl -i -0777 -pe 's/.*mod.*;//g' ffi/mod.rs

# Remove reexports that go with those modules
perl -i -0777 -pe 's/.*use.*;//g' ffi/mod.rs

# Remove the `Debug` implementation for `c_void`. This is done in two steps
# to avoid regex tripping over `{`/`}`
perl -i -0777 -pe 's/fn fmt.*?\{.*?\}//gs' ffi/mod.rs
perl -i -0777 -pe 's/impl fmt::Debug for.*?\{.*?\}//gs' ffi/mod.rs

# Just for sanity sake when looking at the generated files, eliminate repeated empty lines
perl -i -0777 -pe 's/\n{3,}/\n\n/gs' ffi/mod.rs

What might actually be better is to move the types we care about into a new mod types, then only the stability attributes need to be stripped (edit: yeah, you should just do this rather than copying my regex hacks).

Tests

Write a test file that asserts each size at compile time, using what we have in smallcore for something like assert_eq! (you could also add an assert_eq macro but 🤷)

// tests.rs

use super::*; // `super` will include everything from `smallcore` once glued together

pub const TEST_C_CHAR_SIZE: () = if size_of::<ffi::c_char>() != CLANG_C_CHAR_SIZE {
    panic!("wrong c_char size");
};

pub const TEST_C_LONG_SIZE: () = if size_of::<ffi::c_long>() != CLANG_C_LONG_SIZE {
    panic!("wrong c_long size");
};

Glue

This is the only target-specific part: each target needs to generate its own file where you fill in the CLANG_* variables based on whatever you collected from Clang.

/* boilerplate part */
#![allow(unused)]

// include! rather than `mod` with `path` since this one has crate-level
// attributes
include!("path/to/smallcore.rs");

// Path to the simplified FFI file
#[path = "path/to/ffi.rs"]
mod ffi;

#[path = "path/to/tests.rs"]
mod tests;

/* end boilerplate */

/* Per-target: append to the template based on Clang's output */

const CLANG_C_CHAR_SIZE: usize = 1;
const CLANG_C_LONG_SIZE: usize = 8;
// ...

Then just have rustc build the file for the relevant target; if our sizes are wrong, we'll get a failure at const eval. You can even pass -Zno-codegen to make this faster (the rustc equivalent of cargo c).

jieyouxu · 2024-12-09T06:29:28Z

I'll take a look later today, thanks for the detailed writeup.

tgross35 · 2024-12-09T06:32:39Z

For checking the sign, you could do add something like this to tests.rs:

trait Signed {
    const SIGNED: bool;
}

impl Signed for i8 {
    const SIGNED: bool = true;
}

impl Signed for u8 {
    const SIGNED: bool = false;
}

const TEST_C_CHAR_SIGNED: () = if ffi::c_char::SIGNED ^ CLANG_C_CHAR_SIGNED {
    panic("mismatched c_char sign");
};

This requires adding a BitXor trait and impl (like Not/PartialEq)

tgross35 · 2024-12-09T06:38:09Z

I'll take a look later today, thanks for the detailed writeup.

Thanks! This is less nice than having core available ofc, but this builds a no_core crate for all targets in about a minute on my computer, and it seems reasonably robust.

Most fragile thing is probably the text processing of ffi/mod.rs, but I think we could move everything relevant to ffi/types.rs and just leave a note there not to import anything from crate.

tgross35 · 2024-12-09T06:53:01Z

One other thought: this PR shouldn't fix anything that doesn't currently line up, so there should be xfails. It might be easiest to just do this in tests.rs:

// tests that always pass don't need a cfg_if

cfg_if! {
    if #[cfg(target_os = "foobar")] {
        // FIXME: long isn't long enough on foobar
        const XFAIL_C_LONG_SIZE: usize = 16;
        pub const TEST_C_LONG_SIZE: () = if size_of::<ffi::c_long>() != XFAIL_C_LONG_SIZE {
            panic("wrong c_long size");
        };
    } else {
        // Default test
        pub const TEST_C_LONG_SIZE: () = if size_of::<ffi::c_long>() != CLANG_C_LONG_SIZE {
            panic("wrong c_long size");
        };
    }
}

ricci009 · 2024-12-10T00:15:24Z

Thanks for the write up, I appreciate it! Will read it and look to understand it tonight :).

Just a quick question that might be off topic but I created the following function for testing purposes:

pub fn create_type_sizes() -> std::collections::HashMap<String, String> {
    let mut sizes = std::collections::HashMap::new();
    sizes.insert("c_char".to_string(), std::mem::size_of::<core::ffi::c_char>().to_string());
    sizes.insert("c_int".to_string(), std::mem::size_of::<core::ffi::c_int>().to_string());
    sizes.insert("c_long".to_string(), std::mem::size_of::<core::ffi::c_long>().to_string());
    sizes.insert("c_ptrdiff_t".to_string(), std::mem::size_of::<core::ffi::c_ptrdiff_t>().to_string());
    sizes.insert("c_size_t".to_string(), std::mem::size_of::<core::ffi::c_size_t>().to_string());
    sizes.insert("c_ssize_t".to_string(), std::mem::size_of::<core::ffi::c_ssize_t>().to_string());
    sizes.insert("c_uint".to_string(), std::mem::size_of::<core::ffi::c_uint>().to_string());
    sizes.insert("c_ulong".to_string(), std::mem::size_of::<core::ffi::c_ulong>().to_string());
    sizes
}

I wanted to save the sizes for the c types for the current target but I get 3 errors for the following types:

c_ptrdiff_t
c_size_t
c_ssize_t

The error is as follows:

use of unstable library feature c_size_t.

The open issue is issue #88345

My concern is that in the tests we are using size_of::ffi::TYPE(). If we input the type as c_size_t or the other 2 we will get an error. Should this effect the run-make test at all or is it fine?

Edit: I am assuming adding #![feature(c_size_t)] allows the run-make test not to be affected by this.

ricci009 · 2024-12-10T01:28:10Z

disregard this commit, gonna be following what you outlined for the solution.

jieyouxu · 2024-12-10T02:20:55Z

I was eepy yesterday, going to revisit this in a moment today. I'll need to revisit what the test requirements are.

tgross35 · 2024-12-10T03:59:18Z

Edit: I am assuming adding #![feature(c_size_t)] allows the run-make test not to be affected by this.

Just for reference, this would be correct if importing from core. It shouldn't be relevant though with the no_core solution since the file has to get copied out of core and have the stability attributes stripped.

jieyouxu · 2024-12-10T09:41:22Z

Solution to #1

Rust has the internal no_core feature which lets you skip building core and bring only the features you need. It's pretty weird to work with since you need to redefine every little thing you want to use (we provide minicore.rs to help with some of that), but it's pretty useful for running tests. Here, we need to hook a few additional lang items and intrinsics:

[... trimmde ...]

(@jieyouxu any interest in just putting some of this in minicore?)

Yes, if these items are only in core (not alloc, not std-only), please do add them to tests/auxiliary/minicore.rs.

Solution to #2

We can't build core so that means we can't just access the types via core::ffi. This module is simple enough that it can be cleaned up into something that doesn't depend on the rest of core, I just quickly with perl for experimentation but you can just use regex from run_make_support.

[... trimmed ...]

What might actually be better is to move the types we care about into a new mod types, then only the stability attributes need to be stripped (edit: yeah, you should just do this rather than copying my regex hacks).

Moving to mod types; sounds less fragile to me, and if the resulting test can properly exercise mod types; through proper name resolution + type checking etc., then please do that instead of regex hacks which will necessarily be fragile.

Glue

This is the only target-specific part: each target needs to generate its own file where you fill in the CLANG_* variables based on whatever you collected from Clang.
/* boilerplate part */
#![allow(unused)]

// include! rather than `mod` with `path` since this one has crate-level
// attributes
include!("path/to/smallcore.rs");

// Path to the simplified FFI file
#[path = "path/to/ffi.rs"]
mod ffi;

#[path = "path/to/tests.rs"]
mod tests;

/* end boilerplate */

/* Per-target: append to the template based on Clang's output */

const CLANG_C_CHAR_SIZE: usize = 1;
const CLANG_C_LONG_SIZE: usize = 8;
// ...
Then just have rustc build the file for the relevant target; if our sizes are wrong, we'll get a failure at const eval. You can even pass -Zno-codegen to make this faster (the rustc equivalent of cargo c).

I don't actually know what -Z no-codegen does, but --emit=metadata is what the check-pass mode does.

tgross35 · 2024-12-27T10:20:42Z

@rustbot author

@ricci009 just let us know if you get stuck (absolutely no pressure if it's just a time thing)

ricci009 · 2024-12-27T15:58:27Z

@tgross35
Hi, sorry I just got swarmed over this month with holiday and work stuff. I am still working on it and will let you know if there are any issues or I get stuck. Thanks for reaching out :)

ricci009 · 2025-01-12T06:21:22Z

@tgross35
I believe I am building the target file incorrectly.

let rustc_output = rustc() .arg("-Z") .arg("unstable-options") .arg("--target") .arg(target) .arg(&file_name) .run();

this is how I attempt to build it but I receive this error:

error[E0463]: can't find crate for std

I have tried to fix this problem through defining #![no_std] in the target file but that does not seem right and does not work.

Smallcore is still the same as what you outlined and I created a mod.rs file like you explained.

tgross35 · 2025-01-12T06:47:46Z

I have tried to fix this problem through defining #![no_std] in the target file but that does not seem right and does not work.

That file also needs #![feature(no_core)] and #[no_core]. I'll take a closer look at the rest soon to see if it looks right.

Also as @jieyouxu mentioned above, you can add whatever is missing to tests/auxiliary/minicore.rs and include that rather than creating a new file smallcore.rs (sorry my advice was a bit outdated), so that may make some things simpler.

ricci009 · 2025-01-30T23:19:06Z

Couldn't have done it without you. Huge thank you for your support :) .

tgross35

Two minor comment requests, and squash please, but this looks great to me! Thank you for keeping at this for so long.

I'd like Jieyou to take a look as well as the rmake expert, which should happen in a few more days. And that gives enough time for the core::ffi refactoring to happen first, if you're able to do that.

tests/run-make/core-ffi-typecheck-clang/rmake.rs

tgross35 · 2025-01-30T23:16:36Z

tests/run-make/core-ffi-typecheck-clang/rmake.rs

+const MAPPED_TARGETS: &[(&str, &str)] = &[
+    ("armv5te-unknown-linux-uclibcgnueabi", "armv5te-unknown-linux"),
+    ("mips-unknown-linux-uclibc", "mips-unknown-linux"),
+    ("mipsel-unknown-linux-uclibc", "mips-unknown-linux"),
+    ("powerpc-unknown-linux-gnuspe", "powerpc-unknown-linux-gnu"),
+    ("powerpc-unknown-linux-muslspe", "powerpc-unknown-linux-musl"),
+    ("x86_64-unknown-l4re-uclibc", "x86_64-unknown-l4re"),
+];


Are these targets where we have the wrong LLVM string and should fix that, or where LLVM is different from Clang? A note would be good.

The first val in the pair is a viable llvm target but it is not supported on clang. Hence I just map it to the "default" version that is supported on clang.

For reference:

❯ rustc -Z unstable-options --target=mipsel-unknown-linux-uclibc --print target-spec-json { "arch": "mips", ... "llvm-target": "mipsel-unknown-linux-uclibc", ... "os": "linux", ... }

clang attempt to build:

clang: error: version 'uclibc' in target triple 'mipsel-unknown-linux-uclibc' is invalid Compiler returned: 1

I remove 'uclibc' from the target and proceed to compile with clang.

A follow up on this is what should the non-working llvm target map to? Is it viable to assume I can map it to mips-unknown-linux?

I think this is fine, the libc used shouldn't change the primitive size. A comment mentioning that they work for LLVM but not for clang is sufficient 👍

Suggestion: indeed, please document why this mapping is needed (and what purpose it serves)

ricci009 · 2025-01-30T23:36:21Z

I'd like Jieyou to take a look as well as the rmake expert, which should happen in a few more days. And that gives enough time for the core::ffi refactoring to happen first, if you're able to do that.

Yup gonna start working on this ASAP, I hope to be done with it before Jieyou takes a look at this.

the regex preprocessing would be more robust if the relevant types from core/src/ffi/mod.rs were first moved to library/core/src/ffi/primitives.rs, then there isn't a need to deal with traits / c_str / va_list / whatever might wind up in that module in the future.

Added what the PR should be for reference.

jieyouxu

Thanks for working on this. I left some preliminary feedback (I'll do another pass after the core/ffi changes).

jieyouxu · 2025-01-31T07:53:10Z

tests/auxiliary/minicore.rs

+/// Vendored from the 'cfg_if' crate
+
+macro_rules! cfg_if {
+    // match if/else chains with a final `else`
+    (
+        $(
+            if #[cfg( $i_meta:meta )] { $( $i_tokens:tt )* }
+        ) else+
+        else { $( $e_tokens:tt )* }
+    ) => {
+        cfg_if! {
+            @__items () ;
+            $(
+                (( $i_meta ) ( $( $i_tokens )* )) ,
+            )+
+            (() ( $( $e_tokens )* )) ,
+        }
+    };
+
+    // Internal and recursive macro to emit all the items
+    //
+    // Collects all the previous cfgs in a list at the beginning, so they can be
+    // negated. After the semicolon is all the remaining items.
+    (@__items ( $( $_:meta , )* ) ; ) => {};
+    (
+        @__items ( $( $no:meta , )* ) ;
+        (( $( $yes:meta )? ) ( $( $tokens:tt )* )) ,
+        $( $rest:tt , )*
+    ) => {
+        // Emit all items within one block, applying an appropriate #[cfg]. The
+        // #[cfg] will require all `$yes` matchers specified and must also negate
+        // all previous matchers.
+        #[cfg(all(
+            $( $yes , )?
+            not(any( $( $no ),* ))
+        ))]
+        cfg_if! { @__identity $( $tokens )* }
+
+        // Recurse to emit all other items in `$rest`, and when we do so add all
+        // our `$yes` matchers to the list of `$no` matchers as future emissions
+        // will have to negate everything we just matched as well.
+        cfg_if! {
+            @__items ( $( $no , )* $( $yes , )? ) ;
+            $( $rest , )*
+        }
+    };
+
+    // Internal macro to make __apply work out right for different match types,
+    // because of how macros match/expand stuff.
+    (@__identity $( $tokens:tt )* ) => {
+        $( $tokens )*
+    };
+}


Problem: this isn't actually a core macro. If you want cfg_if, can you actually just depend on cfg_if-the-crate in src/tools/run-make-support, then re-export cfg_if via run-make-support? A run-make test will have access to run-make-support. See how object or regex are re-exported in run-make-support.

Oh I see, you actually want cfg_if for the test file. In that case, can you just put cfg_if in the test file, and not in minicore.rs?

Unfortunately cfg_if is needed for processed_mod.rs. What I could do to remove it from minicore.rs (although this is really ugly code) is put it into the rmake_content string.

let mut rmake_content = format!( r#" #![no_std] #![no_core] #![feature(link_cfg)] #![allow(unused)] #![crate_type = "rlib"] /* begin minicore content */ {minicore_content} /* end minicore content */ /// Vendored from the 'cfg_if' crate macro_rules! cfg_if {{ // match if/else chains with a final `else` ( $( if #[cfg( $i_meta:meta )] {{ $( $i_tokens:tt )* }} ) else+ else {{ $( $e_tokens:tt )* }} ) => {{ cfg_if! {{ @__items () ; $( (( $i_meta ) ( $( $i_tokens )* )) , )+ (() ( $( $e_tokens )* )) , }} }}; // Internal and recursive macro to emit all the items // // Collects all the previous cfgs in a list at the beginning, so they can be // negated. After the semicolon is all the remaining items. (@__items ( $( $_:meta , )* ) ; ) => {{}}; ( @__items ( $( $no:meta , )* ) ; (( $( $yes:meta )? ) ( $( $tokens:tt )* )) , $( $rest:tt , )* ) => {{ // Emit all items within one block, applying an appropriate #[cfg]. The // #[cfg] will require all `$yes` matchers specified and must also negate // all previous matchers. #[cfg(all( $( $yes , )? not(any( $( $no ),* )) ))] cfg_if! {{ @__identity $( $tokens )* }} // Recurse to emit all other items in `$rest`, and when we do so add all // our `$yes` matchers to the list of `$no` matchers as future emissions // will have to negate everything we just matched as well. cfg_if! {{ @__items ( $( $no , )* $( $yes , )? ) ; $( $rest , )* }} }}; // Internal macro to make __apply work out right for different match types, // because of how macros match/expand stuff. (@__identity $( $tokens:tt )* ) => {{ $( $tokens )* }}; }} #[path = "processed_mod.rs"] mod ffi; #[path = "tests.rs"] mod tests; "# );

jieyouxu · 2025-01-31T07:56:34Z

tests/auxiliary/minicore.rs

+#[lang = "panic"]
+#[rustc_const_panic_str]
+const fn panic(_expr: &&'static str) -> ! {
+    abort();
+}


Problem [PANIC 1/2]: AFAICT, this signature is wrong (this signature has one layer of reference too many), because:

rust/library/core/src/panicking.rs

Lines 128 to 133 in c37fbd8

#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never), cold)]

#[cfg_attr(feature = "panic_immediate_abort", inline)]

#[track_caller]

#[rustc_const_stable_indirect] // must follow stable const rules since it is exposed to stable

#[lang = "panic"] // used by lints and miri for panics

pub const fn panic(expr: &'static str) -> ! {

jieyouxu · 2025-01-31T07:56:56Z

tests/auxiliary/minicore.rs

+#[macro_export]
+macro_rules! panic {
+    ($msg:literal) => {
+        $crate::panic(&$msg)
+    };
+}


Problem [PANIC 2/2]: ... which is why this has an extra & on $msg.

jieyouxu · 2025-01-31T07:57:50Z

tests/auxiliary/minicore.rs

+}
+
+#[lang = "panic"]
+#[rustc_const_panic_str]


Problem: I think we may need to apply #[inline]/#[inline(never)]/#[inline(always)] and #[rustc_nounwind] and match core's usage, because that can influence codegen. Note that panic inline attreibutes are gated behind the panic_immediate_abort cfg, e.g.

https://github.com/rust-lang/rust/blob/c37fbd873a15e7cdc92476f7d7b964f6c05e64cd/library/core/src/panicking.rs#L81C1-L82C55

I am a bit confused on this to be honest. Would I just apply #[inline(always)] to allow for a panic_immediate_abort? Or do I literally just add all the features you highlighted above the panic function like this for it to behave correctly:

#[lang = "panic"] #[rustc_const_panic_str] #[cfg_attr(not(feature = "panic_immediate_abort"), inline(never), cold)] #[cfg_attr(feature = "panic_immediate_abort", inline)] #[track_caller] #[rustc_nounwind] const fn panic(_expr: &'static str) -> ! { abort() }

For the time being, I would make sure that the following attrs are included to minimize the difference versus the actual decl:

#[lang = "panic"]

#[rustc_const_panic_str]

#[inline(never)]

#[cold]

#[track_caller]

#[rustc_nounwind]

to line up the attributes. I wouldn't yet bother with panic_immediate_abort because minicore via //@ add-core-stubs isn't built with panic_immediate_abort.

In the long run, it would be very nice if we didn't need a minicore and just use a core API-layer thing, but that's a moonshot so.

jieyouxu · 2025-01-31T08:00:09Z

tests/auxiliary/minicore.rs

+impl PartialEq for usize {
+    fn eq(&self, other: &usize) -> bool {
+        (*self) == (*other)
+    }
+}


Suggestion: I think you could use a macro to construct these impl PartialEqs so as long as the shape remains compatible since I think that's also what core does.

tests/run-make/core-ffi-typecheck-clang/rmake.rs

jieyouxu · 2025-01-31T08:03:49Z

tests/run-make/core-ffi-typecheck-clang/rmake.rs

+const MAPPED_TARGETS: &[(&str, &str)] = &[
+    ("armv5te-unknown-linux-uclibcgnueabi", "armv5te-unknown-linux"),
+    ("mips-unknown-linux-uclibc", "mips-unknown-linux"),
+    ("mipsel-unknown-linux-uclibc", "mips-unknown-linux"),
+    ("powerpc-unknown-linux-gnuspe", "powerpc-unknown-linux-gnu"),
+    ("powerpc-unknown-linux-muslspe", "powerpc-unknown-linux-musl"),
+    ("x86_64-unknown-l4re-uclibc", "x86_64-unknown-l4re"),
+];


Suggestion: indeed, please document why this mapping is needed (and what purpose it serves)

tests/run-make/core-ffi-typecheck-clang/rmake.rs

Extract `core::ffi` primitives to a separate (internal) module ### Introduce library/core/src/ffi/primitives.rs The regex preprocessing for PR rust-lang#133944 would be more robust if the relevant types from core/src/ffi/mod.rs were first moved to library/core/src/ffi/primitives.rs, then there isn't a need to deal with traits / c_str / va_list / whatever might wind up in that module in the future r? `@tgross35`

Rollup merge of rust-lang#136334 - ricci009:primitivers, r=tgross35 Extract `core::ffi` primitives to a separate (internal) module ### Introduce library/core/src/ffi/primitives.rs The regex preprocessing for PR rust-lang#133944 would be more robust if the relevant types from core/src/ffi/mod.rs were first moved to library/core/src/ffi/primitives.rs, then there isn't a need to deal with traits / c_str / va_list / whatever might wind up in that module in the future r? `@tgross35`

tgross35 · 2025-02-13T17:31:52Z

I'm not sure if you are ready or not but just comment @rustbot ready to update the labels when you are (no rush of course)

ricci009 · 2025-02-17T01:02:05Z

@rustbot ready

I am not sure if I implemented the panic macro correctly. I found it needed panic_fmt for the const evaluated panics. On top of this, adding the additional needed attributes is causing a bunch of the tests to fail and I am unsure why.

The additional attributes in question:

#[lang = "panic"]
//#[rustc_const_panic_str]
//#[inline(never)]
//#[cold]
//#[track_caller]
//#[rustc_nounwind]
const fn panic(expr: &'static str) -> ! {
    abort()
}

Also added the partialEq as a macro into minicore.rs.

For some reason, I had to add "amdgcn-amd-amdhsa" as a skipped target as on the most up-to-date pull of the master branch, it was for some reason testing this target. Not sure if "-nogpulib" is needed if I just directly skip the GPU target.

bors · 2025-02-17T14:51:42Z

☔ The latest upstream changes (presumably #137164) made this pull request unmergeable. Please resolve the merge conflicts.

jieyouxu · 2025-02-19T10:28:12Z

I'll take a look ~tmrw.

jieyouxu

I left some more feedback.

For the approach, I'd like another compiler reviewer to take a look for a vibe-check. I'm slightly concerned that the current approach might be (too) fragile. Maybe cc @oli-obk or @workingjubilee or @saethlin?

jieyouxu · 2025-02-23T17:36:22Z

tests/run-make/core-ffi-typecheck-clang/rmake.rs

+        rfs::remove_file(&file_name);
+        if !rustc_output.status().success() {
+            panic!("Failed for target {}", target);
+        }


Suggestion:

Suggested change

rfs::remove_file(&file_name);

if !rustc_output.status().success() {

panic!("Failed for target {}", target);

}

Don't remove the per-target rmake file to preserve the output artifacts. Between test runs (or on explicit clean), the whole test out directory gets removed.

rustc().run() already asserts successful output and will print the rustc stderr if it fails. If you want to see which target it failed on, I think you might leave a println!("checking target {target}...") before actually running rustc() on the per-target rmake.rs.

jieyouxu · 2025-02-23T17:37:41Z

tests/run-make/core-ffi-typecheck-clang/rmake.rs

+            #![no_std]
+            #![no_core]


Suggestion:

Suggested change

#![no_std]

#![no_core]

these are redundant. minicore must already be #![no_std] and #![no_core].

jieyouxu · 2025-02-23T17:44:19Z

tests/auxiliary/minicore.rs

@@ -17,6 +17,7 @@
 #![feature(no_core, lang_items, rustc_attrs, decl_macro, naked_functions, f16, f128)]
 #![allow(unused, improper_ctypes_definitions, internal_features)]
 #![feature(asm_experimental_arch)]
+#![feature(intrinsics)]


Problem: this merge-conflicts with the latest master, can you collapse this feature with the feature list present on master?

jieyouxu · 2025-02-23T17:47:18Z

tests/auxiliary/minicore.rs

+
+#[rustc_intrinsic]
+#[rustc_intrinsic_const_stable_indirect]
+#[rustc_intrinsic_must_be_overridden]


Suggestion: can you remove the instances of #[rustc_intrinsic_must_be_overridden] cc #137489

jieyouxu · 2025-02-23T17:47:43Z

tests/auxiliary/minicore.rs

+//impl PartialEq for usize {
+//    fn eq(&self, other: &usize) -> bool {
+//        (*self) == (*other)
+//    }
+//}
+//
+//impl PartialEq for bool {
+//    fn eq(&self, other: &bool) -> bool {
+//        (*self) == (*other)
+//    }
+//}


Question: commented out, duplicated by partialEq! above?

jieyouxu · 2025-02-23T17:52:55Z

tests/auxiliary/minicore.rs

 #[rustc_builtin_macro]
 #[macro_export]
 macro_rules! stringify {
    ($($t:tt)*) => {
        /* compiler built-in */
    };
 }
+
+#[macro_export]
+macro_rules! partialEq {


Nit: can you call this derive_partial_eq or partialeq_impl or sth

jieyouxu · 2025-02-23T18:07:29Z

tests/auxiliary/minicore.rs

+#[lang = "eq"]
+pub trait PartialEq<Rhs: ?Sized = Self> {


Suggestion: please move the trait decl before the $trait_impl! macro and macro invocation.

jieyouxu · 2025-02-23T18:10:44Z

tests/run-make/core-ffi-typecheck-clang/rmake.rs

+
+    let file_name = "processed_mod.rs";
+
+    rfs::write(&file_name, content);


Suggestion: write a line of #![allow(non_camel_case_types)] before the stripped primitives.rs content to trim down amount of warnings if the test do fail.

jieyouxu · 2025-02-23T18:27:11Z

tests/auxiliary/minicore.rs

+#[lang = "panic"]
+//#[rustc_const_panic_str]
+//#[inline(never)]
+//#[cold]
+//#[track_caller]
+//#[rustc_nounwind]
+const fn panic(expr: &'static str) -> ! {
+    abort()
+}


Problem: let's pick not(feature = "panic_immediate_abort") configuration, and to match core (as best as possible)

rust/library/core/src/panicking.rs

Lines 128 to 133 in b522e7c

#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never), cold)]

#[cfg_attr(feature = "panic_immediate_abort", inline)]

#[track_caller]

#[rustc_const_stable_indirect] // must follow stable const rules since it is exposed to stable

#[lang = "panic"] // used by lints and miri for panics

pub const fn panic(expr: &'static str) -> ! {

#[inline(never)] #[cold] #[track_caller] #[lang = "panic"] pub const fn panic(expr: &'static str) -> ! {

jieyouxu · 2025-02-23T18:29:05Z

tests/auxiliary/minicore.rs

+#[lang = "panic_fmt"]
+const fn panic_fmt(fmt: &str) -> ! {
+    abort()
+}


Problem: this doesn't match the real panic_fmt's signature:

rust/library/core/src/panicking.rs

Lines 49 to 55 in b522e7c

#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never), cold)]

#[cfg_attr(feature = "panic_immediate_abort", inline)]

#[track_caller]

#[lang = "panic_fmt"] // needed for const-evaluated panics

#[rustc_do_not_const_check] // hooked by const-eval

#[rustc_const_stable_indirect] // must follow stable const rules since it is exposed to stable

pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {

#[inline(never)] #[cold] #[track_caller] #[lang = "panic_fmt"] #[rustc_do_not_const_check] pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {

I'm somewhat concerned about panic signatures divering between real core and minicore...

cc @oli-obk do you know if this is a problem?

Not really. None of the other flags are really relevant to the caller or the language

I'm surprised we don't have a check that the signature is correct. Getting the wrong signature is likely to lead to an ICE or miscompile in the future. For example, codegen generates calls to these lang items assuming that their signature is correct.

Please bring minicore in line with the lang item implementation in core proper.

Also opened #137531

saethlin · 2025-02-23T22:33:47Z

This PR is doing simple textual manipulation of Rust code in the standard library and C code from clang. This seems like a really flaky approach to me, I don't want random library contributors patching up this test because they broke it while updating the ffi module. Has any alternative to that been explored?

rustbot assigned tgross35 Dec 6, 2024

rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 6, 2024

rustbot assigned jieyouxu and unassigned tgross35 Dec 6, 2024

workingjubilee reviewed Dec 6, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

tgross35 mentioned this pull request Dec 10, 2024

De-duplicate and improve definition of core::ffi::c_char #132975

Merged

tgross35 self-assigned this Dec 22, 2024

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 27, 2024

This comment has been minimized.

Sign in to view

tgross35 approved these changes Jan 30, 2025

View reviewed changes

ricci009 mentioned this pull request Jan 31, 2025

Extract core::ffi primitives to a separate (internal) module #136334

Merged

jieyouxu requested changes Jan 31, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2025

rustbot added the has-merge-commits PR has merge commits, merge with caution. label Feb 12, 2025

This comment has been minimized.

Sign in to view

ricci009 force-pushed the master branch from 4f3fd82 to eacd23e Compare February 12, 2025 03:27

rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Feb 12, 2025

preliminary changes - panic & macro partequal need fixing

91e970f

ricci009 force-pushed the master branch from eacd23e to 91e970f Compare February 12, 2025 03:32

This comment has been minimized.

Sign in to view

errors in panic tags

461c183

panic reimplementation & partialEq macro

f42ac3b

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 17, 2025

This comment has been minimized.

Sign in to view

issue with attributes on panic

a2a1d60

jieyouxu requested changes Feb 23, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2025

	#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never), cold)]
	#[cfg_attr(feature = "panic_immediate_abort", inline)]
	#[track_caller]
	#[rustc_const_stable_indirect] // must follow stable const rules since it is exposed to stable
	#[lang = "panic"] // used by lints and miri for panics
	pub const fn panic(expr: &'static str) -> ! {


		let file_name = "processed_mod.rs";

		rfs::write(&file_name, content);

Run-make test to check core::ffi::c_* types against clang #133944

Are you sure you want to change the base?

Run-make test to check core::ffi::c_* types against clang #133944

Conversation

ricci009 commented Dec 6, 2024 • edited by tgross35 Loading

rustbot commented Dec 6, 2024

Choose a reason for hiding this comment

This comment has been minimized.

jyn514 commented Dec 6, 2024 • edited Loading

tgross35 commented Dec 9, 2024 • edited Loading

Solution to #1

Solution to #2

Tests

Glue

jieyouxu commented Dec 9, 2024

tgross35 commented Dec 9, 2024 • edited Loading

tgross35 commented Dec 9, 2024 • edited Loading

tgross35 commented Dec 9, 2024

ricci009 commented Dec 10, 2024 • edited Loading

ricci009 commented Dec 10, 2024 • edited Loading

This comment has been minimized.

jieyouxu commented Dec 10, 2024 • edited Loading

tgross35 commented Dec 10, 2024

jieyouxu commented Dec 10, 2024

Solution to #1

Solution to #2

Glue

tgross35 commented Dec 27, 2024 • edited Loading

ricci009 commented Dec 27, 2024

ricci009 commented Jan 12, 2025 • edited Loading

This comment has been minimized.

tgross35 commented Jan 12, 2025 • edited Loading

ricci009 commented Jan 30, 2025

tgross35 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricci009 Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricci009 commented Jan 30, 2025

jieyouxu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

tgross35 commented Feb 13, 2025

ricci009 commented Feb 17, 2025 • edited Loading

This comment has been minimized.

bors commented Feb 17, 2025

jieyouxu commented Feb 19, 2025

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin Feb 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin commented Feb 23, 2025 • edited Loading

Run-make test to check `core::ffi::c_*` types against clang #133944

Run-make test to check `core::ffi::c_*` types against clang #133944

ricci009 commented Dec 6, 2024 •

edited by tgross35

Loading

jyn514 commented Dec 6, 2024 •

edited

Loading

tgross35 commented Dec 9, 2024 •

edited

Loading

tgross35 commented Dec 9, 2024 •

edited

Loading

tgross35 commented Dec 9, 2024 •

edited

Loading

ricci009 commented Dec 10, 2024 •

edited

Loading

ricci009 commented Dec 10, 2024 •

edited

Loading

jieyouxu commented Dec 10, 2024 •

edited

Loading

tgross35 commented Dec 27, 2024 •

edited

Loading

ricci009 commented Jan 12, 2025 •

edited

Loading

tgross35 commented Jan 12, 2025 •

edited

Loading

ricci009 Jan 30, 2025 •

edited

Loading

jieyouxu Feb 10, 2025 •

edited

Loading

ricci009 commented Feb 17, 2025 •

edited

Loading

jieyouxu left a comment •

edited

Loading

saethlin Feb 23, 2025 •

edited

Loading

saethlin commented Feb 23, 2025 •

edited

Loading