rust-lang / rust-lang/rust

tidy struggles to reason about revisions

Open
#136,763 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

A-compiletest A-tidy C-bug T-bootstrap
Dominant language
Rust
Stars
119k
Forks
16.1k
PR merge metrics
PR metrics pending

Description

after the following diff:

    tests/assembly: cross-compile x86-return-float
    
    We choose to test for Linux and Windows instead of random other targets.

diff --git a/tests/assembly/x86-return-float.rs b/tests/assembly/x86-return-float.rs
index 423263c9673..35b9e2d18d3 100644
--- a/tests/assembly/x86-return-float.rs
+++ b/tests/assembly/x86-return-float.rs
@@ -1,19 +1,27 @@
 //@ assembly-output: emit-asm
-//@ only-x86
 // FIXME(#114479): LLVM miscompiles loading and storing `f32` and `f64` when SSE is disabled.
 // There's no compiletest directive to ignore a test on i586 only, so just always explicitly enable
 // SSE2.
 // Use the same target CPU as `i686` so that LLVM orders the instructions in the same order.
 //@ compile-flags: -Ctarget-feature=+sse2 -Ctarget-cpu=pentium4
 // Force frame pointers to make ASM more consistent between targets
-//@ compile-flags: -Copt-level=3 -C force-frame-pointers
+//@ compile-flags: -C force-frame-pointers
+// At opt-level=3, LLVM can merge two movss into one movsd, and we aren't testing for that.
+//@ compile-flags: -Copt-level=2
 //@ filecheck-flags: --implicit-check-not fld --implicit-check-not fst
-//@ revisions: normal win
-//@[normal] ignore-windows
-//@[win] only-windows
+//@ revisions: linux win
+//@ add-core-stubs
+//@ needs-llvm-components: x86
+//@[linux] compile-flags: --target i686-unknown-linux-gnu
+//@[win] compile-flags: --target i686-pc-windows-msvc
 
 #![crate_type = "lib"]
 #![feature(f16, f128)]
+#![feature(no_core)]
+#![no_core]
+
+extern crate minicore;
+use minicore::*;
 

I got this feedback from tidy:

fmt: checked 5834 files
tidy check
/checkout/tests/assembly/x86-return-float.rs: revision [unspecified] should not specify `needs-llvm-components:` as it doesn't need `--target`
/checkout/tests/assembly/x86-return-float.rs: revision linux should specify `needs-llvm-components:` as it has `--target` set
/checkout/tests/assembly/x86-return-float.rs: revision win should specify `needs-llvm-components:` as it has `--target` set
some tidy checks failed

I think this review should get pushback, but I'm not sure how best to phrase it.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with tests/assembly/x86-return-float.rs and reproduce the tidy check output for its linux and win revisions. Read how tidy interprets revisions, --target, and needs-llvm-components; done means the validation matches the intended per-revision directives or the issue has a precise, supported correction.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
testing-qa, tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.