Take commandline arguments into account for incr. comp. #35340

michaelwoerister · 2016-08-04T22:35:46Z

Implements the conservative strategy described in #33727.

From now one, every time a new commandline option is added, one has to specify if it influences the incremental compilation cache. I've tried to implement this as automatic as possible: One just has to added either the [TRACKED] or the [UNTRACKED] marker next to the field. The Options, CodegenOptions, and DebuggingOptions definitions in session::config show plenty of examples.

The PR removes some cruft from session::config::Options, mostly unnecessary copies of flags also present in DebuggingOptions or CodeGenOptions in the same struct.

One notable removal is the cfg field that contained the values passed via --cfg commandline arguments. I chose to remove it because (1) its content is only a subset of what later is stored in hir::Crate::config and it's pretty likely that reading the cfgs from Options would not be what you wanted, and (2) we could not incorporate it into the dep-tracking hash of the Options struct because of how the test framework works, leaving us with a piece of untracked but vital data.

It is now recommended (just as before) to access the crate config via the krate() method in the HIR map.

Because the cfg field is not present in the Options struct any more, some methods in the CompilerCalls trait now take the crate config as an explicit parameter -- which might constitute a breaking change for plugin authors.

rust-highfive · 2016-08-04T22:35:57Z

r? @nikomatsakis

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

nikomatsakis · 2016-08-05T13:34:50Z

src/librustc/session/config.rs

+    use syntax::feature_gate::UnstableFeatures;
+    use syntax::parse::token::InternedString;
+
+    pub trait DepTrackingHash {


for some types, like DefPath, it'd be nice to have access to a tcx -- maybe add that as an argument? (of course they can get it through TLS too, but...)

This trait wasn't really meant to be used for something other than commandline arguments. However, if we decide that we want to use it for the ICH, that would certainly be possible.

michaelwoerister · 2016-08-09T13:36:04Z

cc @nrc regarding CompilerCalls
cc @rust-lang/compiler @rust-lang/tools

nikomatsakis · 2016-08-09T14:15:46Z

src/librustc/session/config.rs

+        debuginfo: DebugInfoLevel [TRACKED],
+        lint_opts: Vec<(String, lint::Level)> [TRACKED],
+        lint_cap: Option<lint::Level> [TRACKED],
+        describe_lints: bool [UNTRACKED],


ps I love this [UNTRACKED] notation. Such a beautiful use of macros.

That said, this is a good example of the backwards compat sort of thing that macros are supposed to disallow. ;)

That said, this is a good example of the backwards compat sort of thing that macros are supposed to disallow. ;)

To clarify: imagine if types were extended to support T[...]; in that case, this would not parse anymore. Personally I think it's fine to leave it as is, but you could also add some separator token like ; or ,.

So it turns out that there's almost no common character allowed directly after a ty production :)
; and , probably would be allowed, but I think they are kind of misleading in this context. I'll just leave it the way it is. We can always change it easily if need be.

michaelwoerister · 2016-08-09T15:31:38Z

@jonas-schievink might also be interested in this :)

nikomatsakis · 2016-08-09T16:21:32Z

src/librustc/session/config.rs

+        ($t:ty) => (
+            impl DepTrackingHash for Vec<$t> {
+                fn hash(&self, hasher: &mut SipHasher, error_format: ErrorOutputType) {
+                    let mut elems = self.clone();


Oh man. I guess this is good but kind of sucks.

Not that I have a better idea =)

ps seems mildly better to make a vector of indices and sort that:

let mut elems: Vec<_> = (0..self.len()).collect(); elems.sort_by(|i, j| self[i].cmp(&self[j])); for (index, original_index) in elems.iter().enumerate() { self[original_index].hash(); }

Right :)
On the other hand, this code is executed exactly once for a tiny amount of data...

nikomatsakis · 2016-08-09T17:19:10Z

r=me modulo nits and resolving the questions that @alexcrichton raised =)

alexcrichton · 2016-08-09T18:00:44Z

I think all my points are resolved, I didn't realize that incrementality was only up to codegen and linking wasn't incremental. In that case I think this is good to go modulo taking native libraries into account which affect dllimport on MSVC

michaelwoerister · 2016-08-09T18:25:53Z

... and linking wasn't incremental

It will hopefully be so at least for gold users. But that has little to do with us (except for passing the right flags to the linker).

EDIT: And the MSVC linker can do that too, I think.

michaelwoerister · 2016-08-09T19:30:11Z

@bors r=nikomatsakis

I think I've addressed all the nits.

bors · 2016-08-09T19:30:13Z

📌 Commit 845e37b has been approved by nikomatsakis

michaelwoerister · 2016-08-09T19:32:25Z

Regarding search_path and externs, we'll have to make sure that we detect changes in the library files themselves. Most of it should be caught be Metadata hashes already.

bors · 2016-08-11T08:59:10Z

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

Commandline arguments influence whether incremental compilation can use its compilation cache and thus their changes relative to previous compilation sessions need to be taking into account. This commit makes sure that one has to specify for every commandline argument whether it influences incremental compilation or not.

.. and use it to purge the incremental compilation cache if necessary.

The 'cfg' in the Options struct is only the commandline-specified subset of the crate configuration and it's almost always wrong to read that instead of the CrateConfig in HIR crate node.

michaelwoerister · 2016-08-11T14:58:01Z

OK, I could reproduce the test failure after a rebase (I guess it contained a change that bors' merge already had too), and it seems that there's something wrong with either macros or extern crates: If the main function in the test contains a println!() then the test fails:

module WorkProduct(WorkProductId("commandline_args")) is dirty because Hir("commandline_args/45447b7afbd5e544f7d0f1df0fccd26014d9850130abd3f020b89ff96b82079f-exe::main[0]") changed or was removed

Without the println! everything works as expected...

michaelwoerister · 2016-08-11T15:19:15Z

@bors r=nikomatsakis

bors · 2016-08-11T15:19:16Z

📌 Commit 67f19e5 has been approved by nikomatsakis

bors · 2016-08-12T13:24:46Z

⌛ Testing commit 67f19e5 with merge eddcb8d...

…sakis Take commandline arguments into account for incr. comp. Implements the conservative strategy described in #33727. From now one, every time a new commandline option is added, one has to specify if it influences the incremental compilation cache. I've tried to implement this as automatic as possible: One just has to added either the `[TRACKED]` or the `[UNTRACKED]` marker next to the field. The `Options`, `CodegenOptions`, and `DebuggingOptions` definitions in `session::config` show plenty of examples. The PR removes some cruft from `session::config::Options`, mostly unnecessary copies of flags also present in `DebuggingOptions` or `CodeGenOptions` in the same struct. One notable removal is the `cfg` field that contained the values passed via `--cfg` commandline arguments. I chose to remove it because (1) its content is only a subset of what later is stored in `hir::Crate::config` and it's pretty likely that reading the cfgs from `Options` would not be what you wanted, and (2) we could not incorporate it into the dep-tracking hash of the `Options` struct because of how the test framework works, leaving us with a piece of untracked but vital data. It is now recommended (just as before) to access the crate config via the `krate()` method in the HIR map. Because the `cfg` field is not present in the `Options` struct any more, some methods in the `CompilerCalls` trait now take the crate config as an explicit parameter -- which might constitute a breaking change for plugin authors.

bors · 2016-08-12T15:58:54Z

💔 Test failed - auto-linux-64-debug-opt

sophiajt · 2016-08-12T16:01:47Z

@bors retry

michaelwoerister · 2016-08-12T16:01:48Z

@bors retry

This seems spurious ...

eddyb · 2016-08-12T16:08:15Z

@alexcrichton Can we increase the timeout? And/or add RUSTFLAGS_STAGE2=-Ztime-passes to debug-opt.

alexcrichton · 2016-08-12T16:09:49Z

I've not been watching bors over the past week or so, can we open an issue about this? It would be good to have all information related to this in one issue (I don't know the context here)

michaelwoerister · 2016-08-12T22:43:01Z

See #35628 for the timeout.

bors · 2016-08-13T05:42:47Z

⌛ Testing commit 67f19e5 with merge d6ae9eb...

bors · 2016-08-13T08:13:09Z

💔 Test failed - auto-linux-64-debug-opt

michaelwoerister · 2016-08-15T13:37:01Z

@bors retry

bors · 2016-08-15T15:35:18Z

⌛ Testing commit 67f19e5 with merge f65d96f...

…sakis Take commandline arguments into account for incr. comp. Implements the conservative strategy described in #33727. From now one, every time a new commandline option is added, one has to specify if it influences the incremental compilation cache. I've tried to implement this as automatic as possible: One just has to added either the `[TRACKED]` or the `[UNTRACKED]` marker next to the field. The `Options`, `CodegenOptions`, and `DebuggingOptions` definitions in `session::config` show plenty of examples. The PR removes some cruft from `session::config::Options`, mostly unnecessary copies of flags also present in `DebuggingOptions` or `CodeGenOptions` in the same struct. One notable removal is the `cfg` field that contained the values passed via `--cfg` commandline arguments. I chose to remove it because (1) its content is only a subset of what later is stored in `hir::Crate::config` and it's pretty likely that reading the cfgs from `Options` would not be what you wanted, and (2) we could not incorporate it into the dep-tracking hash of the `Options` struct because of how the test framework works, leaving us with a piece of untracked but vital data. It is now recommended (just as before) to access the crate config via the `krate()` method in the HIR map. Because the `cfg` field is not present in the `Options` struct any more, some methods in the `CompilerCalls` trait now take the crate config as an explicit parameter -- which might constitute a breaking change for plugin authors.

bors · 2016-08-15T18:34:59Z

rust-highfive assigned nikomatsakis Aug 4, 2016

nikomatsakis reviewed Aug 5, 2016
View reviewed changes

michaelwoerister force-pushed the incr-comp-cli-args branch 2 times, most recently from f7af64e to a61e3ff Compare August 9, 2016 13:12

michaelwoerister added T-tools T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 9, 2016

michaelwoerister changed the title ~~WIP: Take commandline arguments into account for incr. comp.~~ Take commandline arguments into account for incr. comp. Aug 9, 2016

nikomatsakis reviewed Aug 9, 2016
View reviewed changes

michaelwoerister added 4 commits August 11, 2016 09:56

Save dep-tracking hash of commandline arguments in dep-graph file.

d3578ab

.. and use it to purge the incremental compilation cache if necessary.

Remove the 'cfg' field from session::config::Options.

65eb024

The 'cfg' in the Options struct is only the commandline-specified subset of the crate configuration and it's almost always wrong to read that instead of the CrateConfig in HIR crate node.

Address comments by reviewers.

a7de0bc

michaelwoerister added 2 commits August 11, 2016 10:59

Fix incremental/commandline-args test.

3a70aa5

Always add -Z incremental-info to incremental compilation tests.

67f19e5

michaelwoerister force-pushed the incr-comp-cli-args branch from 26fd699 to 67f19e5 Compare August 11, 2016 15:00

bors merged commit 67f19e5 into rust-lang:master Aug 15, 2016

This was referenced Aug 15, 2016

Change description of no-stack-check (#34915) #35156

Closed

Fix debug line number info for macro expansions. #35238

Merged

michaelwoerister mentioned this pull request Aug 16, 2016

Take command line arguments into account when doing incremental compilation #33727

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take commandline arguments into account for incr. comp. #35340

Take commandline arguments into account for incr. comp. #35340

michaelwoerister commented Aug 4, 2016 •

edited

Loading

rust-highfive commented Aug 4, 2016

nikomatsakis Aug 5, 2016

michaelwoerister Aug 5, 2016

michaelwoerister commented Aug 9, 2016

nikomatsakis Aug 9, 2016

nikomatsakis Aug 9, 2016

michaelwoerister Aug 9, 2016

michaelwoerister commented Aug 9, 2016

nikomatsakis Aug 9, 2016

nikomatsakis Aug 9, 2016

nikomatsakis Aug 9, 2016 •

edited

Loading

michaelwoerister Aug 9, 2016

nikomatsakis commented Aug 9, 2016

alexcrichton commented Aug 9, 2016

michaelwoerister commented Aug 9, 2016 •

edited

Loading

michaelwoerister commented Aug 9, 2016

bors commented Aug 9, 2016

michaelwoerister commented Aug 9, 2016

bors commented Aug 11, 2016

michaelwoerister commented Aug 11, 2016

michaelwoerister commented Aug 11, 2016

bors commented Aug 11, 2016

bors commented Aug 12, 2016

bors commented Aug 12, 2016

sophiajt commented Aug 12, 2016

michaelwoerister commented Aug 12, 2016

eddyb commented Aug 12, 2016

alexcrichton commented Aug 12, 2016

michaelwoerister commented Aug 12, 2016

bors commented Aug 13, 2016

bors commented Aug 13, 2016

michaelwoerister commented Aug 15, 2016

bors commented Aug 15, 2016

bors commented Aug 15, 2016

Take commandline arguments into account for incr. comp. #35340

Take commandline arguments into account for incr. comp. #35340

Conversation

michaelwoerister commented Aug 4, 2016 • edited Loading

rust-highfive commented Aug 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented Aug 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented Aug 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Aug 9, 2016

alexcrichton commented Aug 9, 2016

michaelwoerister commented Aug 9, 2016 • edited Loading

michaelwoerister commented Aug 9, 2016

bors commented Aug 9, 2016

michaelwoerister commented Aug 9, 2016

bors commented Aug 11, 2016

michaelwoerister commented Aug 11, 2016

michaelwoerister commented Aug 11, 2016

bors commented Aug 11, 2016

bors commented Aug 12, 2016

bors commented Aug 12, 2016

sophiajt commented Aug 12, 2016

michaelwoerister commented Aug 12, 2016

eddyb commented Aug 12, 2016

alexcrichton commented Aug 12, 2016

michaelwoerister commented Aug 12, 2016

bors commented Aug 13, 2016

bors commented Aug 13, 2016

michaelwoerister commented Aug 15, 2016

bors commented Aug 15, 2016

bors commented Aug 15, 2016

michaelwoerister commented Aug 4, 2016 •

edited

Loading

nikomatsakis Aug 9, 2016 •

edited

Loading

michaelwoerister commented Aug 9, 2016 •

edited

Loading