Skip to content

fix issue 54153 by not testing issue-18804 on Windows nor OS X. #56772

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Conversation

pnkfelix
Copy link
Member

Fix #54153

@rust-highfive
Copy link
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 13, 2018
@nikic
Copy link
Contributor

nikic commented Dec 13, 2018

It's not entirely obvious to me why this test requires optimization.

@pnkfelix pnkfelix force-pushed the issue-54153-linkage-sometimes-requires-optimizations branch from daecedc to 35da0c6 Compare December 13, 2018 12:08
@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 13, 2018

its not obvious to me either; the requirement may only manifest itself on "OS X" ...

which would lead one to wonder if instead I should tag this test with an // ignore-macos, and then file an additional test just of "OS X" that has the -O flag required?

(Note also that #[linkage] is notorious for being platform-dependent and also has a number of supposed latent bugs...)

In any case I am non-plussed that I cannot do optimize-tests = false in my config.toml without running into this.

@pnkfelix
Copy link
Member Author

cc #18804

@nikic
Copy link
Contributor

nikic commented Dec 13, 2018

On Ubuntu, at least a manual run seems to work fine. I did:

rustc +nightly -C opt-level=0 lib.rs
rustc +nightly --extern lib=liblib.rlib -C opt-level=0 test.rs
./test

Not sure how well that corresponds to whatever run-pass does.

@nikic
Copy link
Contributor

nikic commented Dec 13, 2018

After checking IR, with optimizations the extern_weak global is just optimized away, so it's not actually testing anything. Looking at src/test/run-pass/linkage1.rs, which is another test that uses extern_weak, it skips windows and macos, so I'm assuming those just don't support this linkage.

So I think the right way to fix this is to a) add ignore-windows and ignore-macos as they have no extern_weak linkage and b) add -C no-prepopulate-passes so this test actually tests something.

@alexcrichton
Copy link
Member

Ah yeah I agree with @nikic, this test should be ignored on macos/windows because the extern_weak linkage doesn't really exist there

@pnkfelix
Copy link
Member Author

Okay I'll revise the PR.

As a drive-by, add `-C no-prepopulate-passes` as suggested by nikic.
@pnkfelix pnkfelix force-pushed the issue-54153-linkage-sometimes-requires-optimizations branch from 35da0c6 to 42167b9 Compare December 14, 2018 11:03
@pnkfelix pnkfelix changed the title fix issue 54153 by always running a problematic tests with -O. fix issue 54153 by not testing issue-18804 on Windows nor OS X. Dec 14, 2018
@nikic
Copy link
Contributor

nikic commented Dec 14, 2018

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Dec 14, 2018

📌 Commit 42167b9 has been approved by nikic

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018
…mes-requires-optimizations, r=nikic

fix issue 54153 by not testing issue-18804 on Windows nor OS X.

Fix rust-lang#54153
@nikic
Copy link
Contributor

nikic commented Dec 14, 2018

@bors r-

Failed in rollup #56817 with ... an LLVM assertion failure :(

[01:00:08] ---- [run-pass] run-pass/issues/issue-18804/main.rs stdout ----
[01:00:08] 
[01:00:08] error: test compilation failed although it shouldn't!
[01:00:08] status: signal: 6
[01:00:08] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/issues/issue-18804/main.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/issues/issue-18804/main/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-C" "no-prepopulate-passes" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/issues/issue-18804/main/auxiliary"
[01:00:08] ------------------------------------------
[01:00:08] 
[01:00:08] ------------------------------------------
[01:00:08] stderr:
[01:00:08] stderr:
[01:00:08] ------------------------------------------
[01:00:08] rustc: /checkout/src/llvm/include/llvm/ADT/StringRef.h:239: char llvm::StringRef::operator[](size_t) const: Assertion `Index < Length && "Invalid index!"' failed.
[01:00:08] ------------------------------------------
[01:00:08] 
[01:00:08] thread '[run-pass] run-pass/issues/issue-18804/main.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3252:9
[01:00:08] note: Run with `RUST_BACKTRACE=1` for a backtrace.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 14, 2018
@nikic
Copy link
Contributor

nikic commented Dec 14, 2018

Just checked this locally with an assertion-enabled build ... the issue is that due to -C no-prepopulate-passes the name-anon-globals pass is not run, but we have anon globals and are using ThinLTO buffers. We have checks in place to emit a compile error in this case (rather than this wonderful LLVM assertion), but they happen to miss this particular case.

To fix this you can either drop the -C no-prepopulate-passes or add an additional -C passes=name-anon-globals (we do this in a few other tests).

We should probably start always running name-anon-globals (even under no-prepopulate-passes), as this issue has come up a few times now and we never quite seem to catch all the cases where it's necessary to run it.

@pnkfelix
Copy link
Member Author

@bors r=nikic

@bors
Copy link
Collaborator

bors commented Dec 17, 2018

📌 Commit 933efd7 has been approved by nikic

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 17, 2018
@pnkfelix
Copy link
Member Author

@bors rollup

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 19, 2018
…mes-requires-optimizations, r=nikic

fix issue 54153 by not testing issue-18804 on Windows nor OS X.

Fix rust-lang#54153
bors added a commit that referenced this pull request Dec 19, 2018
Rollup of 15 pull requests

Successful merges:

 - #56363 (Defactored Bytes::read)
 - #56663 (Remove lifetime from Resolver)
 - #56689 (add a lint group for lints emitted by rustdoc)
 - #56772 (fix issue 54153 by not testing issue-18804 on Windows nor OS X.)
 - #56820 (format-related tweaks)
 - #56881 (Implement Eq, PartialEq and Hash for atomic::Ordering)
 - #56907 (Fix grammar in compiler error for array iterators)
 - #56908 (rustc: Don't ICE on usage of two new target features)
 - #56910 (Do not point at delim spans for complete correct blocks)
 - #56913 (Enable stack probes for UEFI images)
 - #56918 (Profiler: simplify total_duration, improve readability)
 - #56931 (Update release notes for Rust 1.31.1)
 - #56947 (Add targets thumbv7neon-linux-androideabi and thumbv7neon-unknown-linux-gnueabihf)
 - #56948 (Update LLVM submodule)
 - #56959 (Fix mobile menu rendering collision with tooltip.)

Failed merges:

 - #56914 (Ignore ui/target-feature-gate on sparc, sparc64, powerpc, powerpc64 and powerpc64le)

r? @ghost
@bors bors merged commit 933efd7 into rust-lang:master Dec 19, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants