Make only rustc_trans depend on rustc_llvm #41565

hanna-kruppe · 2017-04-26T21:35:54Z

With these changes, only rustc_trans depends directly on rustc_llvm (and no crate gained a new dependency on trans). This means changing LLVM doesn't rebuild librustc or rustc_metadata, only rustc_trans, rustc_driver and the rustc executable
Also, rustc_driver technically doesn't know about LLVM any more (of course, it still handles a ton of options that conceptually refer to LLVM, but it delegates their implementation to trans).

What I didn't implement was merging most or all of rustc_llvm into rustc_trans. I ran into a nasty bug, which was probably just a silly typo somewhere but I probably won't have the time to figure it out in the next week or two. I opened #41699 for that step.

Fixes #41473

rust-highfive · 2017-04-26T21:35:59Z

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2017-04-27T19:33:29Z

Thanks for the PR! We’ll periodically check in on it to make sure that @arielb1 or someone else from the team reviews it soon.

bors · 2017-05-01T19:58:04Z

☔ The latest upstream changes (presumably #41560) made this pull request unmergeable. Please resolve the merge conflicts.

hanna-kruppe · 2017-05-02T10:10:38Z

This is now ready for review, though it still can't merge because of #27812. I updated the PR description to summarize what this PR does and doesn't do.

hanna-kruppe · 2017-05-04T14:27:06Z

Fixed the travis failure, I missed one place in rustdoc (as I had feared when I did this refactoring 😅 ).

Mark-Simulacrum · 2017-05-07T00:38:05Z

Looks like there are a few more test failures:

[00:53:00] failures:
[00:53:00]     [run-make] run-make/issue-19371
[00:53:00]     [run-make] run-make/sysroot-crates-are-unstable

hanna-kruppe · 2017-05-07T11:06:03Z

Fixed run-make/issue-19371, thanks for pointing that out!

The other failure is expected due to #27812. Fixing it involves either adding #![cfg_attr(rustbuild, unstable(...))] to the crates.io packages pulled in (boo) or @eddyb implementing -Z force-unstable=rustc_private. Either way, this PR can be reviewed independently of that.

arielb1

nits are minor. fix these if you want.

r+ modulo owning_ref working

arielb1 · 2017-05-07T13:59:13Z

src/librustc_metadata/cstore_impl.rs

@@ -565,4 +552,9 @@ impl CrateStore for cstore::CStore {
        drop(visible_parent_map);
        self.visible_parent_map.borrow()
    }
+


nit: extra newline

arielb1 · 2017-05-07T14:21:55Z

src/librustc_trans/lib.rs

@@ -180,3 +180,55 @@ pub struct CrateTranslation {
 }

 __build_diagnostic_array! { librustc_trans, DIAGNOSTICS }
+
+use rustc::session::Session;
+pub fn init(sess: &Session) {


Could you make this return a token so that you can't call the 10 or so public trans functions without calling init (AFAICT half of these - provide, find_crate_name, filename_for_input, default_output_for_target, invalid_output_for_target - don't need LLVM, but a few others do)?

arielb1 · 2017-05-07T14:29:26Z

src/librustc_trans/llvm_util.rs

+use libc::{c_int, c_char};
+use std::ffi::CString;
+
+pub fn init(sess: &Session) {


Could you make this return a token so that you can't call the 10 or so public trans functions without calling init (AFAICT half of these - provide, find_crate_name, filename_for_input, default_output_for_target, invalid_output_for_target - don't need LLVM, but a few others do)?

Can't they just call init themselves? The Once would prevent misuse.

I'm a bit torn on this. Threading a token everywhere is a lot of infrastructure for the benefit IMHO. Initializing when calling into trans solves that, but needs care when exposing new functions from trans. Requiring the driver to call init obviously places a burden on the driver, but it's a relatively simple one (a single call, and can be the same for all backends) that needs to be observed in fewer places.

I've decided to punt on this for the time being. I hope we'll get something like a Backend trait object, which would solve this issue without having to audit every public API of trans.

bors · 2017-05-08T07:51:12Z

☔ The latest upstream changes (presumably #41818) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2017-05-10T10:29:15Z

src/librustc/middle/cstore.rs

@@ -268,6 +286,9 @@ pub trait CrateStore {
                                 reachable: &NodeSet)
                                 -> EncodedMetadata;
    fn metadata_encoding_version(&self) -> &[u8];
+
+    // access to the metadata loader
+    fn metadata_loader(&self) -> &MetadataLoader;


Can you move this next to crate_data_as_rc_any (at the top of the trait)? They're both special.

eddyb · 2017-05-10T10:30:28Z

src/librustc_driver/lib.rs

@@ -652,12 +657,10 @@ impl RustcDefaultCalls {
                    }
                }
                PrintRequest::TargetCPUs => {
-                    let tm = create_target_machine(sess);
-                    unsafe { llvm::LLVMRustPrintTargetCPUs(tm); }
+                    rustc_trans::print_target_cpus(sess);


Can this be a single entry-point that takes a PrintRequest?

Yes, I just didn't realize how when I wrote this code. One advantage of leaving a PR to rot for a week: You get a fresh perspective 😄

eddyb · 2017-05-10T10:31:07Z

src/librustc_driver/lib.rs

-            println!("LLVM version: {}.{}",
-                     llvm::LLVMRustVersionMajor(), llvm::LLVMRustVersionMinor());
-        }
+        rustc_trans::print_llvm_version();


Can this be print_version? Other backends would print other information.

eddyb · 2017-05-10T10:31:49Z

src/librustc_driver/test.rs

@@ -103,13 +104,14 @@ fn test_env<F>(source_string: &str,

    let dep_graph = DepGraph::new(false);
    let _ignore = dep_graph.in_ignore();
-    let cstore = Rc::new(CStore::new(&dep_graph));
+    let cstore = Rc::new(CStore::new(&dep_graph, box rustc_trans::LlvmMetadataLoader));


Can this not use a dummy CStore? cc @nikomatsakis

bors · 2017-05-15T12:54:08Z

💔 Test failed - status-travis

Mark-Simulacrum · 2017-05-15T12:57:44Z

Legitimate failure, presumably: [01:22:46] thread '[run-make] run-make/llvm-pass' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2472.

hanna-kruppe · 2017-05-15T13:10:31Z

Oops! I thought I'd tested run-make. Maybe it was on Windows, where that test is disabled? Anyway, fixed.

hanna-kruppe · 2017-05-15T13:16:35Z

(Well, hopefully fixed, I can't test right now due to being on Windows.)

eddyb · 2017-05-15T14:52:46Z

@bors r+

bors · 2017-05-15T14:52:47Z

📌 Commit 04a16ff has been approved by eddyb

Make only rustc_trans depend on rustc_llvm With these changes, only rustc_trans depends directly on rustc_llvm (and no crate gained a new dependency on trans). This means changing LLVM doesn't rebuild librustc or rustc_metadata, only rustc_trans, rustc_driver and the rustc executable Also, rustc_driver technically doesn't know about LLVM any more (of course, it still handles a ton of options that conceptually refer to LLVM, but it delegates their implementation to trans). What I *didn't* implement was merging most or all of rustc_llvm into rustc_trans. I ran into a nasty bug, which was probably just a silly typo somewhere but I probably won't have the time to figure it out in the next week or two. I opened rust-lang#41699 for that step. Fixes rust-lang#41473

bors · 2017-05-15T19:01:48Z

⌛ Testing commit 04a16ff with merge ceef6c9...

bors · 2017-05-15T19:08:07Z

💔 Test failed - status-appveyor

alexcrichton · 2017-05-15T19:10:54Z

@bors: retry

network failure Tracking issue for spurious network failures on bots #40474

bors · 2017-05-15T20:04:28Z

⌛ Testing commit 04a16ff with merge ed1bd1a...

bors · 2017-05-15T20:09:22Z

💔 Test failed - status-appveyor

Mark-Simulacrum · 2017-05-15T20:17:57Z

@bors retry

Handle.exe network failure Tracking issue for spurious network failures on bots #40474.

bors · 2017-05-15T21:44:22Z

⌛ Testing commit 04a16ff with merge 8ec9946...

bors · 2017-05-15T21:49:08Z

💔 Test failed - status-appveyor

alexcrichton · 2017-05-15T22:12:03Z

@bors: retry

network issues Tracking issue for spurious network failures on bots #40474

bors · 2017-05-16T05:41:17Z

⌛ Testing commit 04a16ff with merge fa78d5b...

Make only rustc_trans depend on rustc_llvm With these changes, only rustc_trans depends directly on rustc_llvm (and no crate gained a new dependency on trans). This means changing LLVM doesn't rebuild librustc or rustc_metadata, only rustc_trans, rustc_driver and the rustc executable Also, rustc_driver technically doesn't know about LLVM any more (of course, it still handles a ton of options that conceptually refer to LLVM, but it delegates their implementation to trans). What I *didn't* implement was merging most or all of rustc_llvm into rustc_trans. I ran into a nasty bug, which was probably just a silly typo somewhere but I probably won't have the time to figure it out in the next week or two. I opened #41699 for that step. Fixes #41473

bors · 2017-05-16T08:14:25Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing fa78d5b to master...

They're not used in `rustc_session`, and `rustc_metadata` is a more obvious location. `MetadataLoader` was originally put into `rustc_session` in rust-lang#41565 to avoid a dependency on LLVM, but things have changed a lot since then and that's no longer relevant, e.g. `rustc_codegen_llvm` depends on `rustc_metadata`.

rust-highfive assigned arielb1 Apr 26, 2017

alexcrichton added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2017

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2017

hanna-kruppe force-pushed the llvm-sys branch 4 times, most recently from b09da8c to c328b3e Compare April 30, 2017 23:48

hanna-kruppe force-pushed the llvm-sys branch from c328b3e to ae6c9e9 Compare May 2, 2017 09:58

hanna-kruppe mentioned this pull request May 2, 2017

Merge (most of) rustc_llvm into rustc_trans #41699

Closed

hanna-kruppe changed the title ~~WIP towards only rustc_trans depend on rustc_llvm~~ Make only rustc_trans depend on rustc_llvm May 2, 2017

hanna-kruppe force-pushed the llvm-sys branch 2 times, most recently from 36c2a96 to 4d765c8 Compare May 4, 2017 14:26

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 May 7, 2017

hanna-kruppe force-pushed the llvm-sys branch from 4d765c8 to 8bc4162 Compare May 7, 2017 11:02

arielb1 approved these changes May 7, 2017

View reviewed changes

arielb1 reviewed May 7, 2017

View reviewed changes

eddyb reviewed May 10, 2017

View reviewed changes

Fix run-make/llvm-pass

04a16ff

frewsxcv mentioned this pull request May 15, 2017

Rollup of 7 pull requests #42010

Closed

frewsxcv mentioned this pull request May 15, 2017

Rollup of 6 pull requests #42012

Closed

bors merged commit 04a16ff into rust-lang:master May 16, 2017

kennytm mentioned this pull request Aug 16, 2017

incr.comp.: Crate Metadata not corresponding to a DefId is untracked. #41417

Closed

Make only rustc_trans depend on rustc_llvm #41565

Make only rustc_trans depend on rustc_llvm #41565

Conversation

hanna-kruppe commented Apr 26, 2017 • edited Loading

rust-highfive commented Apr 26, 2017

alexcrichton commented Apr 27, 2017

bors commented May 1, 2017

hanna-kruppe commented May 2, 2017

hanna-kruppe commented May 4, 2017

Mark-Simulacrum commented May 7, 2017

hanna-kruppe commented May 7, 2017 • edited Loading

arielb1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 8, 2017

eddyb May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb May 10, 2017 • edited Loading

Choose a reason for hiding this comment

bors commented May 15, 2017

Mark-Simulacrum commented May 15, 2017

hanna-kruppe commented May 15, 2017

hanna-kruppe commented May 15, 2017

eddyb commented May 15, 2017

bors commented May 15, 2017

bors commented May 15, 2017

bors commented May 15, 2017

alexcrichton commented May 15, 2017

bors commented May 15, 2017

bors commented May 15, 2017

Mark-Simulacrum commented May 15, 2017

bors commented May 15, 2017

bors commented May 15, 2017

alexcrichton commented May 15, 2017

bors commented May 16, 2017

bors commented May 16, 2017

hanna-kruppe commented Apr 26, 2017 •

edited

Loading

hanna-kruppe commented May 7, 2017 •

edited

Loading

eddyb May 10, 2017 •

edited

Loading

eddyb May 10, 2017 •

edited

Loading