Skip to content
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

rustc: --emit=rlibmeta outputs an rlib with only metadata, works with -Z no-trans. #26234

Closed
wants to merge 2 commits into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 12, 2015

Useful for running analysis passes on downstream crates without actually doing codegen.
cc @bstrie who wants to use this for a cargo check command.

@rust-highfive
Copy link
Collaborator

r? @Aatch

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

@alexcrichton
Copy link
Member

Nice! This feels like it's a bit ad-hoc though and it'd unfortunately be an insta-stable interface to the compiler. For something like cargo check I don't think this is necessary as 99% of the time all your dependencies are already built, but I've wanted this for other aspects such as documenting other platforms and/or pipelining compilations.

Some specific concerns I have are:

  • This just uses the same rlib format, but the object file is notably missing. This can lead to surprising errors if you try to actually link against those rlibs. I imagined this would create a whole new separate file type (e.g. *.rmeta) which the compiler would then give a first-class error message about not being able to link against if that is requested.
  • The relationship between -Z no-trans and --emit=rmeta is somewhat odd to me, I would expect the compiler to naturally stop at the right point if only --emit=rmeta is present (e.g. as it does with other output types today).
  • Implementation-wise I'd prefer to see rmeta have a more first-class position instead of being a few targeted if-else conditionals on the normal rlib path as these two forms may diverge over time.

@Aatch
Copy link
Contributor

Aatch commented Jun 12, 2015

r? @alexcrichton I've got no opinion on the design issues.

@rust-highfive rust-highfive assigned alexcrichton and unassigned Aatch Jun 12, 2015
@eddyb
Copy link
Member Author

eddyb commented Jun 12, 2015

... So GitHub/chromium managed to lose my comment, and I'm not retyping all that.
The important thing is that I don't want this merged in its current state, I've barely begun understanding the code involved here.
But I do want @bstrie to asses whether this would work for cargo check.

The reason -Z no-trans isn't "implied" by --emit=rlibmeta is because the metadata actually changes after trans, as it gets symbol names and a few other things (that only influence trans in downstream crates).

Should this kind of metadata always be emitted before trans, with certain information missing?
Maybe the extra information could be split out into a different bit of metadata that is emitted after trans?

That would make sense if it was going to be used to pipeline compilations.

@alexcrichton
Copy link
Member

But I do want @bstrie to asses whether this would work for cargo check.

With rust-lang/cargo#1701, I believe that cargo check is basically cargo rustc -- -Z no-trans, so I'd recommend starting to investigate there (before taking this route)

The reason -Z no-trans isn't "implied" by --emit=rlibmeta is because the metadata actually changes after trans, as it gets symbol names and a few other things (that only influence trans in downstream crates).

Interesting! I had forgotten about this. This poses an interesting problem that may be difficult to overcome, unfortunately. In terms of pipelining compilations, there are two synchronization points which need to happen if we have two sets of metadata:

  1. Once the first phase of metadata is produced, immediate downstream dependents can start compiling.
  2. Once I'm ready to trans, all upstream dependencies need to have produced the second phase of metadata before I can continue.

I feel like managing multiple synchronization points in the compiler could get quite complicated (even one is nontrivial), and if we take a route like this I'd much prefer to minimize the complexity as much as possible.

In the ideal world we could do something like have a "dry run" of trans which doesn't actually translate anything (but calculates symbol names), or perhaps even just have the ability to calculate symbol names ahead of trans. That way there'd only need to be one metadata format that's the same across dylibs, rlibs, and standalone files.

@eddyb
Copy link
Member Author

eddyb commented Jun 12, 2015

Ah, that's a good point, I had forgotten how deterministic everything should be - maybe it can be computed ahead of trans (and... cached, so trans is more efficient at it. may also be possible to run trans from the metadata alone).

@alexcrichton
Copy link
Member

I would also be somewhat more comfortable landing this ahead of time if the ability to emit a metadata file is tied to the ability to use unstable features (e.g. this is only enabled on nightly).

@bstrie
Copy link
Contributor

bstrie commented Jun 13, 2015

Ideally I'd also like cargo check to be a precursor to allowing dependents to begin compiling while their dependencies are still being codegenned, which to me implies that we'd want separate files for metadata and linkable artifacts. Alex, in the past when I've asked you about this you seemed to agree that separate files was a good idea, and that we might want to consider making separate files the normal mode of compilation regardless (though I could be misremembering).

@@ -785,7 +787,7 @@ pub fn rustc_short_optgroups() -> Vec<RustcOptGroup> {
"NAME"),
opt::multi("", "emit", "Comma separated list of types of output for \
the compiler to emit",
"[asm|llvm-bc|llvm-ir|obj|link|dep-info]"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manpage must be updated too.

@nagisa
Copy link
Member

nagisa commented Jun 13, 2015

Bikeshed: Is rlibmeta somehow specific to rlibs? Don’t we bundle metadata with shared libraries as well? If so, could this be named just --emit=metadata instead?

@alexcrichton
Copy link
Member

Alex, in the past when I've asked you about this you seemed to agree that separate files was a good idea, and that we might want to consider making separate files the normal mode of compilation regardless (though I could be misremembering).

Yeah I definitely think we'll want a separate and dedicated file for this, but I don't think we should be producing one in all cases. It's quite nice that an rlib and/or dylib is a standalone artifact that doesn't need to be paired with something else, so I'd prefer to keep it like that if possible.

Don’t we bundle metadata with shared libraries as well? If so, could this be named just --emit=metadata instead?

Good point! I'd also be fine with --emit=rmeta (rust metadata)

@eddyb
Copy link
Member Author

eddyb commented Jun 13, 2015

@nagisa shared libraries use compressed metadata - there was also "rlib" in there because, well, it's a "valid" rlib right now (if you don't try linking with it).
Definitely not keeping "rlibmeta" or the whole archive around the metadata.

@bors
Copy link
Contributor

bors commented Jun 19, 2015

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

@alexcrichton
Copy link
Member

Closing out of inactivity now, but I'm excited to see where this goes in the future!

@eddyb
Copy link
Member Author

eddyb commented Aug 18, 2015

I just realized that crater might have an use for this. Could be a good reason to revive the branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants