Skip to content

1.26.0 - stdsimd/coresimd/x86/mod.rs test failures on non-x86 architectures #50988

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
infinity0 opened this issue May 23, 2018 · 23 comments · Fixed by rust-lang/stdarch#466
Closed
Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@infinity0
Copy link
Contributor

Same failures on Debian and Fedora

test ../stdsimd/coresimd/x86/mod.rs - coresimd::x86::__m256d (line 32) ... FAILED
test ../stdsimd/coresimd/x86/mod.rs - coresimd::x86::__m128 (line 32) ... FAILED
test ../stdsimd/coresimd/x86/mod.rs - coresimd::x86::__m64 (line 39) ... FAILED
test ../stdsimd/coresimd/x86/mod.rs - coresimd::x86::__m256i (line 36) ... FAILED
test ../stdsimd/coresimd/x86/mod.rs - coresimd::x86::__m128i (line 39) ... FAILED
test ../stdsimd/coresimd/x86/mod.rs - coresimd::x86::__m128d (line 32) ... FAILED
test ../stdsimd/coresimd/x86/mod.rs - coresimd::x86::__m256 (line 32) ... FAILED

Current master has diffs on top of 1.26.0 looking like this:

     /// ```
-    /// # #![feature(cfg_target_feature, target_feature, stdsimd)]
+    /// # #![cfg_attr(not(dox), feature(stdsimd))]
     /// # #![cfg_attr(not(dox), no_std)]

but on a first glance this doesn't look like it would fix the failures.

cc @cuviper

@cuviper
Copy link
Member

cuviper commented May 23, 2018

I guess these need a target_arch config filter too?

@cuviper cuviper added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-testsuite Area: The testsuite used to check the correctness of rustc labels May 23, 2018
@infinity0
Copy link
Contributor Author

Is there a way to do that for the whole file, like the opposite of // ignore ?

@infinity0
Copy link
Contributor Author

infinity0 commented May 29, 2018

The error message says:

---- ../stdsimd/coresimd/x86/mod.rs - coresimd::x86::__m256d (line 32) stdout ----
	error: 
is_x86_feature_detected can only be used on x86 and x86_64 targets.
You can prevent it from being used in other architectures by
guarding it behind a cfg(target_arch) as follows:

    #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] {
        if is_x86_feature_detected(...) { ... }
    }

So I'm going to try that and report back. Ironically this overly-strict macro is defined in the very same crate whose tests fail its strictness, in stdsimd/arch/detect/mod.rs.

What's curious is that we don't get the same errors for very similar tests defined in coresimd/x86/avx2.rs, which also use is_x86_feature_detected that are ungarded by a cfg(). There are other similarities and differences about these tests but none of them seem like they should be causing such a failure vs non-failure that we're seeing.

@infinity0
Copy link
Contributor Author

cc @gnzlbg who last touched the detection macro

@gnzlbg
Copy link
Contributor

gnzlbg commented May 29, 2018

I guess these need a target_arch config filter too?

The whole coresimd/x86 module has a cfg on the target_arch for x86 and x86_64 (https://github.com/rust-lang-nursery/stdsimd/blob/master/coresimd/mod.rs#L149).

Why is that module being compiled for your ppc builds? I don't know, our build bots for ppc, ppc64 (be), and ppc64le are not failing, but this doesn't help you much.

Maybe it has something to do with dox, but AFAIK dox is only defining when running cargo doc which does not run any tests. cc @QuietMisdreavus

@gnzlbg
Copy link
Contributor

gnzlbg commented May 29, 2018

Digging into the logs, this is the command that fails

  Doc-tests core
     Running `/<<BUILDDIR>>/rustc-1.26.0+dfsg1/build/bootstrap/debug/rustdoc --test /<<BUILDDIR>>/rustc-1.26.0+dfsg1/src/libcore/lib.rs --crate-name core -L dependency=/<<BUILDDIR>>/rustc-1.26.0+dfsg1/build/powerpc64le-unknown-linux-gnu/stage1-std/powerpc64le-unknown-linux-gnu/release/deps -L dependency=/<<BUILDDIR>>/rustc-1.26.0+dfsg1/build/powerpc64le-unknown-linux-gnu/stage1-std/release/deps --extern core=/<<BUILDDIR>>/rustc-1.26.0+dfsg1/build/powerpc64le-unknown-linux-gnu/stage1-std/powerpc64le-unknown-linux-gnu/release/deps/libcore-dfc03e5d45d15e08.rlib`
rustdoc command: "/<<BUILDDIR>>/rustc-1.26.0+dfsg1/build/powerpc64le-unknown-linux-gnu/stage2/bin/rustdoc" "--test" "/<<BUILDDIR>>/rustc-1.26.0+dfsg1/src/libcore/lib.rs" "--crate-name" "core" "-L" "dependency=/<<BUILDDIR>>/rustc-1.26.0+dfsg1/build/powerpc64le-unknown-linux-gnu/stage1-std/powerpc64le-unknown-linux-gnu/release/deps" "-L" "dependency=/<<BUILDDIR>>/rustc-1.26.0+dfsg1/build/powerpc64le-unknown-linux-gnu/stage1-std/release/deps" "--extern" "core=/<<BUILDDIR>>/rustc-1.26.0+dfsg1/build/powerpc64le-unknown-linux-gnu/stage1-std/powerpc64le-unknown-linux-gnu/release/deps/libcore-dfc03e5d45d15e08.rlib" "--cfg" "stage1" "--cfg" "dox" "--sysroot" "/<<BUILDDIR>>/rustc-1.26.0+dfsg1/build/powerpc64le-unknown-linux-gnu/stage1" "-Z" "force-unstable-if-unmarked" "-Z" "unstable-options" "--crate-version" "1.26.0"

So it might well be that the current setup attempts to run all doc tests of all architectures on all architectures, but that was never going to work.


cc @alexcrichton : this looks like an issue with whitelisting those architectures with dox. The flag that we want here is one that imports those modules when generating documentation, but not when running documentation tests.

cc @GuillaumeGomez

@QuietMisdreavus
Copy link
Member

The reason that dox flag is there is so that the standard library documentation shows information for as many platforms as it can at once, just like how std::os has information on both Linux and Windows. Rustdoc should avoid executing doctests if they're properly tagged with a #[doc(cfg(...))] attribute, as the public exports are. However, this makes me think that those tests are being run from the private modules, due to the path being shown in the failure text. Those aren't tagged with doc(cfg) because of how 64-bit chips extend the primitives of their 32-bit counterpart, so for example they're tagged as x86 or x86_64 based on where they're exported rather than where they're defined.

This makes me wonder whether we should move those attributes and use something like #[doc(cfg(any(target_arch = "x86", target_arch = "x86_64")))], which will add lots of noise to the docs but possibly make these tests run properly.

@gnzlbg
Copy link
Contributor

gnzlbg commented May 29, 2018

However, this makes me think that those tests are being run from the private modules, due to the path being shown in the failure text. Those aren't tagged with doc(cfg) because of how 64-bit chips extend the primitives of their 32-bit counterpart, so for example they're tagged as x86 or x86_64 based on where they're exported rather than where they're defined.

Urgh :/

This makes me wonder whether we should move those attributes and use something like #[doc(cfg(any(target_arch = "x86", target_arch = "x86_64")))], which will add lots of noise to the docs but possibly make these tests run properly.

Let's just do that. There are only a bunch of those types.

@QuietMisdreavus
Copy link
Member

It's possible that we can make the doc(cfg) handling a little more clever by reducing the condition if it gets something like all(a, any(a, b)) to just make it use a in that situation, and leave the doc(cfg) tags on the exports, which should leave the docs as-is, and also properly tag all the doctests! That sounds like it starts to lean into "CS research" territory tho. >_>

@QuietMisdreavus
Copy link
Member

I've opened rust-lang/stdarch#466 to tag the private modules. We'll need to run doctests for libcore in a powerpc64 environment (or in Debian/Fedora's environment, if that doesn't work) to be sure it fixes the issue.

@infinity0
Copy link
Contributor Author

infinity0 commented May 30, 2018

Thanks, I've added this to Debian and uploaded, results will appear here in about 4-10 hours for version 1.26.0+dfsg1-1~exp4.

@gnzlbg
Copy link
Contributor

gnzlbg commented May 30, 2018

Why is debian running ppc64le tests at all? It is a tier2 target which means "test are not guaranteed to pass" and in general they won't because we don't run them at the rustc level (even though some components might run them).

If you want ppc64le tests to pass the first step should be to make it a tier1 rustc target. Otherwise, whatever you fix today we might break tomorrow accidentally.

@infinity0
Copy link
Contributor Author

Seems like @QuietMisdreavus 's fix worked, thanks!

We run all tests everywhere and try to get things working on as many platforms as possible, it's one of the things about Debian that sets it apart from many other FOSS projects. I know it's tier2 for rust, I thought the project would appreciate bug reports.

In general ppc64 has been pretty well-behaved for over a year, and the test failures have been minor issues like this one. Other architectures have also been working fairly well, but notably not mips. I imagine the main thing holding back the rust project from bumping it up to tier1 is the lack of build machines, which we have plenty of at Debian.

@gnzlbg
Copy link
Contributor

gnzlbg commented May 30, 2018

@infinity0 I did not know it was working that good. I think @lu-zero might be interested / able of pushing ppc64le to tier1 since it has been doing a lot of work on altivec and vsx intrinsics.

Maybe you should open an issue about it.

@lu-zero
Copy link
Contributor

lu-zero commented May 30, 2018

I would be glad to help, what tier1 would involve?

@gnzlbg
Copy link
Contributor

gnzlbg commented May 30, 2018

running all rustc tests on ppc64le instead of just compiling the binaries. I don't know if qemu would be enough (like in stdsimd) or if we need to do so on the real hardware, but there are real powerpc's available for CI, so, worst case we'll need to do that. Note that rustc test set up is "similar" to the stdsimd one, which you are already a bit familiar with.

@lu-zero
Copy link
Contributor

lu-zero commented May 31, 2018

I'm sure @edelsohn would be glad to help regarding hardware for CI :)

@edelsohn
Copy link

The Rust CI community can request a Power8 PPC64LE VM at OSUOSL

http://osuosl.org/services/powerdev/

List David Edelsohn as the sponsor.

@lu-zero
Copy link
Contributor

lu-zero commented May 31, 2018

I can probably do all the paperwork, but I'll need some help to deal with the CI setup.

@QuietMisdreavus
Copy link
Member

cc @rust-lang/infra for the note about setting up CI with a ppc64le machine ^


I've tested running the docs with the change to stdsimd, and it turns out my fears about the "platform warning" becoming too verbose were completely unfounded. It turns out the piece that propagates doc(cfg) attributes to contained items (the propagate-doc-cfg pass) runs after non-public items are stripped and re-exports are inlined, so the docs for libcore don't change from where they are right now. It's still alarming that that environment tries to run doctests from private modules, but at least for now these specific doctests won't run any more.

@QuietMisdreavus
Copy link
Member

Update: Turns out, "rustdoc runs doctests on private items despite not being asked to document them" is an old issue: #30094

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 4, 2018

Is this still happening?

@workingjubilee workingjubilee added the A-SIMD Area: SIMD (Single Instruction Multiple Data) label Mar 3, 2023
@jieyouxu
Copy link
Member

Triage: AFAICT this is not actionable because we don't guarantee Tier 2 tests will pass, but thank you for the report.

@jieyouxu jieyouxu added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants