Skip to content

Audit metadata dep-graph edges #35111

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

Closed
nikomatsakis opened this issue Jul 29, 2016 · 4 comments
Closed

Audit metadata dep-graph edges #35111

nikomatsakis opened this issue Jul 29, 2016 · 4 comments
Assignees
Labels
A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

When we record the metadata, we create a new task for the metadata pertaining to each item. The incoming edges to this task are then used to compute the hash of the metadata. This is all good, but there are a number of cases of "information leaks" that will lead to missing edges.

One example:

fn encode_info_for_method<'a, 'tcx>(ecx: &EncodeContext<'a, 'tcx>,
                                    rbml_w: &mut Encoder,
                                    index: &mut CrateIndex<'a, 'tcx>,
                                    m: &ty::Method<'tcx>,
                                    is_default_impl: bool,
                                    parent_id: NodeId,
                                    impl_item_opt: Option<&hir::ImplItem>) {

    debug!("encode_info_for_method: {:?} {:?}", m.def_id,
           m.name);
    let _task = index.record(m.def_id, rbml_w); // creates a task X

Here, if the function reads from m or impl_item_opt, this will not result in any incoming edges to the task X. To make this correct, we should either add artificial, explicit reads to account for those parameters or -- probably better -- minimize the data we pass in, and have encode_info_for_method go fetch things out of the appropriate maps, which will register the proper reads.

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-incr-comp Area: Incremental compilation labels Jul 29, 2016
@nikomatsakis nikomatsakis added this to the Incremental compilation alpha milestone Jul 29, 2016
@michaelwoerister
Copy link
Member

So, if we used TyCtxt::visit_all_items_in_krate() instead of Crate::visit_all_items() at

krate.visit_all_items(&mut EncodeVisitor {
, that would fix the majority of problems, right?

@nikomatsakis
Copy link
Contributor Author

@michaelwoerister yeah, actually, I think that was a mistake on my part. That would certainly be a good step. :) Probably ought to change the names there to make the distinction more obvious.

@nikomatsakis nikomatsakis self-assigned this Aug 10, 2016
@nikomatsakis
Copy link
Contributor Author

I'll tackle this.

@michaelwoerister
Copy link
Member

It would also be good to see if we are reading data from external crates that is not linked to a specific DefId (i.e. "global" data like the disambiguator) but still influences codegen in some way.

bors added a commit that referenced this issue Aug 18, 2016
Restructure metadata encoder to track deps precisely

This issue restructures meta-data encoding to track dependencies very precisely. It uses a cute technique I hope to spread elsewhere, where we can guard the data flowing into a new dep-graph task and ensure that it is not "leaking" information from the outside, which would result in missing edges. There are no tests because we don't know of any bugs in the old system, but it's clear that there was leaked data.

The commit series is standalone, but the refactorings are kind of "windy". It's a good idea to read the comment in `src/librustc_metadata/index_builder.rs` to get a feeling for the overall strategy I was aiming at.

In several cases, I wound up adding some extra hashtable lookups, I think primarily for looking up `AdtDef` instances. We could remove these by implementing `DepGraphRead` for an `AdtDef` and then having it register a read to the adt-defs table, I guess, but I doubt it is really noticeable.

Eventually I think I'd like to extend this pattern to the dep-graph more generally, since it effectively guarantees that data cannot leak.

Fixes #35111.

r? @michaelwoerister
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants