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 test directive needs-target-has-atomic #133095

Closed
wants to merge 2 commits into from
Closed
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
16 changes: 15 additions & 1 deletion src/tools/compiletest/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ use std::ffi::OsString;
use std::path::{Path, PathBuf};
use std::process::Command;
use std::str::FromStr;
use std::sync::OnceLock;
use std::sync::{LazyLock, OnceLock};
use std::{fmt, iter};

use build_helper::git::GitConfig;
use regex::Regex;
use semver::Version;
use serde::de::{Deserialize, Deserializer, Error as _};
use test::{ColorConfig, OutputFormat};
Expand Down Expand Up @@ -477,6 +478,19 @@ impl Config {
ASM_SUPPORTED_ARCHS.contains(&self.target_cfg().arch.as_str())
}

pub fn has_atomic(&self, size: &str) -> bool {
static TARGET_HAS_ATOMIC: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r#"target_has_atomic="(?<size>[0-9a-zA-Z]+)""#).unwrap());

TARGET_HAS_ATOMIC
.captures_iter(&rustc_output(
self,
&["--print=cfg", "--target", &self.target],
Default::default(),
))
.any(|caps| &caps["size"] == size)
}

pub fn git_config(&self) -> GitConfig<'_> {
GitConfig {
git_repository: &self.git_repository,
Expand Down
1 change: 1 addition & 0 deletions src/tools/compiletest/src/directive-list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
"needs-sanitizer-thread",
"needs-std-debug-assertions",
"needs-symlink",
"needs-target-has-atomic",
"needs-threads",
"needs-unwind",
"needs-wasmtime",
Expand Down
50 changes: 47 additions & 3 deletions src/tools/compiletest/src/header/needs.rs
Copy link
Member

Choose a reason for hiding this comment

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

Problem: we should keep //@ needs-target-has-atomic matching the #[cfg(target_has_atomic)] semantics otherwise it adds needless confusion. In particular, this directive should support (cf. https://doc.rust-lang.org/reference/conditional-compilation.html#target_has_atomic):

//@ needs-target-has-atomic: 8
//@ needs-target-has-atomic: 16
//@ needs-target-has-atomic: 32
//@ needs-target-has-atomic: 64
//@ needs-target-has-atomic: 128
//@ needs-target-has-atomic: ptr

Possibly comma-separated so the test-writer can combine them, e.g.

//@ target-has-atomic: 8, 16, 32, 64, 128, ptr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we accept (none), i.e. below

//@ needs-target-has-atomic

like the cfg equivalent?

#[cfg(target_has_atomic)]

Copy link
Member

@jieyouxu jieyouxu Nov 21, 2024

Choose a reason for hiding this comment

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

No. Apparently target_has_atomic = "8" is not quite target_has_atomic; the latter is a nightly-only unstable cfg. For the purposes of these tests, I think we are only interested in the target_has_atomic="value" builtin cfgs. We can always slightly adjust this logic in a follow-up if needed.

Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,12 @@ pub(super) fn handle_needs(
},
];

let (name, comment) = match ln.split_once([':', ' ']) {
Some((name, comment)) => (name, Some(comment)),
None => (ln, None),
// Because `needs-target-has-atomic` accepts comma separated arguments following a colon to
// specify data sizes, we check whether comment starts with colon.
let (name, comment, comment_starts_with_colon) = if let Some(index) = ln.find([':', ' ']) {
(&ln[..index], Some(&ln[index + 1..]), ln.as_bytes()[index] == b':')
} else {
(ln, None, false)
};
Comment on lines +176 to 180
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: this also accepts a whitespace separator, please only accept : to help consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This accepts a whitespace separator, but only when the directive isn't needs-target-has-atomic. See here.
It should accept a whitespace separator when other than needs-target-has-atomic because it is currently accepted. Shouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

I'll double-check tmrw


if !name.starts_with("needs-") {
Expand All @@ -185,6 +188,11 @@ pub(super) fn handle_needs(
return IgnoreDecision::Continue;
}

// Check here because `needs-target-has-atomic` requires parsing comments.
if name == "needs-target-has-atomic" {
return handle_needs_target_has_atomic(comment_starts_with_colon, comment, config);
}

let mut found_valid = false;
for need in needs {
if need.name == name {
Expand All @@ -210,6 +218,42 @@ pub(super) fn handle_needs(
}
}

fn handle_needs_target_has_atomic(
comment_starts_with_colon: bool,
comment: Option<&str>,
config: &Config,
) -> IgnoreDecision {
// `needs-target-has-atomic` requires comma-separated data sizes following a collon.
if !comment_starts_with_colon || comment.is_none() {
return IgnoreDecision::Error {
message: "`needs-target-has-atomic` requires data sizes for atomic operations".into(),
};
}
let comment = comment.unwrap();

// Parse the comment to specify data sizes.
for size in comment.split(',').map(|size| size.trim()) {
if !["ptr", "128", "64", "32", "16", "8"].contains(&size) {
return IgnoreDecision::Error {
message: "expected values for `needs-target-has-atomic` are: `128`, `16`, \\
`32`, `64`, `8`, and `ptr`"
.into(),
};
}
if !config.has_atomic(size) {
return IgnoreDecision::Ignore {
reason: if size == "ptr" {
"ignored on targets without ptr-size atomic operations".into()
} else {
format!("ignored on targets without {size}-bit atomic operations")
},
};
}
}

IgnoreDecision::Continue
}

struct Need {
name: &'static str,
condition: bool,
Expand Down
25 changes: 25 additions & 0 deletions src/tools/compiletest/src/header/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,31 @@ fn threads_support() {
}
}

#[test]
fn target_has_atomic_support() {
let atomics = [
("aarch64-unknown-linux-gnu", &["8", "16", "32", "64", "128", "ptr"][..], &[][..]),
("aarch64-apple-darwin", &["8", "16", "32", "64", "128", "ptr"], &[]),
("i686-pc-windows-gnu", &["8", "16", "32", "64", "ptr"], &["128"]),
("i686-pc-windows-msvc", &["8", "16", "32", "64", "ptr"], &["128"]),
("i686-unknown-linux-gnu", &["8", "16", "32", "64", "ptr"], &["128"]),
("x86_64-apple-darwin", &["8", "16", "32", "64", "128", "ptr"], &[]),
("x86_64-pc-windows-gnu", &["8", "16", "32", "64", "128", "ptr"], &[]),
("x86_64-pc-windows-msvc", &["8", "16", "32", "64", "128", "ptr"], &[]),
("x86_64-unknown-linux-gnu", &["8", "16", "32", "64", "ptr"], &["128"]),
];

for (target, supported, not_supported) in atomics {
let config = cfg().target(target).build();
for size in supported {
assert!(config.has_atomic(size));
}
for size in not_supported {
assert!(!config.has_atomic(size));
}
}
}

fn run_path(poisoned: &mut bool, path: &Path, buf: &[u8]) {
let rdr = std::io::Cursor::new(&buf);
iter_header(Mode::Ui, "ui", poisoned, path, rdr, &mut |_| {});
Expand Down
Loading