add --crate-type metadata #37681

nrc · 2016-11-10T05:30:48Z

alexcrichton

Looks great to me, thanks @nrc!

The only major thing I see is whether the format here is an rlib with a metadata file or just the metadata file itself (I'd sorta prefer the latter).

Can you also add a test with some native libraries? That is, despite whatever #[link] directives are found, they're all ignored when generating a metadata crate type.

Also, can you add tests which emit both an rlib and a metadata crate? Just to make sure nothing explodes too badly there.

alexcrichton · 2016-11-10T19:36:40Z

src/librustc/session/config.rs

@@ -1632,6 +1634,7 @@ impl fmt::Display for CrateType {
            CrateTypeStaticlib => "staticlib".fmt(f),
            CrateTypeCdylib => "cdylib".fmt(f),
            CrateTypeProcMacro => "proc-macro".fmt(f),
+            CrateTypeMetadata => "rmeta".fmt(f),


s/rmeta/metadata/

alexcrichton · 2016-11-10T19:37:25Z

src/librustc_metadata/creader.rs

@@ -750,7 +753,7 @@ impl<'a> CrateLoader<'a> {
        };
        info!("panic runtime not found -- loading {}", name);

-        let (cnum, data, _) = self.resolve_crate(&None, name, name, None,
+        let (cnum, data) = self.resolve_crate(&None, name, name, None,
                                                 syntax_pos::DUMMY_SP,


(left indent)

alexcrichton · 2016-11-10T19:41:52Z

src/librustc_metadata/locator.rs

@@ -53,6 +53,13 @@
 //! is a platform-defined dynamic library. Each library has a metadata somewhere
 //! inside of it.
 //!
+//! A third kind of dependency is an rmeta file. These are rlibs, which contain


Having read through this file, everything looks good. I might expect though that libfoo.rmeta is not an archive but rather just the literal metadata file in the rlib normally itself. This'd avoid going through LLVM's archive support which may speed things up a bit, but probably won't have that much effect on perf.

I'm not sure if overall it'd be less code as well, just a thought!

alexcrichton · 2016-11-10T19:42:59Z

src/librustc_trans/back/link.rs

@@ -190,6 +189,7 @@ pub fn link_binary(sess: &Session,
    let mut out_filenames = Vec::new();
    for &crate_type in sess.crate_types.borrow().iter() {
        // Ignore executable crates if we have -Z no-trans, as they will error.
+        // TODO do we need to check for CrateTypeMetadata here?


Presumably CrateTypeMetadata halts before the linking stage, right? So we should just skip it here?

alexcrichton · 2016-11-10T19:44:12Z

src/librustc_trans/back/link.rs

@@ -344,7 +350,7 @@ fn link_binary_output(sess: &Session,
    };

    match crate_type {
-        config::CrateTypeRlib => {
+        config::CrateTypeRlib | config::CrateTypeMetadata => {


Hm so this is where I start to get a little uneasy, link_rlib has tons of logic for object files, native libraries, etc. Perhaps it'd be clearer and/or easier to just emit the metadata here to a dedicated file?

alexcrichton · 2016-11-10T19:45:00Z

src/librustc_trans/back/write.rs

-            sess.opts.output_types.contains_key(&OutputType::Exe);
+            (sess.crate_types.borrow().contains(&config::CrateTypeRlib) &&
+             sess.opts.output_types.contains_key(&OutputType::Exe)) ||
+            sess.crate_types.borrow().contains(&config::CrateTypeMetadata);


We don't want bitcode for metadata, right? Just the metadata file?

alexcrichton · 2016-11-10T19:45:09Z

src/librustc_trans/back/write.rs

    let needs_crate_object =
-            sess.opts.output_types.contains_key(&OutputType::Exe);
+            sess.opts.output_types.contains_key(&OutputType::Exe) ||
+            sess.crate_types.borrow().contains(&config::CrateTypeMetadata);


(same as above)

alexcrichton · 2016-11-10T19:46:18Z

src/test/compile-fail/rmeta-lib-pass.rs

+
+// aux-build:rmeta_rlib.rs
+// no-prefer-dynamic
+// must-compile-successfully


Whoa, I had no idea this existed!

alexcrichton · 2016-11-10T19:48:55Z

cc @rust-lang/tools as well, a new crate type!

@eddyb did you want to hold off on merging until the compiler team has a chance to talk about this as well?

eddyb · 2016-11-10T20:13:26Z

Yeah, ~~although we might not be having enough of a meeting today~~.
My main concern is that crate types are usually user-visible while this is a "mode".
That said, there is no real difference between an rlib and a dylib as far as metadata goes, so "metadata-only library" works for me. It'd be interesting to see how tools could use a version of this with all MIR.

For example, miri could evaluate everything (possibly emulating FFI) and other potential backends don't really have a concept of "library" or "linking" and rather do codegen only once, for the final "executable".

OTOH, the metadata file is part of an rlib like an object file, and you can get the latter through --emit.
I'm not fully decided either way, partly because --emit and "crate types" are not fully orthogonal.

michaelwoerister · 2016-11-10T20:20:49Z

Another question would be whether to include all MIR of the crate or none at all, maybe? Right now, this will omit MIR for those functions that would have been translated to machine code if this was an rlib, which does not make so much sense any more in the new setting.

alexcrichton · 2016-11-10T20:27:25Z

@eddyb yeah I'm not sure if this is most appropriate as an --emit or just a new crate type, but a new crate type seems fine to me (it'll compile down to the same thing essentially).

I'm not sure I follow the bits about MIR/miri personally, but so long as it serves the problem of "fast compilation which doesn't require linkers" seems plausible to me!

@michaelwoerister I'd personally expect MIR to be omitted if we can, in the sense that I'm thinking metadata crates are purely used for static analysis and/or checking compiles. If MIR is needed for that it's included but if not then it's omitted.

eddyb · 2016-11-10T20:28:37Z

You need at least MIR for constants, now and const fn in the future, even without any codegen.

michaelwoerister · 2016-11-10T20:46:20Z

If we only include the minimal set of MIR needed for static analysis, we would be able to completely skip MIR optimization passes, translation item collection, and codegen unit partitioning. That might be a worthwhile goal for the future.

alexcrichton · 2016-11-10T20:47:13Z

Indeed! I'd imagine that we optimize metadata-generation to be as fast as possible and skip as much as possible in the compiler

nrc · 2016-11-10T21:06:55Z

I'd like to raise the question of stability here - to what extent can we change the contents of the output of a stable option like this? For obvious reasons I'd like to land this asap, but it sounds like there a things we could improve in the future around metadata, etc.

On --emit vs --crate-type. I think this is a user-facing change if the user is using the compiler directly. I agree the two options are not at all orthogonal. I believe in the current model, --crate-type makes sense because we are changing the kind of crate emitted, but not really changing anything else.

eddyb · 2016-11-10T21:12:37Z

@nrc You probably want to enforce that no codegen happens in dependent crates, i.e. metadata crates can only be used to build more metadata crates, and only expand this in the future if we want to.

nrc · 2016-11-10T21:30:24Z

@eddyb is that not a Cargo issue, rather than a rustc one? We don't do any codegen for dep crates in any case do we?

alexcrichton · 2016-11-10T22:09:12Z

@nrc I feel like the stability here is the same as rlibs, which is to say that you're required to treat it as a black box from the outside and it's only guaranteed to work with one compiler. Put another way, there's zero stability but it hopefully doesn't have any bugs :)

eddyb · 2016-11-10T22:25:21Z

After the compiler meeting, I'm pretty much on board with this approach, even if it overlaps with both --emit and future incremental recompilation workdir structure, it's still relevant in its own right.

As long as it cannot be misused from dependent crates and there's no stabilization schedule, SGTM.

nrc · 2016-11-10T23:07:29Z

@eddyb it would be insta-stable (at least the option would be, sounds like the contents would be perma-unstable).

nrc · 2016-11-15T22:32:34Z

Updated to use a raw metadata file, rather than a degenerate rlib.

And added a test

alexcrichton · 2016-11-16T15:52:34Z

src/librustc_metadata/locator.rs

@@ -845,6 +860,15 @@ fn get_metadata_section_imp(target: &Target,
                Ok(blob)
            }
        };
+    } else if flavor == CrateFlavor::Rmeta {
+        let mut file = File::open(filename).map_err(|_|
+            format!("could not open file: '{}'", filename.display()))?;


Could this include the reason it failed to open as well? (e.g. the argument to map_err)

I was basically following the errors from the rlib branch directly above here. Those follow the same pattern (ignoring the actual error), although in that case because the error gets converted into an Option in the meantime. So I could (c.f., rlibs) add the extra info here, but it would be inconsistent with the rlib version.

Oh ok, I guess whatever is fine. We shouldn't ever throw away the error context, but it's not really that critical.

alexcrichton · 2016-11-16T15:53:51Z

Looks good to me! r=me assuming compiler team's OK w/ this as well

bors · 2016-11-16T19:48:30Z

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

msiglreith · 2016-11-18T10:33:26Z

Just a quick question as I'm quite interested in this crate type: Will it contain all MIR as rlibs exclude some parts afaik (e.g https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/encoder.rs#L826-L834)?

nrc · 2016-11-18T19:12:37Z

Just a quick question as I'm quite interested in this crate type: Will it contain all MIR as rlibs exclude some parts afaik (e.g https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/encoder.rs#L826-L834)?

No, it contains exactly the same metadata that an rlib would contain.

With the same semantics as -Zno-trans

nrc · 2016-11-20T18:23:30Z

@bors: r= @alexcrichton

bors · 2016-11-20T18:23:33Z

📌 Commit d038ebc has been approved by ``

nrc · 2016-11-21T20:46:34Z

@bors: r=@alexcrichton

bors · 2016-11-21T20:46:35Z

📌 Commit d038ebc has been approved by @alexcrichton

bors · 2016-11-22T06:33:54Z

⌛ Testing commit d038ebc with merge 084ed2a...

bors · 2016-11-22T07:47:49Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

sanxiyn · 2016-11-22T09:45:49Z

Travis failed tidy. "/checkout/src/librustc_metadata/locator.rs:735: line longer than 100 chars".

nrc · 2016-11-22T23:51:49Z

@bors: r=@alexcrichton

bors · 2016-11-22T23:52:02Z

📌 Commit af1b195 has been approved by @alexcrichton

bors · 2016-11-23T03:54:10Z

⌛ Testing commit af1b195 with merge 5196ca8...

@alexcrichton

add --crate-type metadata r? @alexcrichton

bors · 2016-11-23T07:21:43Z

brson · 2016-11-29T04:32:16Z

This looks like it is insta-stable. Why is it not feature gated?

nrc · 2016-11-29T05:05:18Z

This looks like it is insta-stable. Why is it not feature gated?

it does the same thing as -Zno-trans which has been around forever
the data emitted is not stable and can be changed at any time
the only point of this (c.f., just using -Zno-trans) is for it to be stable
it was proposed and accepted at the tools team meeting as a stable feature (apologies if the insta-stable-ness of it was not communicated well enough)
the motivation here is to have something Cargo can use, and Cargo only uses stable features.

@alexcrichton

cargo check ~~This is not finished - the big omission is no tests, and it needs some more testing too. It also requires rust-lang/rust#37681 However, this is my first non-trivial Cargo patch, so I'd like to get some early feedback. r? @alexcrichton and cc @rust-lang/tools since this adds a new Cargo sub-command (although we have previously discussed and approved the idea in a tools meeting).

rust-highfive assigned alexcrichton Nov 10, 2016

nrc mentioned this pull request Nov 10, 2016

Add --emit metadata #37411

Closed

eddyb added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 10, 2016

alexcrichton reviewed Nov 10, 2016

View reviewed changes

nrc force-pushed the crate-metadata branch from 768d089 to 22dc7e3 Compare November 10, 2016 21:01

nikomatsakis removed the I-nominated label Nov 10, 2016

eddyb added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 11, 2016

nrc force-pushed the crate-metadata branch from 22dc7e3 to 7f0d56a Compare November 15, 2016 22:31

alexcrichton reviewed Nov 16, 2016

View reviewed changes

nrc mentioned this pull request Nov 16, 2016

cargo check rust-lang/cargo#3296

Merged

Add --crate-type metadata

b286a2f

With the same semantics as -Zno-trans

nrc added 2 commits November 21, 2016 07:10

Read in rmeta crates

534556a

Tests

baedc3b

nrc force-pushed the crate-metadata branch from 7f0d56a to d038ebc Compare November 20, 2016 18:23

Rebasing and review changes

af1b195

nrc force-pushed the crate-metadata branch from d038ebc to af1b195 Compare November 22, 2016 23:51

bors added a commit that referenced this pull request Nov 23, 2016

Auto merge of #37681 - nrc:crate-metadata, r=@alexcrichton

5196ca8

add --crate-type metadata r? @alexcrichton

bors merged commit af1b195 into rust-lang:master Nov 23, 2016

bors mentioned this pull request Nov 23, 2016

[RFC] Add flag for Unicode based Frontend support #37703

Closed

add --crate-type metadata #37681

add --crate-type metadata #37681

Conversation

nrc commented Nov 10, 2016

alexcrichton 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Nov 10, 2016

eddyb commented Nov 10, 2016 • edited Loading

michaelwoerister commented Nov 10, 2016

alexcrichton commented Nov 10, 2016

eddyb commented Nov 10, 2016

michaelwoerister commented Nov 10, 2016

alexcrichton commented Nov 10, 2016

nrc commented Nov 10, 2016

eddyb commented Nov 10, 2016

nrc commented Nov 10, 2016

alexcrichton commented Nov 10, 2016

eddyb commented Nov 10, 2016

nrc commented Nov 10, 2016

nrc commented Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Nov 16, 2016

bors commented Nov 16, 2016

msiglreith commented Nov 18, 2016

nrc commented Nov 18, 2016

nrc commented Nov 20, 2016

bors commented Nov 20, 2016

nrc commented Nov 21, 2016

bors commented Nov 21, 2016

bors commented Nov 22, 2016

bors commented Nov 22, 2016

sanxiyn commented Nov 22, 2016

nrc commented Nov 22, 2016

bors commented Nov 22, 2016

bors commented Nov 23, 2016

bors commented Nov 23, 2016

brson commented Nov 29, 2016

nrc commented Nov 29, 2016

eddyb commented Nov 10, 2016 •

edited

Loading

nrc commented Nov 15, 2016 •

edited

Loading