-
Notifications
You must be signed in to change notification settings - Fork 13.7k
rustc: Load the rustc_trans
crate at runtime
#47671
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,7 +69,7 @@ extern crate tempdir; | |
|
||
use back::bytecode::RLIB_BYTECODE_EXTENSION; | ||
|
||
pub use llvm_util::{target_features, print_version, print_passes}; | ||
pub use llvm_util::target_features; | ||
|
||
use std::any::Any; | ||
use std::path::PathBuf; | ||
|
@@ -149,13 +149,16 @@ impl !Send for LlvmTransCrate {} // Llvm is on a per-thread basis | |
impl !Sync for LlvmTransCrate {} | ||
|
||
impl LlvmTransCrate { | ||
pub fn new(sess: &Session) -> Box<TransCrate> { | ||
llvm_util::init(sess); // Make sure llvm is inited | ||
pub fn new() -> Box<TransCrate> { | ||
box LlvmTransCrate(()) | ||
} | ||
} | ||
|
||
impl TransCrate for LlvmTransCrate { | ||
fn init(&self, sess: &Session) { | ||
llvm_util::init(sess); // Make sure llvm is inited | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is fine for now but in the future we might want to have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe let There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I think we can always create a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah so the two locations this was necessary (not taking a I it'd be possible to have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't fully parse arguments before processing the version? Is it a separate codepath? EDIT: note that you can't load the backend without also parsing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well this PR doesn't use target specs yet, so it's not necessary for now. But I am curious, because I was hoping for a straight-forward implementation, is the "printing the version skips checking the rest of the CLI arguments" guaranteed/relied upon? EDIT: I guess one could argue that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it basically would just require more refactoring. We'd have to, for example, initialize the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we validate everything early and buffer the errors or something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess? Again though I didn't get the impression that the argument parsing was at all ready for this sort of refactoring, in the sense that it seemed outside the scope of this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't want to block this PR, this is more for future reference. |
||
} | ||
|
||
fn print(&self, req: PrintRequest, sess: &Session) { | ||
match req { | ||
PrintRequest::RelocationModels => { | ||
|
@@ -183,6 +186,19 @@ impl TransCrate for LlvmTransCrate { | |
} | ||
} | ||
|
||
fn print_passes(&self) { | ||
llvm_util::print_passes(); | ||
} | ||
|
||
fn print_version(&self) { | ||
llvm_util::print_version(); | ||
} | ||
|
||
#[cfg(not(stage0))] | ||
fn diagnostics(&self) -> &[(&'static str, &'static str)] { | ||
&DIAGNOSTICS | ||
} | ||
|
||
fn target_features(&self, sess: &Session) -> Vec<Symbol> { | ||
target_features(sess) | ||
} | ||
|
@@ -252,8 +268,8 @@ impl TransCrate for LlvmTransCrate { | |
|
||
/// This is the entrypoint for a hot plugged rustc_trans | ||
#[no_mangle] | ||
pub fn __rustc_codegen_backend(sess: &Session) -> Box<TransCrate> { | ||
LlvmTransCrate::new(sess) | ||
pub fn __rustc_codegen_backend() -> Box<TransCrate> { | ||
LlvmTransCrate::new() | ||
} | ||
|
||
struct ModuleTranslation { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,18 +8,12 @@ | |
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
// ignore-cross-compile | ||
|
||
#![feature(rustc_private)] | ||
|
||
extern crate tempdir; | ||
|
||
use std::env; | ||
use std::fs; | ||
use tempdir::TempDir; | ||
use std::path::PathBuf; | ||
|
||
fn main() { | ||
let td = TempDir::new("create-dir-all-bare").unwrap(); | ||
env::set_current_dir(td.path()).unwrap(); | ||
let path = PathBuf::from(env::var_os("RUST_TEST_TMPDIR").unwrap()); | ||
env::set_current_dir(&path).unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these changes necessary? Can we no longer link to tempdir from tests? Or is it that tempdir wasn't being rebuilt because rustbuild is buggy (I would not be surprised). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The dependencies of librustc_trans are no longer available in the sysroot (as we just manually copy the one DLL), and one of its dependencies was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I recall, the previous solution to this "problem" would be to move tempdir to a dependency of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's true yeah we could do that but I'm also always a fan of tests with fewer dependencies :) |
||
fs::create_dir_all("create-dir-all-bare").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.
This might be a function that should lie on the trait -- it seems like it should anyway, or maybe the trait could return a vec of errors that need to be added.
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.
Heh it actually is! Unfortunately though there's not a great way to load it here, so I figured that for the one extended error in librustc_trans we could fudge it for now and figure it out later.
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.
Could we just load all of the diagnostics, independent of backend, and then just some of them would be unused?
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.
Unfortunately that's also gonna be difficult I think. I believe that with this PR as-is we could do that, but in the future I think we'll only want to dlopen one version of LLVM into the current process, and at this point in time we don't actually know the trans backend we'd otherwise be using. That means that with the Emscripten target we'd load the normal LLVM backend to load diagnostics and then load the Emscripten backend to actually do trans. I think that it would work ok? The "global" nature of the dlopen though required here though may throw a wrench into that..
In essence though I don't think there's a quick fix or an easy workaround for this, we'll want to deal with it later I think.
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.
Oh also note that I added a dynamic assertion for this, the business with the
LOADED
constant, which asserts that we only load a backend once.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.
Sure, yeah, I understand that we don't want to dlopen multiple rustc_trans backends, but I was rather suggesting that we put all the diagnostic definition in
driver
and then the backend would use whichever ones it needed, though not all. This does increase the API surface of driver.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.
The diagnostics can probably be moved to the
librustc_mir/monomorphize
instance collector, so they're shared by all backends.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 yes indeed, such a strategy would for sure work!