-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Allow writing metadata without llvm #44085
Conversation
r? @arielb1 (rust_highfive has picked a reviewer for you, use r? to override) |
2a63467
to
dd253c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to see this as a possibility! Left a few minor comments about bootstrap.
src/bootstrap/compile.rs
Outdated
builder.cargo(compiler, Mode::Libstd, target, "build") | ||
}else{ | ||
builder.cargo(compiler, Mode::Libstd, target, "check") | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong/unrelated? If not, reasoning would be appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should make this no llvm only. When stage 1 rustc doesnt have llvm support it cant build regular rlibs, but only metadata only rlibs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I suppose that makes sense.
src/bootstrap/compile.rs
Outdated
@@ -161,6 +165,7 @@ pub fn std_cargo(build: &Build, | |||
// missing | |||
// We also only build the runtimes when --enable-sanitizers (or its | |||
// config.toml equivalent) is used | |||
//cargo.env("RUST_FLAGS", "-Zno-trans"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to go before we merge.
src/librustc_driver/driver.rs
Outdated
::rustc_trans_utils::find_exported_symbols(tcx, &analysis.reachable); | ||
let (metadata, _hashes) = | ||
cstore.encode_metadata(tcx, &link_meta, &exported_symbols); | ||
let mut builder = Builder::new(File::create("abc.rlib").unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to make this write to the file asked to, instead of abc.rlib.
|
What is this error message of the "Fix tidy errors" commit:
|
|
src/bootstrap/bin/rustc.rs
Outdated
@@ -159,6 +159,10 @@ fn main() { | |||
cmd.arg("-C").arg("panic=abort"); | |||
} | |||
|
|||
if cfg!(not(feature="llvm")) && stage != "0" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't you mean !self.build.config.llvm_enabled
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesnt have self.build.config
but i dont think its compiled with the feature, so i think i will have to use an env var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, an env var would be fine. I think there's already precedent for that (i.e. grep for a env var with LLVM in it) so there may not be a need to add a new one.
|
☔ The latest upstream changes (presumably #44142) made this pull request unmergeable. Please resolve the merge conflicts. |
bec1f52
to
ea497a2
Compare
☔ The latest upstream changes (presumably #43017) made this pull request unmergeable. Please resolve the merge conflicts. |
b88fcf7
to
243d64f
Compare
I added a generic |
@@ -524,14 +524,20 @@ impl<'a> Builder<'a> { | |||
// For other crates, however, we know that we've already got a standard | |||
// library up and running, so we can use the normal compiler to compile | |||
// build scripts in that situation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also modify the comment?
src/librustc_driver/driver.rs
Outdated
@@ -243,7 +248,48 @@ pub fn compile_input(sess: &Session, | |||
tcx.print_debug_stats(); | |||
} | |||
|
|||
let trans = phase_4_translate_to_llvm(tcx, analysis, incremental_hashes_map, | |||
#[cfg(not(feature="llvm"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving this outside of the main driver function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remove the #[cfg]
src/librustc_driver/driver.rs
Outdated
@@ -243,7 +248,48 @@ pub fn compile_input(sess: &Session, | |||
tcx.print_debug_stats(); | |||
} | |||
|
|||
let trans = phase_4_translate_to_llvm(tcx, analysis, incremental_hashes_map, | |||
#[cfg(not(feature="llvm"))] | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure about the idea of creating dylibs that are actually rlibs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without llvm we cant create real dylibs, but many rustc crates are compiled to dylibs, so by emitting rlibs instead we can compile all rustc libs.
src/librustc_driver/lib.rs
Outdated
|
||
#[cfg(not(feature="llvm"))] | ||
mod no_llvm_metadata_loader { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to an out-of-line module?
src/librustc_driver/lib.rs
Outdated
#[cfg(not(feature="llvm"))] | ||
mod rustc_trans { | ||
use syntax_pos::symbol::Symbol; | ||
mod trans_metadata_only { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to an out-of-line module?
src/librustc_trans_traits/lib.rs
Outdated
fn get_dylib_metadata(&self, _target: &Target, _filename: &Path) -> Result<ErasedBoxRef<[u8]>, String> { | ||
bug!("DummyMetadataLoader::get_dylib_metadata"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also move the "metadata-only" TransCrate
implementation here to rustc_trans_utils
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do it
src/librustc_trans_traits/Cargo.toml
Outdated
@@ -0,0 +1,17 @@ | |||
[package] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this exist as a separate crate from rustc_trans_utils
? They are both depended upon by exactly rustc_trans
and rustc_driver
, and they semantically perform a very similar task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about utils to be used by backends only, while traits contains the glue between backends and rustc_driver. I can merge them though.
src/librustc_driver/lib.rs
Outdated
|
||
use self::back::write::OngoingCrateTranslation; | ||
#[cfg(not(feature="llvm"))] | ||
mod rustc_trans { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move all of these items to the TransCrate
trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because most of them are llvm specific, eg they would still use llvm even if trans is non llvm. (enable_llvm_debug, print_passes)
06155cf
to
e130ccc
Compare
@arielb1 rebased and fixed most nits |
42e12da
to
56c4670
Compare
@bors r+ |
📌 Commit 3c32c6a has been approved by |
⌛ Testing commit 3c32c6a with merge 0e7f2f843dce90acc9e8ec1f29665ba69ac1f844... |
💔 Test failed - status-travis |
Error should be fixed now. |
@bors r+ |
📌 Commit 843cd5b has been approved by |
Allow writing metadata without llvm # Todo: * [x] Rebase * [x] Fix eventual errors * [x] <strike>Find some crate to write elf files</strike> (will do it later) Cc #43842
☀️ Test successful - status-appveyor, status-travis |
Todo:
Find some crate to write elf files(will do it later)Cc #43842