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

Mark asm tests as requiring LLVM 10.0.1 #83485

Merged
merged 1 commit into from
Mar 26, 2021
Merged

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Mar 25, 2021

No description provided.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Mar 25, 2021
@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member Author

Amanieu commented Mar 25, 2021

Inline asm requires LLVM 10.0.1 but the Ubuntu package is only 10.0.0. Unfortunate.

@Amanieu Amanieu closed this Mar 25, 2021
@Amanieu Amanieu reopened this Mar 25, 2021
@Amanieu
Copy link
Member Author

Amanieu commented Mar 25, 2021

I've added proper LLVM version annotations to the tests so they should be enabled automatically when we do the next LLVM version bump.

@Amanieu Amanieu changed the title Enable asm tests on system LLVM Mark asm tests as requiring LLVM 10.0.1 Mar 25, 2021
@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 25, 2021

📌 Commit 62e7331 has been approved by joshtriplett

@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 Mar 25, 2021
@camelid camelid added A-inline-assembly Area: inline asm!(..) A-testsuite Area: The testsuite used to check the correctness of rustc labels Mar 25, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 25, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#83055 ([rustdoc] Don't document stripped items in JSON renderer.)
 - rust-lang#83437 (Refactor rust-lang#82270 as lint instead of an error)
 - rust-lang#83444 (Fix bootstrap tests on beta)
 - rust-lang#83456 (Add docs for Vec::from functions)
 - rust-lang#83463 (ExitStatusExt: Fix missing word in two docs messages)
 - rust-lang#83470 (Fix patch note about rust-lang#80653 not mentioning nested nor recursive)
 - rust-lang#83485 (Mark asm tests as requiring LLVM 10.0.1)
 - rust-lang#83486 (Don't ICE when using `#[global_alloc]` on a non-item statement)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c5edb4f into rust-lang:master Mar 26, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 26, 2021
@infinity0
Copy link
Contributor

Should codegen/alloc-optimisation.rs really have been shifted away from no-system-llvm? If I understand correctly it depends on rust-specific changes to LLVM regarding knowledge about how malloc works. The test fails on all Debian architectures like this:

------------------------------------------
stderr:
------------------------------------------
/<<PKGBUILDDIR>>/src/test/codegen/alloc-optimisation.rs:10:17: error: CHECK-NEXT: is not on the line after the previous match
 // CHECK-NEXT: ret void
                ^
/<<PKGBUILDDIR>>/build/x86_64-unknown-linux-gnu/test/codegen/alloc-optimisation/alloc-optimisation.ll:25:2: note: 'next' match was here
 ret void
 ^
/<<PKGBUILDDIR>>/build/x86_64-unknown-linux-gnu/test/codegen/alloc-optimisation/alloc-optimisation.ll:11:7: note: previous match ended here
start:
      ^
/<<PKGBUILDDIR>>/build/x86_64-unknown-linux-gnu/test/codegen/alloc-optimisation/alloc-optimisation.ll:12:1: note: non-matching line after previous match is here
 %0 = tail call i8* @__rust_alloc(i64 4, i64 4) #2
^

Input file: /<<PKGBUILDDIR>>/build/x86_64-unknown-linux-gnu/test/codegen/alloc-optimisation/alloc-optimisation.ll
Check file: /<<PKGBUILDDIR>>/src/test/codegen/alloc-optimisation.rs

-dump-input=help explains the following input dump.

Input was:
<<<<<<
         .
         .
         .
        20: 
        21: "_ZN5alloc5boxed12Box$LT$T$GT$3new17haf7fc1520aeaf056E.exit": ; preds = %start
        22:  %2 = bitcast i8* %0 to i32*
        23:  store i32 %data, i32* %2, align 4
        24:  tail call void @__rust_dealloc(i8* nonnull %0, i64 4, i64 4) #2
        25:  ret void
next:10      !~~~~~~~ error: match on wrong line
        26: }
        27: 
        28: ; Function Attrs: nounwind nonlazybind uwtable
        29: declare i32 @rust_eh_personality(i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*) unnamed_addr #0
        30: 
         .
         .
         .
>>>>>>

------------------------------------------

@Amanieu
Copy link
Member Author

Amanieu commented Oct 6, 2021

Oh yea that's definitely a mistake. Rust's LLVM has a custom patch to recognize __rust_alloc as an allocation function like malloc, but this patch isn't in upstream LLVM.

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 6, 2021
fix: alloc-optimisation is only for rust llvm

As discussed at the bottom of rust-lang#83485.

On a separate note I'll take this chance ask, is it worth pulling in that patch (to recognise `__rust_dealloc`) into Debian's system LLVM? The main factors for us to consider would be (1) is the optimisation significant and (2) is there not any significant negative impact to non-rust packages that use LLVM.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inline-assembly Area: inline asm!(..) A-testsuite Area: The testsuite used to check the correctness of rustc 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.

9 participants