Skip to content

Commit

Permalink
Rollup merge of rust-lang#133736 - jieyouxu:needs-target-has-atomic, …
Browse files Browse the repository at this point in the history
…r=compiler-errors

Add `needs-target-has-atomic` directive

Before this PR, 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 PR 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>.

This PR supersedes rust-lang#133095 and is co-authored by `@kei519,` because it was somewhat subtle, and it turned out easier to implement than to review.

rustc-dev-guide docs PR: rust-lang/rustc-dev-guide#2154
  • Loading branch information
GuillaumeGomez authored Dec 2, 2024
2 parents 8f5a801 + 99c2322 commit 80e0448
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 @@ -153,6 +153,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 80e0448

Please sign in to comment.