Mostly fix metadata_only backend and extract some code out of rustc_codegen_llvm #51590
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
bjorn3
commented
Jun 16, 2018
src/librustc_driver/lib.rs
Outdated
| **This is suitable for dev purposes only**", | ||
| backend_name, sysroot.display()); | ||
| early_warn(ErrorOutputType::default(), &warn); | ||
| return rustc_codegen_utils::codegen_backend::MetadataOnlyCodegenBackend::new; |
There was a problem hiding this comment.
I couldn't find a reliable way to pass -Zcodegen-backend=metadata-only to stage1+, but not the bootstrap compiler.
| @@ -0,0 +1,112 @@ | |||
| use rustc::session::Session; | |||
There was a problem hiding this comment.
Moved to rustc_codegen_utils, because MetadataOnlyCodegenBackend needs to pass them to target_features_whitelist to compile core.
There was a problem hiding this comment.
I don't think this is a good direction - LLVM shouldn't be treated that special. Is there a way to make core not contain a usage core::arch if the target features are missing?
There was a problem hiding this comment.
@eddyb what do you mean here with "if the target features are missing"? Because of run-time feature detection, core::arch always need to be available.
I don't think this is a good direction - LLVM shouldn't be treated that special
Once we add cranelift as a backend we'll need to map Rust's feature names to backend names (once for LLVM, once for Cranelift), and all backends won't support all targets. So I think that at least the white-lists are going to need to exist on a per backend basis.
| let buf: OwningRef<Vec<u8>, [u8]> = OwningRef::new(buf).into(); | ||
| return Ok(rustc_erase_owner!(buf.map_owner_box())); | ||
| } | ||
| } |
There was a problem hiding this comment.
It couldn't load all metadata produced by rustc_codegen_llvm anyway, because it didn't support dylibs.
| @@ -81,96 +76,27 @@ pub trait CodegenBackend { | |||
| ) -> Result<(), CompileIncomplete>; | |||
| } | |||
|
|
|||
| pub struct DummyCodegenBackend; | |||
There was a problem hiding this comment.
Not even in librustc_driver/test.rs?
src/librustc_codegen_llvm/lib.rs
Outdated
| @@ -79,6 +79,7 @@ use rustc::ty::{self, TyCtxt}; | |||
| use rustc::util::nodemap::{FxHashSet, FxHashMap}; | |||
| use rustc_mir::monomorphize; | |||
| use rustc_codegen_utils::codegen_backend::CodegenBackend; | |||
| use rustc_codegen_utils::time_graph; | |||
There was a problem hiding this comment.
Moved to rustc_codegen_utils, because it is useful for other future backends too.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bjorn3
force-pushed
the
codegen_llvm_extract
branch
from
54a7f98 to
d77bf58
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/bootstrap/compile.rs
Outdated
| @@ -640,7 +642,9 @@ impl Step for CodegenBackend { | |||
| .arg(builder.src.join("src/librustc_codegen_llvm/Cargo.toml")); | |||
| rustc_cargo_env(builder, &mut cargo); | |||
|
|
|||
| features += &build_codegen_backend(&builder, &mut cargo, &compiler, target, backend); | |||
| if !backend.is_empty() { | |||
There was a problem hiding this comment.
This is probably not the check you wanted -- we shouldn't be creating CodegenBackend with an empty string. I think we can probably just remove this if.
There was a problem hiding this comment.
What exactly do you mean?
There was a problem hiding this comment.
backend here isn't an array, but a Interned<String> -- this check should be removed.
src/librustc_codegen_llvm/base.rs
Outdated
| @@ -715,7 +714,7 @@ pub fn codegen_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |||
| rx: mpsc::Receiver<Box<Any + Send>>) | |||
| -> OngoingCodegen { | |||
|
|
|||
| check_for_rustc_errors_attr(tcx); | |||
| ::rustc_codegen_utils::check_for_rustc_errors_attr(tcx); | |||
There was a problem hiding this comment.
What's the advantage to not importing this?
|
r? @eddyb |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bjorn3
changed the title
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Why is it failing? Edit: hopefully fixed it. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Still not fixed :( |
|
Fixed :) |
bjorn3
force-pushed
the
codegen_llvm_extract
branch
2 times, most recently
from
219d2b2 to
fb804ad
Compare
|
Squashed the commits to fix the test |
oli-obk
reviewed
Jun 20, 2018
| @@ -225,7 +161,7 @@ impl CodegenBackend for MetadataOnlyCodegenBackend { | |||
| collector::MonoItemCollectionMode::Eager | |||
| ).0.iter() | |||
| ); | |||
| ::rustc::middle::dependency_format::calculate(tcx); | |||
| //::rustc::middle::dependency_format::calculate(tcx); | |||
bjorn3
commented
Jun 21, 2018
config.toml.example
Outdated
| # make this an empty array (but that probably won't get too far in the | ||
| # bootstrap) | ||
| # make this an empty array (that will disable LLVM, but bootstrap will fail at | ||
| # the time the final rustc binary is built for stage 1) |
There was a problem hiding this comment.
Is this the right stage? I always get confused about it.
There was a problem hiding this comment.
I don't think this needs to be that explicit, keeping the original "but that probably won't get too far in the bootstrap" and just adding "which will disable LLVM" before it seems enough.
|
cc @alexcrichton on the rustbuild/LLVM changes |
eddyb
reviewed
Jun 22, 2018
src/librustc_codegen_llvm/common.rs
Outdated
| @@ -448,3 +448,101 @@ pub fn ty_fn_sig<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, | |||
| _ => bug!("unexpected type {:?} to ty_fn_sig", ty) | |||
| } | |||
| } | |||
|
|
|||
| pub fn size_and_align_of_dst<'a, 'tcx>(bx: &Builder<'a, 'tcx>, t: Ty<'tcx>, info: ValueRef) | |||
There was a problem hiding this comment.
Why move this? I'd rather see base and common go away just as much as glue, so moving something here doesn't seem that useful. IMO this should be a method on PlaceRef, that's just size_and_align_of.
eddyb
reviewed
Jun 22, 2018
| let header = Header::new("rust.metadata.bin".to_string(), metadata.len() as u64); | ||
| builder.append(&header, Cursor::new(metadata)).unwrap(); | ||
| let mut file = File::create(&output_name).unwrap(); | ||
| file.write_all(metadata).unwrap(); |
There was a problem hiding this comment.
So all library types are effectively the same as --emit=metadata?
There was a problem hiding this comment.
For the metadata-only backend yes, except for dylibs, because these must have compressed metadata.
eddyb
reviewed
Jun 22, 2018
| @@ -2110,7 +2110,6 @@ dependencies = [ | |||
| name = "rustc_codegen_utils" | |||
| version = "0.0.0" | |||
| dependencies = [ | |||
| "ar 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", | |||
There was a problem hiding this comment.
Does something else depend on this crate?
There was a problem hiding this comment.
I was asking because there's a "checksum" line that would also be removed if this was the last use of ar.
I just checked and rustc_driver also depends on ar.
There was a problem hiding this comment.
what??? how did tidy pass then? i removed ar as whitelisted dep
eddyb
reviewed
Jun 22, 2018
|
|
||
| use rustc_data_structures::owning_ref::OwningRef; | ||
| use rustc_data_structures::sync::Lrc; | ||
| use ar::{Archive, Builder, Header}; | ||
| use flate2::Compression; | ||
| use flate2::write::DeflateEncoder; |
There was a problem hiding this comment.
Compressing the metadata, when rustc_metadata expects it.
|
In general I dislike |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bjorn3
force-pushed
the
codegen_llvm_extract
branch
from
1148253 to
ff12beb
Compare
|
Rebased |
This comment has been minimized.
This comment has been minimized.
|
@bors: r+ |
|
📌 Commit 23c0b3b has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jul 8, 2018
Mostly fix metadata_only backend and extract some code out of rustc_codegen_llvm Removes dependency on the `ar` crate and removes the `llvm.enabled` config option in favour of setting `rust.codegen-backends` to `[]`.
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes dependency on the
arcrate and removes thellvm.enabledconfig option in favour of settingrust.codegen-backendsto[].