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

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

Open
wants to merge 4 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
92 changes: 92 additions & 0 deletions tests/auxiliary/minicore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Copy link
Member

Choose a reason for hiding this comment

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

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

#![no_std]
#![no_core]

Expand Down Expand Up @@ -101,10 +102,101 @@ macro_rules! concat {
/* compiler built-in */
};
}

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

#[macro_export]
macro_rules! partialEq {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you call this derive_partial_eq or partialeq_impl or sth

($($ty:ty),* $(,)?) => {
$(
impl PartialEq for $ty {
#[inline]
fn eq(&self, other: &$ty) -> bool {
(*self) == (*other)
}
#[inline]
fn ne(&self, other: &$ty) -> bool {
(*self) != (*other)
}
}
)*
}
}

partialEq!(
bool, char, isize, i8, i16, i32, i64, i128, usize, u8, u16, u32, u64, u128, f16, f32, f64, f128
);

#[lang = "panic"]
//#[rustc_const_panic_str]
//#[inline(never)]
//#[cold]
//#[track_caller]
//#[rustc_nounwind]
const fn panic(expr: &'static str) -> ! {
abort()
}
Comment on lines +136 to +144
Copy link
Member

Choose a reason for hiding this comment

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

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

#[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) -> ! {


#[lang = "panic_fmt"]
const fn panic_fmt(fmt: &str) -> ! {
abort()
}
Comment on lines +146 to +149
Copy link
Member

Choose a reason for hiding this comment

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

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

#[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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@saethlin saethlin Feb 23, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Also opened #137531


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

#[rustc_intrinsic]
#[rustc_intrinsic_const_stable_indirect]
#[rustc_intrinsic_must_be_overridden]
Copy link
Member

Choose a reason for hiding this comment

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

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

pub const fn size_of<T>() -> usize {
loop {}
}

#[rustc_intrinsic]
#[rustc_intrinsic_must_be_overridden]
pub const fn abort() -> ! {
loop {}
}
Comment on lines +165 to +169
Copy link
Member

Choose a reason for hiding this comment

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

Problem: you probably need #[rustc_nounwind]

#[rustc_nounwind]
#[rustc_intrinsic]
#[rustc_intrinsic_must_be_overridden]
pub fn abort() -> ! {
unreachable!()
}

#[rustc_nounwind]
#[rustc_intrinsic]
pub fn abort() -> ! {
    unreachable!()
}


#[lang = "eq"]
pub trait PartialEq<Rhs: ?Sized = Self> {
Comment on lines +171 to +172
Copy link
Member

Choose a reason for hiding this comment

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

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

fn eq(&self, other: &Rhs) -> bool;
fn ne(&self, other: &Rhs) -> bool {
!self.eq(other)
}
Comment on lines +174 to +176
Copy link
Member

Choose a reason for hiding this comment

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

Problem: you should add #[inline] on ne

/// Tests for `!=`. The default implementation is almost always sufficient,
/// and should not be overridden without very good reason.
#[inline]
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_diagnostic_item = "cmp_partialeq_ne"]
fn ne(&self, other: &Rhs) -> bool {
!self.eq(other)
}

}

//impl PartialEq for usize {
// fn eq(&self, other: &usize) -> bool {
// (*self) == (*other)
// }
//}
//
//impl PartialEq for bool {
// fn eq(&self, other: &bool) -> bool {
// (*self) == (*other)
// }
//}
Comment on lines +179 to +189
Copy link
Member

Choose a reason for hiding this comment

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

Question: commented out, duplicated by partialEq! above?


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

impl Not for bool {
type Output = bool;
fn not(self) -> Self {
!self
}
}
Comment on lines +197 to +202
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: you can steal

macro_rules! not_impl {
($($t:ty)*) => ($(
#[stable(feature = "rust1", since = "1.0.0")]
impl Not for $t {
type Output = $t;
#[inline]
fn not(self) -> $t { !self }
}
forward_ref_unop! { impl Not, not for $t }
)*)
}
not_impl! { bool usize u8 u16 u32 u64 u128 isize i8 i16 i32 i64 i128 }

without forward_ref_unop! for now.

but notably, the #[inline] on not matters.

235 changes: 235 additions & 0 deletions tests/run-make/core-ffi-typecheck-clang/rmake.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
//@ needs-force-clang-based-tests
Copy link
Member

Choose a reason for hiding this comment

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

Nit (style): can you move this directive below, and change the // comment block below to become a //! doc comment block?


//! This test checks that the clang defines for each target allign with the core ffi types
//! defined in `library/core/src/ffi/primitives.rs'.
//! Therefore each rust target is queried and the clang defines for each target
//! are read and compared to the core sizes to verify all types and sizes allign at buildtime.
//!
//! If this test fails because Rust adds a target that Clang does not support, this target should be
//! added to `SKIPPED_TARGETS`.

use run_make_support::{clang, regex, rfs, rustc, serde_json};
use serde_json::Value;

// It is not possible to run the Rust test-suite on these targets.
const SKIPPED_TARGETS: &[&str] = &[
// Clang doesn't have built-in support for the Xtensa architecture
"xtensa-esp32-espidf",
"xtensa-esp32-none-elf",
"xtensa-esp32s2-espidf",
"xtensa-esp32s2-none-elf",
"xtensa-esp32s3-espidf",
"xtensa-esp32s3-none-elf",
// Clang doesn't include built-in support for the C-SKY architecture
"csky-unknown-linux-gnuabiv2",
"csky-unknown-linux-gnuabiv2hf",
// GPU target with different memory model/type system - C type sizes don't map cleanly
"amdgcn-amd-amdhsa",
];

// These targets are mapped because clang doesn't recognize the left-side variants
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"),
];
Comment on lines +31 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ricci009 ricci009 Jan 30, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

Copy link
Member

Choose a reason for hiding this comment

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

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


fn main() {
let minicore_path = run_make_support::source_root().join("tests/auxiliary/minicore.rs");

preprocess_core_ffi();

// Print the list of target JSON files, which will be used to match Rust target strings to LLVM

let targets_json =
rustc().arg("--print").arg("all-target-specs-json").arg("-Z").arg("unstable-options").run();
let targets_json_str =
String::from_utf8(targets_json.stdout().to_vec()).expect("error not a string");

let j: Value = serde_json::from_str(&targets_json_str).unwrap();
for (target, v) in j.as_object().unwrap() {
let llvm_target = &v["llvm-target"].as_str().unwrap();

if SKIPPED_TARGETS.iter().any(|&to_skip_target| target == to_skip_target) {
continue;
}

// Create a new variable to hold either the mapped target or original llvm_target
let target_to_use = MAPPED_TARGETS
.iter()
.find(|&&(from, _)| from == *llvm_target)
.map(|&(_, to)| to)
.unwrap_or(llvm_target);

// Run Clang's preprocessor for the relevant target, printing default macro definitions.
let clang_output = clang()
.args(&["-nogpulib", "-E", "-dM", "-x", "c", "/dev/null", "-target", &target_to_use])
.run();

let defines = String::from_utf8(clang_output.stdout()).expect("Invalid UTF-8");

let minicore_content = rfs::read_to_string(&minicore_path);
let mut rmake_content = format!(
r#"
#![no_std]
#![no_core]
Comment on lines +77 to +78
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:

Suggested change
#![no_std]
#![no_core]

these are redundant. minicore must already be #![no_std] and #![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;
"#
);

rmake_content.push_str(&format!(
"
const CLANG_C_CHAR_SIZE: usize = {};
const CLANG_C_CHAR_SIGNED: bool = {};
const CLANG_C_SHORT_SIZE: usize = {};
const CLANG_C_INT_SIZE: usize = {};
const CLANG_C_LONG_SIZE: usize = {};
const CLANG_C_LONGLONG_SIZE: usize = {};
const CLANG_C_FLOAT_SIZE: usize = {};
const CLANG_C_DOUBLE_SIZE: usize = {};
const CLANG_C_SIZE_T_SIZE: usize = {};
const CLANG_C_PTRDIFF_T_SIZE: usize = {};
",
parse_size(&defines, "CHAR"),
char_is_signed(&defines),
parse_size(&defines, "SHORT"),
parse_size(&defines, "INT"),
parse_size(&defines, "LONG"),
parse_size(&defines, "LONG_LONG"),
parse_size(&defines, "FLOAT"),
parse_size(&defines, "DOUBLE"),
parse_size(&defines, "SIZE_T"),
parse_size(&defines, "PTRDIFF_T"),
));

// Generate a target-specific rmake file.
// If type misalignments occur,
// generated rmake file name used to identify the failing target.
let file_name = format!("{}_rmake.rs", target.replace("-", "_").replace(".", "_"));

// Attempt to build the test file for the relevant target.
// Tests use constant evaluation, so running is not necessary.
rfs::write(&file_name, rmake_content);
let rustc_output = rustc()
.arg("-Zunstable-options")
.arg("--emit=metadata")
.arg("--target")
.arg(target)
.arg("-o-")
.arg(&file_name)
.run();
rfs::remove_file(&file_name);
if !rustc_output.status().success() {
panic!("Failed for target {}", target);
}
Comment on lines +185 to +188
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:

Suggested change
rfs::remove_file(&file_name);
if !rustc_output.status().success() {
panic!("Failed for target {}", target);
}
  1. 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.
  2. 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.

}
}

// Helper to parse size from clang defines
fn parse_size(defines: &str, type_name: &str) -> usize {
let search_pattern = format!("__SIZEOF_{}__ ", type_name.to_uppercase());
for line in defines.lines() {
if line.contains(&search_pattern) {
if let Some(size_str) = line.split_whitespace().last() {
return size_str.parse().unwrap_or(0);
}
}
}

// Only allow CHAR to default to 1
if type_name.to_uppercase() == "CHAR" {
return 1;
}

panic!("Could not find size definition for type: {}", type_name);
}

fn char_is_signed(defines: &str) -> bool {
!defines.lines().any(|line| line.contains("__CHAR_UNSIGNED__"))
}

/// Parse core/ffi/mod.rs to retrieve only necessary macros and type defines
fn preprocess_core_ffi() {
let mod_path = run_make_support::source_root().join("library/core/src/ffi/primitives.rs");
let mut content = rfs::read_to_string(&mod_path);

//remove stability features #![unstable]
let mut re = regex::Regex::new(r"#!?\[(un)?stable[^]]*?\]").unwrap();
content = re.replace_all(&content, "").to_string();

//remove doc features #[doc...]
re = regex::Regex::new(r"#\[doc[^]]*?\]").unwrap();
content = re.replace_all(&content, "").to_string();

//remove non inline modules
re = regex::Regex::new(r".*mod.*;").unwrap();
content = re.replace_all(&content, "").to_string();
Comment on lines +229 to +230
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be needed anymore right? Since primitives.rs now has no child modules.


let file_name = "processed_mod.rs";

rfs::write(&file_name, content);
Copy link
Member

Choose a reason for hiding this comment

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

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.

}
Loading
Loading