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

Add test for #80981, print cranelift version #81041

Closed
wants to merge 1 commit into from

Conversation

osa1
Copy link
Contributor

@osa1 osa1 commented Jan 15, 2021

@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 Jan 15, 2021
@osa1
Copy link
Contributor Author

osa1 commented Jan 15, 2021

@bjorn3 this will fail when cranelift backend is used, right? Is cranelift version or anything like that printed in -vV when cranelift is used as the backend?

@bjorn3
Copy link
Member

bjorn3 commented Jan 15, 2021

Can you add a test for -Cpasses=list too?

@bjorn3 this will fail when cranelift backend is used, right?

Indeed

Is cranelift version or anything like that printed in -vV when cranelift is used as the backend?

Nope, nothing is printed. There is a cfg!(feature = "llvm") for the print_version call as llvm is hard coded. Maybe get_builtin_codegen_backend could use the default codegen backend when "" is passed? I can then implement print_version in cg_clif.

@osa1
Copy link
Contributor Author

osa1 commented Jan 15, 2021

@bjorn3 how about this

diff --git a/compiler/rustc_driver/Cargo.toml b/compiler/rustc_driver/Cargo.toml
index b88b556d143..26f657d4c1a 100644
--- a/compiler/rustc_driver/Cargo.toml
+++ b/compiler/rustc_driver/Cargo.toml
@@ -40,4 +40,5 @@ winapi = { version = "0.3", features = ["consoleapi", "debugapi", "processenv"]
 
 [features]
 llvm = ['rustc_interface/llvm']
+cranelift = ['rustc_interface/cranelift']
 max_level_info = ['tracing/max_level_info']
diff --git a/compiler/rustc_driver/src/lib.rs b/compiler/rustc_driver/src/lib.rs
index f8ceb94916e..988dcb23ac5 100644
--- a/compiler/rustc_driver/src/lib.rs
+++ b/compiler/rustc_driver/src/lib.rs
@@ -800,6 +800,8 @@ fn unw(x: Option<&str>) -> &str {
         println!("release: {}", unw(util::release_str()));
         if cfg!(feature = "llvm") {
             get_builtin_codegen_backend("llvm")().print_version();
+        } else if cfg!(feature = "cranelift") {
+            get_builtin_codegen_backend("cranelift")().print_version();
         }
     }
 }
@@ -1089,6 +1091,8 @@ pub fn handle_options(args: &[String]) -> Option<getopts::Matches> {
     if cg_flags.iter().any(|x| *x == "passes=list") {
         if cfg!(feature = "llvm") {
             get_builtin_codegen_backend("llvm")().print_passes();
+        } else if cfg!(feature = "cranelift") {
+            get_builtin_codegen_backend("cranelift")().print_passes();
         }
         return None;
     }
diff --git a/compiler/rustc_interface/Cargo.toml b/compiler/rustc_interface/Cargo.toml
index 2481a27dee7..c9f254104e1 100644
--- a/compiler/rustc_interface/Cargo.toml
+++ b/compiler/rustc_interface/Cargo.toml
@@ -29,6 +29,7 @@ rustc_data_structures = { path = "../rustc_data_structures" }
 rustc_codegen_ssa = { path = "../rustc_codegen_ssa" }
 rustc_symbol_mangling = { path = "../rustc_symbol_mangling" }
 rustc_codegen_llvm = { path = "../rustc_codegen_llvm", optional = true }
+rustc_codegen_cranelift = { path = "../rustc_codegen_cranelift", optional = true }
 rustc_hir = { path = "../rustc_hir" }
 rustc_metadata = { path = "../rustc_metadata" }
 rustc_mir = { path = "../rustc_mir" }
@@ -52,3 +53,4 @@ rustc_target = { path = "../rustc_target" }
 
 [features]
 llvm = ['rustc_codegen_llvm']
+cranelift = ['rustc_codegen_cranelift']
diff --git a/compiler/rustc_interface/src/util.rs b/compiler/rustc_interface/src/util.rs
index f34990a1a10..a22ec92ed5b 100644
--- a/compiler/rustc_interface/src/util.rs
+++ b/compiler/rustc_interface/src/util.rs
@@ -370,6 +370,8 @@ pub fn get_builtin_codegen_backend(backend_name: &str) -> fn() -> Box<dyn Codege
     match backend_name {
         #[cfg(feature = "llvm")]
         "llvm" => rustc_codegen_llvm::LlvmCodegenBackend::new,
+        #[cfg(feature = "cranelift")]
+        "cranelift" => rustc_codegen_cranelift::CraneliftCodegenBackend::new,
         _ => get_codegen_sysroot(backend_name),
     }
 }

We can then implement print_version and print_passes for the cranelift backend.

@bjorn3
Copy link
Member

bjorn3 commented Jan 15, 2021

rustc_codegen_cranelift can't be a dependency of rustc_interface as rustc_codegen_cranelift depends on rustc_interface through rustc_driver. rustc_codegen_cranelift is dynamically loaded at runtime. For this reason the compiler/rustc_interface/src/util.rs change is also not necessary as get_codegen_sysroot can already load rustc_codegen_cranelift.

@osa1
Copy link
Contributor Author

osa1 commented Jan 15, 2021

OK, thanks. I think that's fine. I'll push rest of the changes to this branch, with print_version and print_passes for the cranelift backend.

I don't know what to print in those methods though. Any ideas?

@bjorn3
Copy link
Member

bjorn3 commented Jan 15, 2021

print_passes can be left unimplemented for cg_clif. For print_version I think I will implement it myself.

@osa1
Copy link
Contributor Author

osa1 commented Jan 15, 2021

For print_version I think I will implement it myself.

OK. Please ping me when you do it so I can finish this PR.

@osa1
Copy link
Contributor Author

osa1 commented Jan 15, 2021

@bjorn3
Copy link
Member

bjorn3 commented Jan 15, 2021

@bjorn3 Hm, can I not just use https://docs.rs/cranelift/0.69.0/cranelift/constant.VERSION.html ?

Yeah, I guess so.

@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2021
@Dylan-DPC-zz
Copy link

@osa1 any updates on this?

@osa1 osa1 changed the title Add test for PR #80981 Add test for #80981, print cranelift version Feb 12, 2021
@osa1
Copy link
Contributor Author

osa1 commented Feb 12, 2021

@Dylan-DPC thanks for the ping, I updated the PR now.

@bors -S-waiting-on-author +S-waiting-on-review

@osa1
Copy link
Contributor Author

osa1 commented Feb 12, 2021

@rustbot label: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 12, 2021
- Regression test added for #80981
- -vV now prints cranelift version when cranelift backend is used
@Mark-Simulacrum
Copy link
Member

The PR description suggests this will print the cranelift version, but the version code still seems to contain the LLVM feature guard, which seems surprising. Does this work with cranelift? What am I missing?

@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2021
@osa1
Copy link
Contributor Author

osa1 commented Feb 13, 2021

Sigh.. You're right that the cranelift version is never shown. I don't know how to make it work, without refactoring how cranelift backend is loaded/linked significantly. Let me explain my motivation for this PR and maybe someone can help.

First, I don't care about cranelift version. The reason why I added this is I think there is no way to run a test only when LLVM backend is enabled. So the new test that makes sure -vV shows the LLVM version may be broken in some configurations. I don't know if this is a real problem or not, perhaps it's fine to assume LLVM always. Can anyone comment on this?

Now, the reason why I want -vV to show LLVM version always (and catch any breaking patches before landing) is because I'm using nightly rustc to build Wasm libraries which I link with other Wasm code generated by a different toolchain, using LLVM's wasm-ld. Because Wasm doesn't have a stable ABI I need to make sure the rustc's LLVM, Wasm I generate with custom toolchain, and wasm-ld are all compatible. For this I need to compare versions of rustc's LLVM with wasm-ld (and then manually make sure my toolchain generates compatible Wasm). So I need -vV to work always (in nightly) and as reported in #77975 (comment) it got broken in the past. This PR is to make sure it won't get broken in the future.


If it's fine to assume LLVM always, I'll update the test to just check LLVM version (not cranelift version) and revert print_version change in cranelift backend.

@JohnCSimon JohnCSimon removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 1, 2021
@JohnCSimon JohnCSimon added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 1, 2021
@JohnTitor JohnTitor added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2021
@osa1 osa1 closed this Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants