Skip to content
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

Test x86_64-gnu in PR CI, not x86_64-gnu-llvm-14 #112296

Closed
wants to merge 2 commits into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jun 5, 2023

This is a pre-requisite for #112143, which wants to start using download-rustc in PRs. download-rustc doesn't allow providing an external LLVM.

r? @ghost

@rustbot rustbot added 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-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jun 5, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

When I changed codegen tests, CI running old LLVM was useful because it immediately made me aware that I forgot to make them old LLVM compatible (typed pointers 😿). But that problem should get a lot smaller once we finally drop support for typed pointer LLVM.
But I also clearly see the benefit for using download-rustc, so it's probably worth it. cc @nikic anyways if you have opinions

@nikic
Copy link
Contributor

nikic commented Jun 5, 2023

@Nilstrieb I think that's okay, because the issue goes both ways. If we're testing llvm-14, we don't see codegen test failures for the current LLVM version in PRs, so one of the configurations will not be covered either way, and I'm not sure the llvm-14 configuration is inherently more valuable from that perspective. Fixing llvm-N failures is usually just a matter of slapping min-llvm-version on the test, so doesn't require much trial and error.

@Noratrieb
Copy link
Member

Well, I assume that src/llvm-project LLVM will be tested locally, so both configurations are tested. But yeah, not a big concern.

jyn514 added 2 commits June 6, 2023 18:00
This is a pre-requisite for rust-lang#112143, which wants to start
using download-rustc in PRs. download-rustc doesn't allow providing an external LLVM.
this was a pre-existing latent bug, we just didn't have any CI builders exercising it.

fixes the following errors:
```
 ---- [ui] tests/ui/sanitize/new-llvm-pass-manager-thin-lto.rs#opt1 stdout ----

error in revision `opt1`: test compilation failed although it shouldn't!
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/sanitize/new-llvm-pass-manager-thin-lto.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=i686-unknown-linux-gnu" "--cfg" "opt1" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Cstrip=debuginfo" "--remap-path-prefix=/checkout/tests/ui=fake-test-src-base" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/new-llvm-pass-manager-thin-lto.opt1/a" "-A" "unused" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/i686-unknown-linux-gnu/native/rust-test-helpers" "-Clinker=x86_64-linux-gnu-gcc" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/new-llvm-pass-manager-thin-lto.opt1/auxiliary" "-Zsanitizer=address" "-Clto=thin" "-Copt-level=1"
--- stderr -------------------------------
error: linking with `x86_64-linux-gnu-gcc` failed: exit status: 1
   = note: x86_64-linux-gnu-gcc: error: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/i686-unknown-linux-gnu/lib/librustc-nightly_rt.asan.a: No such file or directory

failures:
    [ui] tests/ui/sanitize/badfree.rs
    [ui] tests/ui/sanitize/address.rs
    [ui] tests/ui/sanitize/use-after-scope.rs
    [ui] tests/ui/sanitize/new-llvm-pass-manager-thin-lto.rs#opt0
    [ui] tests/ui/sanitize/new-llvm-pass-manager-thin-lto.rs#opt1
```
@jyn514 jyn514 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 7, 2023
@jyn514
Copy link
Member Author

jyn514 commented Jun 12, 2023

closing in favor of #112143

@jyn514 jyn514 closed this Jun 12, 2023
@jyn514 jyn514 deleted the no-llvm-14-prs branch June 12, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants