Revamp symbol names for impls (and make them deterministic, etc) #32293

nikomatsakis · Mar 16, 2016

This builds on @michaelwoerister's epic PR #31539 (note that his PR never landed, so I just incorporated it into this one). The main change here is that we remove the "name" from DefPathData for impls, since that name is synthetic and not sufficiently predictable for incr comp. However, just doing that would cause bad symbol names since those are based on the DefPath. Therefore, I introduce a new mechanism for getting symbol names (and also paths for user display) called item_path. This is kind of simplistic for now (based on strings) but I expect to expand it later to support richer types, hopefully generating C++-mangled names that gdb etc can understand. Along the way I cleaned up how we track the path that leads to an extern crate.

There is still some cleanup left undone here. Notably, I didn't remove the impl names altogether -- that would probably make sense. I also didn't try to remove the item_symbols vector. Mostly I want to unblock my other incr. comp. work. =)

r? @eddyb
cc @eddyb @alexcrichton @michaelwoerister

nikomatsakis · Mar 16, 2016

Note that the review is technically only for my comments, not mw's, but of course feel free to comment on those too and I'll patch up any nits or whatever.

eddyb · Mar 16, 2016

eddyb reviewed Mar 16, 2016

View changes

eddyb · Mar 16, 2016

src/librustc/front/map/definitions.rs

+/// the original crate but also its def-id. This is kind of an
+/// augmented version of a `DefPath` that includes a `DefId`. This is
+/// all sort of ugly but the hope is that inlined items will be going
+/// away soon anyway.


MIR can't come soon enough 😞.

We could pass the original id in the trans::inline path just like MIR.

eddyb · Mar 16, 2016

eddyb reviewed Mar 16, 2016

View changes

eddyb · Mar 16, 2016

src/librustc/session/mod.rs

@@ -486,7 +487,7 @@ pub fn build_session_(sopts: config::Options,
        plugin_attributes: RefCell::new(Vec::new()),
        crate_types: RefCell::new(Vec::new()),
        dependency_formats: RefCell::new(FnvHashMap()),
-        crate_disambiguator: RefCell::new(String::new()),
+        crate_disambiguator: Cell::new(token::intern("")),


I think special_idents::invalid is supposed to be empty.

eddyb · Mar 16, 2016

eddyb reviewed Mar 16, 2016

View changes

eddyb · Mar 16, 2016

src/librustc/front/map/definitions.rs

+    pub data: Vec<DisambiguatedDefPathData>,
+
+    /// what krate root is this path relative to?
+    pub krate: ast::CrateNum,


Does DefId still need to hold a CrateNum, or is removing that too expensive?

@eddyb at the moment, DefId does need a CrateNum, because we don't put all the ids from all crates together. e.g. if you compute the def-path from a def-id, we first look at def_id.krate to find the krate, then go lookup the index in the appropriate place based on that.

So the mapping is in the opposite direction than I expected.

eddyb · Mar 16, 2016

eddyb reviewed Mar 16, 2016

View changes

eddyb · Mar 16, 2016

src/librustc_trans/back/symbol_names.rs

+    let def_path = DefPath {
+        data: vec![],
+        krate: cstore::LOCAL_CRATE,
+    };


In the long term we should move to <T as Trait>::method, at least for trait-related shims.

eddyb · Mar 16, 2016

eddyb reviewed Mar 16, 2016

View changes

eddyb · Mar 16, 2016

src/librustc/middle/ty/item_path.rs

+    /// we can omit including the path to the impl itself. This
+    /// function tries to find a "characteristic def-id" for a
+    /// type. It's just a heuristic so it makes some questionable
+    /// decisions and we may want to adjust it later.


I am not sure that this is better than treating all impls as local. For example, structural_impls would use the ugly way - that's it, middle::structural_impls::<impl TypeFoldable for Vec<T>>::fold_ty rather than <Vec<T> as TypeFoldable>::fold_ty.

On Tue, Mar 22, 2016 at 05:54:48AM -0700, arielb1 wrote:

I am not sure that this is better than treating all impls as local. For example, structural_impls would use the ugly way - that's it, middle::structural_impls::<impl TypeFoldable for Vec<T>>::fold_ty rather than <Vec<T> as TypeFoldable>::fold_ty.

Yeah, I don't know what's best. One could also imagine some heuristic
based on "submodules". But I guess one could argue that for
structural_impls it'd be useful to be told that the impl is in that
module, rather than the module for Vec. I'm totally amenable to
follow-up PRs that tweak these heuristics -- probably at some point we
would want to RFC this, but I guess that's a long way in the future.

The orphan rules guarantee that the impl is at least in the same crate as TypeFoldable (seeing that it depends on Vec, not vice-versa) - that would also happen if TypeFoldable was in structural_impls. grep within a crate is not that much of a problem.

eddyb · Mar 17, 2016

eddyb reviewed Mar 17, 2016

View changes

eddyb · Mar 17, 2016

src/compiletest/compiletest.rs

-        compile_lib_path: matches.opt_str("compile-lib-path").unwrap(),
-        run_lib_path: matches.opt_str("run-lib-path").unwrap(),
+        compile_lib_path: make_absolute(opt_path(matches, "compile-lib-path")),
+        run_lib_path: make_absolute(opt_path(matches, "run-lib-path")),


If this fixes run-pass/command-before-exec, I think a revert of the static linking hack @alexcrichton suggested and I merged, is in order.

If this fixes run-pass/command-before-exec, I think a revert of the static linking hack @alexcrichton suggested and I merged, is in order.

Actually I have no real idea what the purpose of this change is. :) @michaelwoerister, care to comment? :)

Yes, this change is solely for run-pass/command-before-exec.

eddyb · Mar 17, 2016

eddyb reviewed Mar 17, 2016

View changes

eddyb · Mar 17, 2016

src/librustc_trans/trans/closure.rs

-    let symbol = mangle_internal_name_by_path_and_seq(path, "closure");
+    let symbol = symbol_names::exported_name(ccx,
+                                             mono_id.def,
+                                             mono_id.params.as_slice());


Sweet, although I'm not sure whether my PR or this should come first - also, maybe exported_name should take an Instance (here still called MonoId)?

@eddyb

also, maybe exported_name should take an Instance (here still called MonoId)?

Yeah, I noticed at some point recently that it takes a &[Ty<'tcx>], which just seems wrong. It ought to take a Substs clearly. I'll see if I can easily address that.

@eddyb

although I'm not sure whether my PR or this should come first

It's a race! No, but seriously -- do you think they'll conflict much?

Also, I was just looking at callers of exported_name. The code in get_item_val seems a bit dubious. Not wrong, but dubious -- it calls exported_name with the id of (e.g.) an enum or function but a single type T where T is basically lookup_item_type(id).ty. This seems to ... mostly be used only for non-generic items, but in that case the type parameter should be altogether unnecessary I would think. Not sure if you refactored this at all? I remember some changes in that direction, so I suspect so. Seems like taking a mono_id would force the question and clean it up.

eddyb · Mar 17, 2016

LGTM, except for the nits, some of which GitHub is marking as being on an outdated diff.

alexcrichton · Mar 17, 2016

alexcrichton reviewed Mar 17, 2016

View changes

alexcrichton · Mar 17, 2016

src/test/compile-fail/symbol-names/impl1.rs

+    pub struct Foo { x: u32 }
+
+    impl Foo {
+        #[rustc_symbol_name] //~ ERROR _ZN5impl13foo3Foo3bar


ok, this is pretty slick

bors · Mar 18, 2016

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

michaelwoerister · Mar 19, 2016

michaelwoerister reviewed Mar 19, 2016

View changes

michaelwoerister · Mar 19, 2016

src/test/compile-fail/symbol-names/basic.rs


+#[rustc_symbol_name] //~ ERROR _ZN5basic4main


This name and others should end with an 'E' to close the nested name that was started with the 'N'.

On Sat, Mar 19, 2016 at 04:01:27PM -0700, Michael Woerister wrote:

+#[rustc_symbol_name] //~ ERROR _ZN5basic4main

This name and others should end with an 'E' to close the nested name that was started with the 'N'.

The names do end with E. I just left that out of the test because I didn't want to put the hash in there.

michaelwoerister · Mar 19, 2016

Looks good to me too. Consolidating path rendering into its own module is a good idea and it's great that we'll be testing symbol names from now on. Looking forward to tinkering with the name mangling scheme.

nikomatsakis · Mar 21, 2016

Rebased. Some of the poor @michaelwoerister's beautifully clean commit history got a bit messed up (I suspect the individual commits no longer build), but seems to build locally. Not yet finished withmake check though.

nikomatsakis · Mar 21, 2016

@bors r=eddyb

bors · Mar 21, 2016

📌 Commit 8786477 has been approved by eddyb

nikomatsakis · Mar 21, 2016

@bors r-

nikomatsakis · Mar 21, 2016

@eddyb

If this fixes run-pass/command-before-exec, I think a revert of the static linking hack @alexcrichton suggested and I merged, is in order.

Does this just mean removing the comment on the test? If so, I did that in the most recent commit.

nikomatsakis · Mar 21, 2016

@bors r=eddyb

bors · Mar 21, 2016

📌 Commit ea70d16 has been approved by eddyb

bors · Mar 22, 2016

⌛️ Testing commit ea70d16 with merge dec92c1...

bors · Mar 22, 2016

💔 Test failed - auto-mac-64-nopt-t

nikomatsakis · Mar 22, 2016

@bors r=eddyb

bors · Mar 22, 2016

📌 Commit 4eadadf has been approved by eddyb

nikomatsakis · Mar 22, 2016

question to @alexcrichton -- do you know if the windows builders have nm and cmp available? If not, feel to r- this PR.

nikomatsakis · Mar 25, 2016

@bors r=alexcrichton

bors · Mar 25, 2016

📌 Commit 726ba66 has been approved by alexcrichton

nikomatsakis · Mar 25, 2016

@bors p=1

nikomatsakis · Mar 25, 2016

(This PR has been hanging around way too long.)

bors · Mar 25, 2016

⌛️ Testing commit 726ba66 with merge 07ee0ed...

bors · Mar 25, 2016

💔 Test failed - auto-win-gnu-64-nopt-t

nikomatsakis · Mar 25, 2016

@bors r=alexcrichton

bors · Mar 25, 2016

📌 Commit 874574d has been approved by alexcrichton

nikomatsakis · Mar 25, 2016

@bors force

bors · Mar 25, 2016

⌛️ Testing commit 874574d with merge 0f50c0d...

bors · Mar 25, 2016

💔 Test failed - auto-win-gnu-64-nopt-t

nikomatsakis · Mar 25, 2016

@bors r=alexcrichton

bors · Mar 25, 2016

📌 Commit 1ea93c2 has been approved by alexcrichton

bors · Mar 26, 2016

⌛️ Testing commit 1ea93c2 with merge 8d2d2be...

bors · Mar 26, 2016

rust-highfive assigned eddyb Mar 16, 2016

nikomatsakis force-pushed the nikomatsakis:incr-comp-def-path-munging branch 2 times, most recently from 3214c33 to bdfc0b4 Mar 21, 2016

nikomatsakis force-pushed the nikomatsakis:incr-comp-def-path-munging branch from 8786477 to 09574bd Mar 21, 2016

nikomatsakis added some commits Mar 25, 2016

Update backtrace test for FIXME on windows

8a7b1bc

Rebase over my PR

Loading status checks…

726ba66

nikomatsakis force-pushed the nikomatsakis:incr-comp-def-path-munging branch from 8cfc003 to 726ba66 Mar 25, 2016

change test to be specific for msvc

Loading status checks…

874574d

remove link-guard test

Loading status checks…

1ea93c2

bors merged commit 1ea93c2 into rust-lang:master Mar 26, 2016
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

homu Test successful
Details

This was referenced Mar 26, 2016

Closed

Rollup of 18 pull requests #32495

Merged

Flatten rustc and rustc_trans module hierarchy slightly. #32432

alexcrichton referenced this pull request Mar 27, 2016
Closed
`unexpected panic` when compiling the `lalrpop` crate #32530

nikomatsakis deleted the nikomatsakis:incr-comp-def-path-munging branch Mar 30, 2016

tbu- referenced this pull request Apr 17, 2016
Closed
Rust compilations are not reproducible #30330

sanxiyn referenced this pull request Apr 25, 2016
Closed
DWARF regression with `impl` on nightly #33192

rust-lang/rust

Join GitHub today

Revamp symbol names for impls (and make them deterministic, etc) #32293

Conversation

nikomatsakis commented Mar 16, 2016

rust-highfive assigned eddyb Mar 16, 2016

This comment has been minimized.

nikomatsakis commented Mar 16, 2016

eddyb reviewed Mar 16, 2016 View changes

This comment has been minimized.

This comment has been minimized.

eddyb reviewed Mar 16, 2016 View changes

This comment has been minimized.

eddyb reviewed Mar 16, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb reviewed Mar 16, 2016 View changes

This comment has been minimized.

eddyb reviewed Mar 16, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb reviewed Mar 17, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb reviewed Mar 17, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb commented Mar 17, 2016

alexcrichton reviewed Mar 17, 2016 View changes

This comment has been minimized.

This comment has been minimized.

bors commented Mar 18, 2016

michaelwoerister reviewed Mar 19, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

michaelwoerister commented Mar 19, 2016

nikomatsakis force-pushed the nikomatsakis:incr-comp-def-path-munging branch 2 times, most recently from 3214c33 to bdfc0b4 Mar 21, 2016

This comment has been minimized.

nikomatsakis commented Mar 21, 2016

This comment has been minimized.

nikomatsakis commented Mar 21, 2016

This comment has been minimized.

bors commented Mar 21, 2016

nikomatsakis force-pushed the nikomatsakis:incr-comp-def-path-munging branch from 8786477 to 09574bd Mar 21, 2016

This comment has been minimized.

nikomatsakis commented Mar 21, 2016

This comment has been minimized.

nikomatsakis commented Mar 21, 2016

This comment has been minimized.

nikomatsakis commented Mar 21, 2016

This comment has been minimized.

bors commented Mar 21, 2016

This comment has been minimized.

bors commented Mar 22, 2016

bors added a commit that referenced this pull request Mar 22, 2016

This comment has been minimized.

bors commented Mar 22, 2016

This comment has been minimized.

nikomatsakis commented Mar 22, 2016

This comment has been minimized.

bors commented Mar 22, 2016

This comment has been minimized.

nikomatsakis commented Mar 22, 2016

nikomatsakis added some commits Mar 25, 2016

nikomatsakis force-pushed the nikomatsakis:incr-comp-def-path-munging branch from 8cfc003 to 726ba66 Mar 25, 2016

This comment has been minimized.

nikomatsakis commented Mar 25, 2016

This comment has been minimized.

bors commented Mar 25, 2016

This comment has been minimized.

nikomatsakis commented Mar 25, 2016

This comment has been minimized.

nikomatsakis commented Mar 25, 2016

eddyb reviewed Mar 16, 2016

View changes

eddyb reviewed Mar 16, 2016

View changes

eddyb reviewed Mar 16, 2016

View changes

eddyb reviewed Mar 16, 2016

View changes

eddyb reviewed Mar 16, 2016

View changes

eddyb reviewed Mar 17, 2016

View changes

eddyb reviewed Mar 17, 2016

View changes

alexcrichton reviewed Mar 17, 2016

View changes

michaelwoerister reviewed Mar 19, 2016

View changes

nikomatsakis force-pushed the nikomatsakis:incr-comp-def-path-munging branch 2 times, most recently from `3214c33` to `bdfc0b4` Mar 21, 2016

nikomatsakis force-pushed the nikomatsakis:incr-comp-def-path-munging branch from `8786477` to `09574bd` Mar 21, 2016

nikomatsakis force-pushed the nikomatsakis:incr-comp-def-path-munging branch from `8cfc003` to `726ba66` Mar 25, 2016

bors merged commit `1ea93c2` into rust-lang:master Mar 26, 2016
1 of 2 checks passed