Add test directive `needs-target-has-atomic` by kei519 · Pull Request #133095 · rust-lang/rust

kei519 · 2024-11-16T06:22:37Z

Add needs-atomic test directive to reduce specifying architectures that have atomic operations in test (fix #87377).

This directive requires comma separated operation sizes:

//@ needs-target-has-atomic: 32
//@ needs-target-has-atomic: 8, ptr

rustbot · 2024-11-16T06:22:45Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (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

rustbot · 2024-11-16T06:22:48Z

Some changes occurred in src/tools/compiletest

cc @jieyouxu

compiler-errors · 2024-11-16T20:15:24Z

Also, is there a typo in the description? I would assume it's //@ needs-atomic: ptr, not //@ needs-atomic ptr.

kei519 · 2024-11-16T20:40:39Z

No. My code accepts both //@ needs-atomic: ptr and //@ needs-atomic ptr as the same. Should it accept only the former one?

jieyouxu · 2024-11-16T20:49:43Z

I'll leave more feedback tmrw, but pls only accept the colon version

kei519 · 2024-11-17T06:30:58Z

Fixed it.

jieyouxu

Thanks for the PR!

We should call this directive //@ needs-target-has-atomic to match the cfg(target_has_atomic) and accept the same values, and we should not accept comments on this directive or have any kind of otherwise fancy parsing because they become a footgun in actual usage.

I left some feedback about issues with the target-has-atomic target-specific info detection and the directive parsing itself. Also we should introduce a new self-test and we should not be modifying that other rustc cli ui test which explicitly is trying to ban setting the builtin cfg by the user.

jieyouxu · 2024-11-20T09:34:56Z

tests/ui/cfg/disallowed-cli-cfgs.rs

Problem: this test must not be modified, this is a compiler ui test checking rustc forbids the user setting a builtin-cfg via cli.

See https://github.com/rust-lang/rust/blob/70e814bd9e532a302763f870c665c5af59c2b632/tests/ui/cfg/disallowed-cli-cfgs.target_has_atomic_.stderr

Instead, this can be a new tests/ui/compiletest-self-test/only-target-has-atomic that cherry-picks a well-known tier 1 arch / target like x86_64-unknown-gnu-linux that is very unlikely to change its target_has_atomic values.

I misunderstood.
However jyn514 says

you have to specify the platforms and architectures you want by hand

on #87377, but aren't there any tests codes including has-atomic constraint written by hand?

Yes, but also no. You can always do the #[cfg(target_has_atomic = "ptr")] by hand, however, that is not known to compiletest (compiletest only looks at //@ directives), so if you do that you can't communicate to compiletest if the test is actually ignored instead of vacuously passed (because #[cfg(blah)] panic!() won't trigger unless there's a blah revision).

jieyouxu · 2024-11-20T09:39:23Z

src/tools/compiletest/src/header/needs.rs

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

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

//@ needs-target-has-atomic

like the cfg equivalent?

#[cfg(target_has_atomic)]

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.

jieyouxu · 2024-11-20T09:40:03Z

src/tools/compiletest/src/header/needs.rs

+    // Because `needs-atomic` accepts one argument after colon to specify data size, we check whther
+    // 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)


Problem: let's not accept comments on this directive because it makes the parsing very complicated. If needed, the test writer can just use a regular comment on a previous/following line.

src/tools/compiletest/src/common.rs

jieyouxu · 2024-11-20T09:50:46Z

src/tools/compiletest/src/common.rs

+    pub fn has_atomic(&self, size: Option<u64>) -> bool {
+        if let Some(size) = size {
+            (self.target_cfg().min_atomic_width()..=self.target_cfg().max_atomic_width())
+                .contains(&size)
+                && self.target_cfg().atomic_cas
+        } else {
+            self.target_cfg().atomic_cas
+        }
+    }


Problem [DETECTION 2/2]: ditto.

jieyouxu · 2024-11-20T09:56:23Z

@rustbot author

kei519 · 2024-11-27T07:21:03Z

has_atomic() was fixed to use --print=cfg output.

And I added a new test, but I'm not sure whether the test is enough.

@rustbot review

jieyouxu

Thanks, I have a few more nits, but this looks good!

jieyouxu · 2024-11-27T16:50:44Z

src/tools/compiletest/src/common.rs

+    pub fn has_atomic(&self, size: &str) -> bool {
+        for config in
+            rustc_output(self, &["--print=cfg", "--target", &self.target], Default::default())
+                .trim()
+                .lines()
+        {
+            let Some((key, value)) = config.split_once("=\"").map(|(k, v)| {
+                (k, v.strip_suffix('"').expect("key-value pair should be properly quoted"))
+            }) else {
+                continue;
+            };
+            if key == "target_has_atomic" && value == size {
+                return true;
+            }
+        }
+        false
+    }


Remark (for myself): I should review how we are handling rustc --print outputs.

Suggestion: actually, can this just use a regex (lazily/once initialized ofc)? Like

static TARGET_HAS_ATOMIC: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"target_has_atomic=\"(?<width>[0-9A-Za-z]+)\"").unwrap() );

jieyouxu · 2024-11-27T16:51:58Z

src/tools/compiletest/src/header/needs.rs

+    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)
    };


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

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?

I'll double-check tmrw

jieyouxu · 2024-11-27T16:52:45Z

src/tools/compiletest/src/header/needs.rs

+        // `needs-target-has-atomic` requires comma-separated data sizes.
+        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")
+                    },
+                };
+            }
+        }
+        return IgnoreDecision::Continue;


Suggestion: this is non-trivial, so can you extract this into a helper?

jieyouxu · 2024-11-27T16:54:17Z

tests/ui/compiletest-self-test/only-target-has-atomic.rs

+// Check this ignored because x86_64-unknown-linux-gnu does not have 128-bit atomic operation.
+
+//@ only-x86_64-unknown-linux-gnu
+//@ needs-target-has-atomic: 128


Suggestion: this could get invalidated in the future because the target might get 128-bit atomic operations in the future, but it's fine in the meantime.

Can you make this instead a headers/tests.rs test? E.g.

rust/src/tools/compiletest/src/header/tests.rs

Line 705 in c322cd5

fn threads_support() {

…#87377

kei519 · 2024-11-28T15:08:00Z

Fixed excpet

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

jieyouxu · 2024-12-02T07:03:20Z

Thanks for the PR, I found it easier to reimplement than to review (because the compiletest setup here was quite convoluted and subtle), so I adapted your changes into a new PR #133736 (co-authored by you, of course) and I rolled another reviewer.

…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

Rollup merge of rust-lang#133736 - jieyouxu:needs-target-has-atomic, 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

rustbot assigned compiler-errors Nov 16, 2024

compiler-errors assigned jieyouxu and unassigned compiler-errors Nov 16, 2024

kei519 force-pushed the fix-87377 branch from cf4e010 to 8d74b2b Compare November 17, 2024 06:14

jieyouxu requested changes Nov 20, 2024

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 Nov 20, 2024

jieyouxu changed the title ~~Add test directive needs-atomic~~ Add test directive needs-target-has-atomic Nov 23, 2024

kei519 force-pushed the fix-87377 branch from 8d74b2b to cf45d77 Compare November 27, 2024 07:14

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 Nov 27, 2024

kei519 requested a review from jieyouxu November 27, 2024 07:21

jieyouxu requested changes Nov 27, 2024

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 Nov 27, 2024

kei519 added 2 commits November 29, 2024 00:03

Add new test directive needs-target-has-atomic to support rust-lang…

0a0db62

…#87377

Add test to self-check Config::has_atomic()

c3c6198

kei519 force-pushed the fix-87377 branch from cf45d77 to c3c6198 Compare November 28, 2024 15:03

jieyouxu 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 Nov 28, 2024

jieyouxu mentioned this pull request Dec 2, 2024

Add needs-target-has-atomic directive #133736

Merged

jieyouxu closed this Dec 2, 2024

kei519 deleted the fix-87377 branch December 2, 2024 07:10

Uh oh!

Conversation

kei519 commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Nov 16, 2024

Uh oh!

rustbot commented Nov 16, 2024

Uh oh!

compiler-errors commented Nov 16, 2024

Uh oh!

kei519 commented Nov 16, 2024

Uh oh!

jieyouxu commented Nov 16, 2024

Uh oh!

kei519 commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jieyouxu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kei519 Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Nov 20, 2024

Uh oh!

kei519 commented Nov 27, 2024

Uh oh!

jieyouxu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kei519 commented Nov 28, 2024

Uh oh!

jieyouxu commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kei519 commented Nov 16, 2024 •

edited

Loading

kei519 commented Nov 17, 2024 •

edited

Loading

kei519 Nov 21, 2024 •

edited

Loading

jieyouxu Nov 21, 2024 •

edited

Loading

jieyouxu commented Dec 2, 2024 •

edited

Loading