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

trans: Exit earlier from base::trans_crate() when compiling rmeta crates. #39184

Merged

Conversation

michaelwoerister
Copy link
Member

Fixes #38964.
r? @eddyb
cc @nrc

@@ -667,7 +667,8 @@ pub fn run_passes(sess: &Session,

// Sanity check
assert!(trans.modules.len() == sess.opts.cg.codegen_units ||
sess.opts.debugging_opts.incremental.is_some());
sess.opts.debugging_opts.incremental.is_some() ||
output_types.contains_key(&OutputType::Metadata));
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that there is no chance that output_types may contain not only Metadata?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just replicating the condition from here:

tcx.sess.opts.output_types.contains_key(&config::OutputType::Metadata) {

It's not an important assertion anyway.

@@ -1145,6 +1145,23 @@ pub fn trans_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
};
let no_builtins = attr::contains_name(&krate.attrs, "no_builtins");

// Skip crate items and just output metadata in -Z no-trans mode.
if tcx.sess.opts.debugging_opts.no_trans ||
tcx.sess.opts.output_types.contains_key(&config::OutputType::Metadata) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather reverse this sort of condition and instead have !output_types.iter().any(|o| o.needs_codegen()).

Copy link
Member Author

Choose a reason for hiding this comment

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

I just so that there's already OutputTypes::should_trans() which does what's needed here. I'll change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe not. What was that with metadata,bin being special?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it looks like that should work.

@eddyb
Copy link
Member

eddyb commented Jan 19, 2017

In general, it'd be neat if you could generate an .o and an .rmeta and package your own .rlib.
Or cargo and/or incremental recompilation could do that to avoid the packaging on intermediary libs.
Linking might be a bit silly but you should be able to do it... somehow.

@michaelwoerister michaelwoerister force-pushed the no-trans-items-for-meta-crates branch from 8020dd9 to b9765c0 Compare January 19, 2017 23:03
@michaelwoerister
Copy link
Member Author

Updated. Both comments should be addressed now.

@@ -1145,6 +1145,23 @@ pub fn trans_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
};
let no_builtins = attr::contains_name(&krate.attrs, "no_builtins");

// Skip crate items and just output metadata in -Z no-trans mode.
if tcx.sess.opts.debugging_opts.no_trans ||
!tcx.sess.opts.output_types.should_trans() {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should even keep -Zno-trans.

@eddyb
Copy link
Member

eddyb commented Jan 19, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Jan 19, 2017

📌 Commit b9765c0 has been approved by eddyb

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 20, 2017
…-meta-crates, r=eddyb

trans: Exit earlier from base::trans_crate() when compiling rmeta crates.

Fixes rust-lang#38964.
r? @eddyb
cc @nrc
@alexcrichton
Copy link
Member

@bors: r-

I believe this causes -Zno-trans to ICE. I've fixed this in the rollup which I hope to land today, though, so it may not be necessary to update this PR.

bors added a commit that referenced this pull request Jan 20, 2017
bors added a commit that referenced this pull request Jan 21, 2017
@bors bors merged commit b9765c0 into rust-lang:master Jan 21, 2017
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.

5 participants