Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Add test directive needs-target-has-atomic #133095

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

Closed
kei519 wants to merge 2 commits into rust-lang:master from kei519:fix-87377

Conversation

Copy link
Contributor

@kei519 kei519 commented Nov 16, 2024 •
edited
Loading

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

saethlin reacted with heart emoji
Copy link
Collaborator

rustbot commented Nov 16, 2024

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 rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 16, 2024
Copy link
Collaborator

rustbot commented Nov 16, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Copy link
Member

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

Copy link
Contributor Author

kei519 commented Nov 16, 2024

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

Copy link
Member

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

Copy link
Contributor Author

kei519 commented Nov 17, 2024 •
edited
Loading

Fixed it.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

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.

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 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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@kei519 kei519 Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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).

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 •
edited
Loading

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.

Comment on lines 174 to 179
// 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)
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 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.

Comment on lines 480 to 492
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
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Problem [DETECTION 2/2]: ditto.

@rustbot 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
Copy link
Member

@rustbot author

@jieyouxu jieyouxu changed the title (削除) Add test directive needs-atomic (削除ここまで) (追記) Add test directive needs-target-has-atomic (追記ここまで) Nov 23, 2024
Copy link
Contributor Author

kei519 commented Nov 27, 2024

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

@rustbot 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
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 480 to 492
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
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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()
 );

Comment on lines +176 to 180
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)
};
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

Comment on lines 193 to 221
// `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;
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 is non-trivial, so can you extract this into a helper?

Comment on lines 1 to 4
// 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
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 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.

fn threads_support() {

@rustbot 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
@jieyouxu 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
Copy link
Contributor Author

kei519 commented Nov 28, 2024

Fixed excpet

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

Copy link
Member

jieyouxu commented Dec 2, 2024 •
edited
Loading

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.

@kei519 kei519 deleted the fix-87377 branch December 2, 2024 07:10
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 2, 2024 
...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
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 2, 2024 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu requested changes

Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for // only-cfg-target-has-atomic to compiletest

AltStyle によって変換されたページ (->オリジナル) /