Skip to content

Commit

Permalink
Add needs-target-has-atomic directive
Browse files Browse the repository at this point in the history
Before this commit, the test writer has to specify platforms and
architectures by hand for targets that have differing atomic width
support. `#[cfg(target_has_atomic)]` is not quite the same because (1)
you may have to specify additional matchers manually which has to be
maintained individually, and (2) the `#[cfg]` blocks does not
communicate to compiletest that a test would be ignored for a given
target.

This commit implements a `//@ needs-target-has-atomic` directive which
admits a comma-separated list of required atomic widths that the target
must satisfy in order for the test to run.

```
//@ needs-target-has-atomic: 8, 16, ptr
```

See <rust-lang#87377>.

Co-authored-by: kei519 <[email protected]>
  • Loading branch information
jieyouxu and kei519 committed Dec 2, 2024
1 parent a522d78 commit 99c2322
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 5 deletions.
22 changes: 21 additions & 1 deletion src/tools/compiletest/src/common.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::{HashMap, HashSet};
use std::collections::{BTreeSet, HashMap, HashSet};
use std::ffi::OsString;
use std::path::{Path, PathBuf};
use std::process::Command;
Expand Down Expand Up @@ -486,6 +486,9 @@ impl Config {
}
}

/// Known widths of `target_has_atomic`.
pub const KNOWN_TARGET_HAS_ATOMIC_WIDTHS: &[&str] = &["8", "16", "32", "64", "128", "ptr"];

#[derive(Debug, Clone)]
pub struct TargetCfgs {
pub current: TargetCfg,
Expand Down Expand Up @@ -611,6 +614,17 @@ impl TargetCfgs {
("panic", Some("abort")) => cfg.panic = PanicStrategy::Abort,
("panic", Some("unwind")) => cfg.panic = PanicStrategy::Unwind,
("panic", other) => panic!("unexpected value for panic cfg: {other:?}"),

("target_has_atomic", Some(width))
if KNOWN_TARGET_HAS_ATOMIC_WIDTHS.contains(&width) =>
{
cfg.target_has_atomic.insert(width.to_string());
}
("target_has_atomic", Some(other)) => {
panic!("unexpected value for `target_has_atomic` cfg: {other:?}")
}
// Nightly-only std-internal impl detail.
("target_has_atomic", None) => {}
_ => {}
}
}
Expand Down Expand Up @@ -645,6 +659,12 @@ pub struct TargetCfg {
pub(crate) xray: bool,
#[serde(default = "default_reloc_model")]
pub(crate) relocation_model: String,

// Not present in target cfg json output, additional derived information.
#[serde(skip)]
/// Supported target atomic widths: e.g. `8` to `128` or `ptr`. This is derived from the builtin
/// `target_has_atomic` `cfg`s e.g. `target_has_atomic="8"`.
pub(crate) target_has_atomic: BTreeSet<String>,
}

impl TargetCfg {
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
51 changes: 47 additions & 4 deletions src/tools/compiletest/src/header/needs.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::common::{Config, Sanitizer};
use crate::common::{Config, KNOWN_TARGET_HAS_ATOMIC_WIDTHS, Sanitizer};
use crate::header::{IgnoreDecision, llvm_has_libzstd};

pub(super) fn handle_needs(
Expand Down Expand Up @@ -171,11 +171,54 @@ pub(super) fn handle_needs(
},
];

let (name, comment) = match ln.split_once([':', ' ']) {
Some((name, comment)) => (name, Some(comment)),
let (name, rest) = match ln.split_once([':', ' ']) {
Some((name, rest)) => (name, Some(rest)),
None => (ln, None),
};

// FIXME(jieyouxu): tighten up this parsing to reject using both `:` and ` ` as means to
// delineate value.
if name == "needs-target-has-atomic" {
let Some(rest) = rest else {
return IgnoreDecision::Error {
message: "expected `needs-target-has-atomic` to have a comma-separated list of atomic widths".to_string(),
};
};

// Expect directive value to be a list of comma-separated atomic widths.
let specified_widths = rest
.split(',')
.map(|width| width.trim())
.map(ToString::to_string)
.collect::<Vec<String>>();

for width in &specified_widths {
if !KNOWN_TARGET_HAS_ATOMIC_WIDTHS.contains(&width.as_str()) {
return IgnoreDecision::Error {
message: format!(
"unknown width specified in `needs-target-has-atomic`: `{width}` is not a \
known `target_has_atomic_width`, known values are `{:?}`",
KNOWN_TARGET_HAS_ATOMIC_WIDTHS
),
};
}
}

let satisfies_all_specified_widths = specified_widths
.iter()
.all(|specified| config.target_cfg().target_has_atomic.contains(specified));
if satisfies_all_specified_widths {
return IgnoreDecision::Continue;
} else {
return IgnoreDecision::Ignore {
reason: format!(
"skipping test as target does not support all of the required `target_has_atomic` widths `{:?}`",
specified_widths
),
};
}
}

if !name.starts_with("needs-") {
return IgnoreDecision::Continue;
}
Expand All @@ -193,7 +236,7 @@ pub(super) fn handle_needs(
break;
} else {
return IgnoreDecision::Ignore {
reason: if let Some(comment) = comment {
reason: if let Some(comment) = rest {
format!("{} ({})", need.ignore_reason, comment.trim())
} else {
need.ignore_reason.into()
Expand Down
37 changes: 37 additions & 0 deletions src/tools/compiletest/src/header/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -787,3 +787,40 @@ fn test_not_trailing_directive() {
run_path(&mut poisoned, Path::new("a.rs"), b"//@ revisions: incremental");
assert!(!poisoned);
}

#[test]
fn test_needs_target_has_atomic() {
use std::collections::BTreeSet;

// `x86_64-unknown-linux-gnu` supports `["8", "16", "32", "64", "ptr"]` but not `128`.

let config = cfg().target("x86_64-unknown-linux-gnu").build();
// Expectation sanity check.
assert_eq!(
config.target_cfg().target_has_atomic,
BTreeSet::from([
"8".to_string(),
"16".to_string(),
"32".to_string(),
"64".to_string(),
"ptr".to_string()
]),
"expected `x86_64-unknown-linux-gnu` to not have 128-bit atomic support"
);

assert!(!check_ignore(&config, "//@ needs-target-has-atomic: 8"));
assert!(!check_ignore(&config, "//@ needs-target-has-atomic: 16"));
assert!(!check_ignore(&config, "//@ needs-target-has-atomic: 32"));
assert!(!check_ignore(&config, "//@ needs-target-has-atomic: 64"));
assert!(!check_ignore(&config, "//@ needs-target-has-atomic: ptr"));

assert!(check_ignore(&config, "//@ needs-target-has-atomic: 128"));

assert!(!check_ignore(&config, "//@ needs-target-has-atomic: 8,16,32,64,ptr"));

assert!(check_ignore(&config, "//@ needs-target-has-atomic: 8,16,32,64,ptr,128"));

// Check whitespace between widths is permitted.
assert!(!check_ignore(&config, "//@ needs-target-has-atomic: 8, ptr"));
assert!(check_ignore(&config, "//@ needs-target-has-atomic: 8, ptr, 128"));
}

0 comments on commit 99c2322

Please sign in to comment.