Incremental compilation auto assert (with except) #45104

vitiral · 2017-10-08T15:32:49Z

bors merged part 1, so this is a WIP of part 2 of #45009 -- auto asserting DepNodes depending on the type of node rustc_clean/dirty is attached to

Framework:

finish auto-detection for specified DepNodes
finish auto-detection for remaining DepNodes

Test Refactors:

rust-highfive · 2017-10-08T15:32:54Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

vitiral · 2017-10-08T15:39:29Z

@michaelwoerister let me know how "complete" you want the DepNode detection. I'm thinking of leavig it as a WIP.

FIXME: I need to do better assertions (using the span_fatal or something) so that the compiler gives a helpful error message on an invalid node.

michaelwoerister · 2017-10-09T12:40:18Z

I think this looks very good. If you have time to keep working on it, I would suggest implementing support for other kinds of items (methods/associated items, type definitions) in the auto-detection code now. Since individual PRs can take quite a while to go from approval to merging, it would be better to do everything in one larger PR, I think.

If you think you won't find the time or motivation to finish this in the short-term then we can also just land the current state and I (or someone else) would take it from there.

But I don't think there's that much work left.

michaelwoerister · 2017-10-09T12:42:54Z

src/librustc_incremental/persist/dirty_clean.rs

+
+// Base and Extra labels to build up the labels
+
+/// DefNodes for Hir, which is pretty much everything


It's DepNode, not DefNode. It is, however, DefPath, which can be rather confusing. "Dep" is for "dependency", "Def" is for "definition".

vitiral · 2017-10-09T13:47:44Z

@michaelwoerister no, I can flush it out. I'll have to take several of the test-case refactors though, hopefully that is okay with you :)

michaelwoerister · 2017-10-09T13:48:47Z

@vitiral O sure, no problem!

vitiral · 2017-10-09T18:52:44Z

@michaelwoerister I'm getting pretty close to groups for all of these and will push up shortly. There is a major difference that I did: I split up const, impl const and trait const and have different labels for each. Please let me know if this was incorrect

vitiral · 2017-10-09T19:25:22Z

@michaelwoerister can you specify which DepNodes to assert for:

mod declaration
external crate
external trait
foreign (external?) item
global asm

michaelwoerister · 2017-10-10T08:46:52Z

I split up const, impl const and trait const and have different labels for each. Please let me know if this was incorrect.

Looks correct to me.

michaelwoerister · 2017-10-10T09:01:44Z

mod declaration

BASE_HIR

external crate

None. These declarations are more or less erased before incremental compilation kicks in.

external trait

I don't know what you mean by that.

foreign (external?) item

Currently, only the entire extern block is checked by the visitor. For that, BASE_HIR should be fine. In the future we'll want to extend checking to the individual declarations within extern blocks. That might be something for another PR though (that then also covers struct/enum/union fields).

global asm

BASE_HIR

vitiral · 2017-10-10T13:37:02Z

What about `use` declarations?

michaelwoerister · 2017-10-10T13:39:42Z

@vitiral use declarations should be the same as extern crate.

vitiral · 2017-10-10T16:55:14Z

leaving a comment here: hit a issue which causes ICE when testing trait declarations. @michaelwoerister and I are looking into ways to solve.

arielb1 · 2017-10-11T11:23:27Z

r? @michaelwoerister

vitiral · 2017-10-13T00:08:32Z

I wanted to put the script I am using to make this easier in case anyone finds it useful. Once this merges I will post it on the main issue:

from __future__ import print_function
import os
import sys
import json

header = '''\
------------------------------------------
stderr:
------------------------------------------\
'''

text = sys.stdin.read()

if "internal compiler error" in text:
    print(text)
    print("!! ICE, ICE, Baby... can't touch that !!")
    sys.exit(1)

_, text = text.split(header)
text = text.split('\n')

current_file = ''
current_line = -1
current_nodes = []

for l in text:
    try:
        data = json.loads(l)
    except Exception:
        print("~~ ", l)
        continue
    try:
        span = data['spans'][0]
        file = os.path.basename(span['file_name'])
        msg = data['message']
        node = msg[1:].split('(')[0]
        line = span['line_start']

        # print('-', line, msg)
        if file != current_file or line != current_line:
            if line != -1:
                print(current_file, current_line, ','.join(sorted(current_nodes)))
            current_line = line
            current_file = file
            current_nodes = []

        current_nodes.append(node)

    except (KeyError, IndexError) as e:
        print("~~ OtherError:", e, l)

I run it by changing all assertions for the file I want to fix to rustc_clean(cfg=...) (I do this through a vim macro). I then run:

./x.py test --stage 1 src/test/incremental | python3 parse_stderr.py

And the output looks like:

type_defs.rs 41 Hir,HirBody,TypeOfItem                                                     
type_defs.rs 53 Hir,HirBody,TypeOfItem
type_defs.rs 65 Hir,HirBody,TypeOfItem
...

while is <file> <line#> <what except should be>

vitiral · 2017-10-13T00:30:16Z

@michaelwoerister I made the changes you requested and am seeing 32/34 failed with things like this:

{"message":"src/librustc/ty/maps/plumbing.rs:712: force_from_dep_node() - Encountered HirBody","code":null,"level":"error: internal compiler error","spans":[],"children":[],"rendered":null}
note: the compiler unexpectedly panicked. this is a bug.                                  
                                                                                                                                      
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
                                                                                          
note: rustc 1.22.0-dev running on x86_64-unknown-linux-gnu                                                                                                                                                                                                                                                                                                                                                                            note: run with `RUST_BACKTRACE=1` for a backtrace                                                                                                                                                                                                                                                                                                                                                                                     incremental: session directory: 8 files hard-linked                                                                                                                                                                incremental: session directory: 0 files copied                                                                                                                                                                     thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:492:8                                          
stack backtrace:                                   
   0:     0x7fc01699a822 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::h2bee036dd609b02b
   1:     0x7fc01699505b - std::sys_common::backtrace::_print::h2a9bd6df49be92d1
   2:     0x7fc0169a8119 - std::panicking::default_hook::{{closure}}::hcae6c022a3505fce                            
   3:     0x7fc0169a7df5 - std::panicking::default_hook::h2b2460f26d5a082b
   4:     0x7fc0169a8628 - std::panicking::rust_panic_with_hook::hf4116880dbeebeea
   5:     0x7fc012445408 - std::panicking::begin_panic::hd4dfc6e452a13272                                                      
   6:     0x7fc012462f25 - rustc_errors::Handler::bug::hfb6778b3fbcffb64
   7:     0x7fc0135659b0 - rustc::session::opt_span_bug_fmt::{{closure}}::h9a48bc05a56034c0           
   8:     0x7fc0135658c1 - rustc::session::opt_span_bug_fmt::h1ace49cd7b33ef98  
   9:     0x7fc013565596 - rustc::session::bug_fmt::h843af0f745f1a8a0                  
  10:     0x7fc0135e2f16 - rustc::ty::maps::plumbing::force_from_dep_node::hbfa0094c577c659d
  11:     0x7fc013d838c3 - rustc_incremental::persist::dirty_clean::DirtyCleanVisitor::check_item::h08ed8b33920106ea
  12:     0x7fc013d926eb - rustc_incremental::persist::save::save_dep_graph::hb615f7e262903cee
  13:     0x7fc0142a4520 - rustc_trans::base::assert_and_save_dep_graph::hf030c35a2e864247
  14:     0x7fc0142a26e4 - rustc_trans::base::trans_crate::h71af7ef3ccb676ae              
  15:     0x7fc014307b53 - <rustc_trans::LlvmTransCrate as rustc_trans_utils::trans_crate::TransCrate>::trans_crate::h1f1b9a0b572dd021
  16:     0x7fc016d0e95f - rustc_driver::driver::compile_input::{{closure}}::h721acac80473dd4a
  17:     0x7fc016c9589e - rustc::ty::context::TyCtxt::create_and_enter::h933e6b78f81d28a4
  18:     0x7fc016d0cd83 - rustc_driver::driver::compile_input::h02b2fda6f05ad3f7                                                                                                                                  
  19:     0x7fc016d2548d - rustc_driver::run_compiler::he5d0cfda82a2592f                                                                                                                                           
  20:     0x7fc016c46691 - std::sys_common::backtrace::__rust_begin_short_backtrace::h898eaaf417b02194                                                                                                             
  21:     0x7fc0169b1bf7 - __rust_maybe_catch_panic                                                                                                                                                                
  22:     0x7fc016c818f0 - <F as alloc::boxed::FnBox<A>>::call_box::h79e40ba85b627dc8                                                                                                                              
  23:     0x7fc0169a6fba - std::sys::imp::thread::Thread::new::thread_start::hc30eb3356e75323f                                                                                                                     
  24:     0x7fc0117c8089 - start_thread                                                                          
  25:     0x7fc01666924e - __clone

I'm changing it so it only calls force when the DepKind is one of the trait DepNodes, we'll see if that works...

vitiral · 2017-10-13T02:36:17Z

@michaelwoerister I'm getting ICE from even trying to query the Hir (and pretty much everything else) on this line:

https://github.com/vitiral/rust/blob/incr_ICE2/src/test/incremental/hashes/trait_defs.rs#L446

Also note: I have added this: https://github.com/vitiral/rust/blob/incr_ICE2/src/librustc_incremental/persist/dirty_clean.rs#L502

incremental: session directory: 5 files hard-linked                                  
incremental: session directory: 0 files copied                                                
thread 'rustc' panicked at 'no entry found for key', src/libcore/option.rs:839:4                                                                                                                                   
stack backtrace:                                                                                                                                                                                                   
   0:     0x7fbfda67e822 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::h2bee036dd609b02b                                                                                                             
   1:     0x7fbfda67905b - std::sys_common::backtrace::_print::h2a9bd6df49be92d1                                                                                                                                   
   2:     0x7fbfda68c119 - std::panicking::default_hook::{{closure}}::hcae6c022a3505fce                       
   3:     0x7fbfda68bdf5 - std::panicking::default_hook::h2b2460f26d5a082b
   4:     0x7fbfda68c628 - std::panicking::rust_panic_with_hook::hf4116880dbeebeea
   5:     0x7fbfda68c48b - std::panicking::begin_panic::h66e7ed06c82f53e8                                                        
   6:     0x7fbfda68c3ef - std::panicking::begin_panic_fmt::h0677c8c3f1de70eb
   7:     0x7fbfda68c375 - rust_begin_unwind                                                          
   8:     0x7fbfda6a65a2 - core::panicking::panic_fmt::ha635165eed891315        
   9:     0x7fbfda6a6615 - core::option::expect_failed::ha4257b2642d22559                                                                                                                                          
  10:     0x7fbfd712f258 - rustc::dep_graph::graph::DepGraph::fingerprint_of::h9f09b217a4c2989e                                                                                                                    
  11:     0x7fbfd7a67794 - rustc_incremental::persist::dirty_clean::DirtyCleanVisitor::check_item::h08ed8b33920106ea
  12:     0x7fbfd7a769eb - rustc_incremental::persist::save::save_dep_graph::hb615f7e262903cee
  13:     0x7fbfd7f88520 - rustc_trans::base::assert_and_save_dep_graph::hf030c35a2e864247
  14:     0x7fbfd7f866e4 - rustc_trans::base::trans_crate::h71af7ef3ccb676ae              
  15:     0x7fbfd7febb53 - <rustc_trans::LlvmTransCrate as rustc_trans_utils::trans_crate::TransCrate>::trans_crate::h1f1b9a0b572dd021
  16:     0x7fbfda9f295f - rustc_driver::driver::compile_input::{{closure}}::h721acac80473dd4a                   
  17:     0x7fbfda97989e - rustc::ty::context::TyCtxt::create_and_enter::h933e6b78f81d28a4
  18:     0x7fbfda9f0d83 - rustc_driver::driver::compile_input::h02b2fda6f05ad3f7
  19:     0x7fbfdaa0948d - rustc_driver::run_compiler::he5d0cfda82a2592f
  20:     0x7fbfda92a691 - std::sys_common::backtrace::__rust_begin_short_backtrace::h898eaaf417b02194
  21:     0x7fbfda695bf7 - __rust_maybe_catch_panic                       
  22:     0x7fbfda9658f0 - <F as alloc::boxed::FnBox<A>>::call_box::h79e40ba85b627dc8
  23:     0x7fbfda68afba - std::sys::imp::thread::Thread::new::thread_start::hc30eb3356e75323f
  24:     0x7fbfd54ac089 - start_thread                                                       
  25:     0x7fbfda34d24e - __clone                                                                                                                                                                                   26:                0x0 - <unknown>

good news is that I can confirm that forcing only the trait-specific DepNodes doesn't itself cause an ICE 😄

vitiral · 2017-10-13T04:25:04Z

@michaelwoerister I have completed what I can do on this and added a FIXME for you. This should be ready for review+merge

This adds auto-assertion to `rustc_clean/dirty` and also implements more comprehensive testing for - src/test/incremental/hashes/enum_constructors.rs - src/test/incremental/hashes/enum_defs.rs - src/test/incremental/hashes/extern_mods.rs - src/test/incremental/hashes/inherent_impls.rs - src/test/incremental/hashes/statics.rs - src/test/incremental/hashes/struct_constructors.rs - src/test/incremental/hashes/type_defs.rs trait_defs.rs and trait_impl.rs are blocked on a hard to triage compiler ICE (at least hard for a newbie like me) having to do with some DepNodes not getting computed for traits. A FIXME has been added in the source to reflect this continued work.

michaelwoerister

Thank you so much, @vitiral! I've just read through the PR and you've clearly put a lot of work and time into this. I think the extension to the framework turned out very nice and make test definitions much more concise and readable.

I've left some comments. The requested changes should be rather quick to do. I'll then take care of the trait definition problems.

michaelwoerister · 2017-10-13T09:35:18Z

src/librustc_incremental/persist/dirty_clean.rs

@@ -39,10 +39,15 @@
 //! previous revision to compare things to.
 //!

+#![allow(dead_code)]


I assume this is because of some yet to be used constants below? I think you can attach the attribute directly to the item that you want to ignore.

michaelwoerister · 2017-10-13T09:46:06Z

src/librustc_incremental/persist/dirty_clean.rs

+        let (name, mut auto) = self.auto_labels(item_id, attr);
+        let except = self.except(attr);
+        for e in except.iter() {
+            if !auto.remove(e) {


Yes, this is a good check to have 👍

michaelwoerister · 2017-10-13T09:53:36Z

src/librustc_incremental/persist/dirty_clean.rs

+                    HirItem::ItemGlobalAsm(..) => ("ItemGlobalAsm", LABELS_HIR_ONLY),
+
+                    // A type alias, e.g. `type Foo = Bar<u8>`
+                    HirItem::ItemTy(..) => ("ItemTy", LABELS_CONST),


Interesting. I'll need to check this. For now let's rather make it LABELS_HIR_ONLY.

michaelwoerister · 2017-10-13T09:55:06Z

src/librustc_incremental/persist/dirty_clean.rs

+                    HirItem::ItemTy(..) => ("ItemTy", LABELS_CONST),
+
+                    // An enum definition, e.g. `enum Foo<A, B> {C<A>, D<B>}`
+                    HirItem::ItemEnum(..) => ("ItemEnum", LABELS_STRUCT),


Could you rename LABELS_STRUCT to LABELS_ADT (as in Abstract Data Type)? That's what structs/enums/unions are called in the compiler.

michaelwoerister · 2017-10-13T09:57:02Z

src/librustc_incremental/persist/dirty_clean.rs

+];
+
+/// Trait-Method DepNodes
+const LABELS_FN_TRAIT: &[&[&str]] = &[


and finally LABELS_FN_IN_TRAIT.

michaelwoerister · 2017-10-13T10:21:37Z

src/test/incremental/hashes/enum_constructors.rs

-#[rustc_clean(label="Hir", cfg="cfail3")]
-#[rustc_dirty(label="HirBody", cfg="cfail2")]
-#[rustc_clean(label="HirBody", cfg="cfail3")]
+#[rustc_clean(cfg="cfail2", except="HirBody,TypeckTables")]


Interesting. I would have thought that that changes the MIR. And it would if it were not all constants.

left a FIXME for you

michaelwoerister · 2017-10-13T10:23:37Z

src/test/incremental/hashes/enum_constructors.rs

-    #[rustc_clean(label="HirBody", cfg="cfail3")]
+    #[rustc_clean(
+        cfg="cfail2",
+        except="FnSignature,Hir,HirBody,MirOptimized,MirValidated,TypeckTables"


One could probably split this string with a \ and it would still work.

michaelwoerister · 2017-10-13T10:49:37Z

src/test/incremental/hashes/enum_defs.rs

-#[rustc_dirty(label="Hir", cfg="cfail2")]
-#[rustc_clean(label="Hir", cfg="cfail3")]
+#[rustc_dirty(cfg="cfail2")]
+#[rustc_clean(cfg="cfail3")]
 #[rustc_metadata_dirty(cfg="cfail2")]
 #[rustc_metadata_clean(cfg="cfail3")]
 #[repr(C)]


The #[repr(C)] here seems to be an oversight.

michaelwoerister · 2017-10-13T10:51:45Z

src/test/incremental/hashes/enum_defs.rs

-#[rustc_dirty(label="Hir", cfg="cfail2")]
-#[rustc_clean(label="Hir", cfg="cfail3")]
+#[rustc_dirty(cfg="cfail2")]
+#[rustc_clean(cfg="cfail3")]
 #[rustc_metadata_dirty(cfg="cfail2")]
 #[rustc_metadata_clean(cfg="cfail3")]
 #[repr(C)]


Another stray #[repr(C)].

And a few more below. All of these tests seem to be broken :( Should have caught that during review a the time...

the only one I left was the test with the header // Add #[repr(C)] to the enum ----... (someone probably copy/pasted that test and forgot to remove the repr).

michaelwoerister · 2017-10-13T11:15:12Z

src/test/incremental/hashes/inherent_impls.rs

 #[rustc_metadata_clean(cfg="cfail2")]
 #[rustc_metadata_clean(cfg="cfail3")]
 impl Foo {
-    #[rustc_dirty(label="Hir", cfg="cfail2")]
-    #[rustc_clean(label="Hir", cfg="cfail3")]
+    #[rustc_clean(cfg="cfail2", except="Hir,HirBody")]


This is curious but an unused lifetime parameter doesn't seem to show up in any of the derived data structures.

left a FIXME for you

vitiral · 2017-10-13T13:37:41Z

Thanks @michaelwoerister! Do you want me to do anything about your "this is interesting" comments, i.e. leave a FIXME for you to look at later?

Edit: and yes, those changes should be easy to implement

vitiral · 2017-10-13T16:04:09Z

@michaelwoerister your comments should all be fixed. I added FIXME for your personal "I'm confused" comments, I'll let you figure those out and delete the FIXME's 😄

michaelwoerister · 2017-10-13T16:16:43Z

Thanks for removing those #[repr(C)]s!

@bors r+

bors · 2017-10-13T16:16:44Z

📌 Commit 80c13ce has been approved by michaelwoerister

bors · 2017-10-14T04:11:55Z

⌛ Testing commit 80c13ce with merge 2aeff0f...

@michaelwoerister

Incremental compilation auto assert (with except) cc @michaelwoerister bors merged part 1, so this is a WIP of part 2 of #45009 -- auto asserting DepNodes depending on the type of node rustc_clean/dirty is attached to Framework: - [x] finish auto-detection for specified DepNodes - [x] finish auto-detection for remaining DepNodes Test Refactors: - [x] consts.rs - [x] enum_constructors.rs - [x] extern_mods.rs - [x] inherent_impls.rs - [x] statics.rs - [x] struct_constructors.rs - ~~**BLOCKED** trait_defs.rs, see FIXME~~ - ~~**BLOCKED** trait_impls.rs~~ - [x] type_defs.rs - [x] enum_defs.rs

bors · 2017-10-14T06:34:16Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 2aeff0f to master...

michaelwoerister · 2017-10-16T09:16:06Z

🎉

rust-highfive assigned arielb1 Oct 8, 2017

This was referenced Oct 8, 2017

tracking issue for rustc_dirty/clean enhancements #45009

Closed

incr.comp.: Update fingerprint-based auto tests for red/green tracking. #44924

Closed

michaelwoerister reviewed Oct 9, 2017

View reviewed changes

carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 9, 2017

rust-highfive assigned michaelwoerister and unassigned arielb1 Oct 11, 2017

vitiral force-pushed the incr_auto_assert2 branch from ff38a3e to 81bb1c2 Compare October 13, 2017 04:24

vitiral force-pushed the incr_auto_assert2 branch from 81bb1c2 to f7fe970 Compare October 13, 2017 04:31

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 13, 2017

michaelwoerister reviewed Oct 13, 2017

View reviewed changes

vitiral force-pushed the incr_auto_assert2 branch from 3b3334e to 80c13ce Compare October 13, 2017 16:13

fix review comments

80c13ce

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2017

bors merged commit 80c13ce into rust-lang:master Oct 14, 2017

vitiral deleted the incr_auto_assert2 branch October 27, 2017 05:59


		// Base and Extra labels to build up the labels

		/// DefNodes for Hir, which is pretty much everything

Incremental compilation auto assert (with except) #45104

Incremental compilation auto assert (with except) #45104

Conversation

vitiral commented Oct 8, 2017 • edited Loading

rust-highfive commented Oct 8, 2017

vitiral commented Oct 8, 2017

michaelwoerister commented Oct 9, 2017

Choose a reason for hiding this comment

vitiral commented Oct 9, 2017

michaelwoerister commented Oct 9, 2017

vitiral commented Oct 9, 2017

vitiral commented Oct 9, 2017 • edited Loading

michaelwoerister commented Oct 10, 2017

michaelwoerister commented Oct 10, 2017

vitiral commented Oct 10, 2017 via email

michaelwoerister commented Oct 10, 2017

vitiral commented Oct 10, 2017

arielb1 commented Oct 11, 2017

vitiral commented Oct 13, 2017 • edited Loading

vitiral commented Oct 13, 2017

vitiral commented Oct 13, 2017 • edited Loading

vitiral commented Oct 13, 2017 • edited Loading

michaelwoerister 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitiral Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitiral commented Oct 13, 2017 • edited Loading

vitiral commented Oct 13, 2017 • edited Loading

michaelwoerister commented Oct 13, 2017

bors commented Oct 13, 2017

bors commented Oct 14, 2017

bors commented Oct 14, 2017

michaelwoerister commented Oct 16, 2017

vitiral commented Oct 8, 2017 •

edited

Loading

vitiral commented Oct 9, 2017 •

edited

Loading

vitiral commented Oct 13, 2017 •

edited

Loading

vitiral commented Oct 13, 2017 •

edited

Loading

vitiral commented Oct 13, 2017 •

edited

Loading

vitiral Oct 13, 2017 •

edited

Loading

vitiral commented Oct 13, 2017 •

edited

Loading

vitiral commented Oct 13, 2017 •

edited

Loading