Skip to content

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

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

Merged
merged 45 commits into from
Mar 26, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
6fdeecf
CrateStore: Allow for custom def_id_to_string mappings in encode_type().
michaelwoerister Feb 10, 2016
5027a79
Add missing entries for enum variants in trans::CrateContext::externa…
michaelwoerister Feb 13, 2016
606c985
Make CrateStore::crate_name() return an InternedString to avoid unnec…
michaelwoerister Feb 12, 2016
c7e54d7
Make library paths passed by compiletest tool absolute.
michaelwoerister Feb 29, 2016
32a2e9a
Compute a salt from arguments passed via -Cmetadata.
michaelwoerister Feb 12, 2016
3a756fe
Make the definite name of the local crate available in the tcx.
michaelwoerister Feb 14, 2016
c77f44e
Make monomorphized functions use stable symbol names.
michaelwoerister Feb 14, 2016
6f60c9e
Make closures use stable symbol names.
michaelwoerister Feb 14, 2016
7a5a988
Make drop glue use new symbol naming scheme.
michaelwoerister Feb 14, 2016
23e54b2
Use new symbol naming scheme for fn-pointer-shims.
michaelwoerister Feb 14, 2016
8ef638e
Use new symbol naming scheme for fn-once-shims.
michaelwoerister Feb 14, 2016
68de171
Use new symbol naming scheme for object shims.
michaelwoerister Mar 1, 2016
7def376
Use new symbol names for items of various kinds.
michaelwoerister Feb 15, 2016
9c965b7
Add a test to verify that we have reproducible compiler builds.
michaelwoerister Feb 18, 2016
fafdfa8
Salt test crates in buildsystem.
michaelwoerister Feb 18, 2016
2eebb7b
Make the compiler emit an error if the crate graph contains two crate…
michaelwoerister Feb 26, 2016
82b5f1d
Remove old symbol naming code.
michaelwoerister Mar 1, 2016
2475707
Add a "link-guard" to avoid accidentally linking to a wrong dylib at …
michaelwoerister Mar 1, 2016
65c0b7c
track def-id for inlined items
nikomatsakis Mar 16, 2016
7b6270b
store krate information more uniformly
nikomatsakis Mar 16, 2016
5e26508
refactor DefPathData variants
nikomatsakis Mar 16, 2016
6056c5f
fallout: update codegen-units tests
nikomatsakis Mar 16, 2016
ab9b844
track the extern-crate def-id rather than path
nikomatsakis Mar 16, 2016
cd5cf09
add krate_attrs accessor
nikomatsakis Mar 16, 2016
2291abf
refactor item-paths in diagnostics, symbol names
nikomatsakis Mar 16, 2016
9776361
unit-test symbol-names and item-paths
nikomatsakis Mar 16, 2016
86fa58d
remove unused variable in compiletest
nikomatsakis Mar 16, 2016
af057bd
renumber diagnostic to avoid conflict
nikomatsakis Mar 16, 2016
5a68116
pacify the merciless tidy: s/E0521/E0522
nikomatsakis Mar 17, 2016
e8dfaa7
Correections due to refactoring .
nikomatsakis Mar 21, 2016
bca9423
Fix accursed issue-4264.pp
nikomatsakis Mar 21, 2016
7e8e671
Remove static linking hack since we are now passing absolute paths
nikomatsakis Mar 21, 2016
b184d27
check only that symbol names are deterministic
nikomatsakis Mar 22, 2016
751c24d
renumber error from E0522 to E0523
nikomatsakis Mar 22, 2016
b385ce1
workarounds to make link guards work on windows
nikomatsakis Mar 23, 2016
4c52745
rip out link guards
nikomatsakis Mar 24, 2016
814477a
Revert "workarounds to make link guards work on windows"
nikomatsakis Mar 24, 2016
87debd9
include the immediate type in the symbol name hash
nikomatsakis Mar 24, 2016
ab0a872
test for immediate symbol name hashing
nikomatsakis Mar 24, 2016
fd25a63
document test, don't use grep
nikomatsakis Mar 24, 2016
24059f7
Drive-by fix for unnecessary `&mut`
nikomatsakis Mar 25, 2016
8a7b1bc
Update backtrace test for FIXME on windows
nikomatsakis Mar 25, 2016
726ba66
Rebase over my PR
nikomatsakis Mar 25, 2016
874574d
change test to be specific for msvc
nikomatsakis Mar 25, 2016
1ea93c2
remove link-guard test
nikomatsakis Mar 25, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mk/tests.mk
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ $(3)/stage$(1)/test/$(4)test-$(2)$$(X_$(2)): \
@$$(call E, rustc: $$@)
$(Q)CFG_LLVM_LINKAGE_FILE=$$(LLVM_LINKAGE_PATH_$(2)) \
$$(subst @,,$$(STAGE$(1)_T_$(2)_H_$(3))) -o $$@ $$< --test \
-L "$$(RT_OUTPUT_DIR_$(2))" \
-Cmetadata="test-crate" -L "$$(RT_OUTPUT_DIR_$(2))" \
$$(LLVM_LIBDIR_RUSTFLAGS_$(2)) \
$$(RUSTFLAGS_$(4))

Expand Down
4 changes: 2 additions & 2 deletions src/compiletest/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,10 @@ impl fmt::Display for Mode {
#[derive(Clone)]
pub struct Config {
// The library paths required for running the compiler
pub compile_lib_path: String,
pub compile_lib_path: PathBuf,

// The library paths required for running compiled programs
pub run_lib_path: String,
pub run_lib_path: PathBuf,

// The rustc executable
pub rustc_path: PathBuf,
Expand Down
12 changes: 10 additions & 2 deletions src/compiletest/compiletest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,17 @@ pub fn parse_config(args: Vec<String> ) -> Config {
}
}

fn make_absolute(path: PathBuf) -> PathBuf {
if path.is_relative() {
env::current_dir().unwrap().join(path)
} else {
path
}
}

Config {
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")),
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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? :)

Copy link
Member

Choose a reason for hiding this comment

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

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

rustc_path: opt_path(matches, "rustc-path"),
rustdoc_path: opt_path(matches, "rustdoc-path"),
python: matches.opt_str("python").unwrap(),
Expand Down
10 changes: 5 additions & 5 deletions src/compiletest/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ fn run_pretty_test_revision(config: &Config,
testpaths,
pretty_type.to_owned()),
props.exec_env.clone(),
&config.compile_lib_path,
config.compile_lib_path.to_str().unwrap(),
Some(aux_dir.to_str().unwrap()),
Some(src))
}
Expand Down Expand Up @@ -635,7 +635,7 @@ fn run_debuginfo_gdb_test(config: &Config, props: &TestProps, testpaths: &TestPa
testpaths,
proc_args,
environment,
&config.run_lib_path,
config.run_lib_path.to_str().unwrap(),
None,
None);
}
Expand Down Expand Up @@ -1315,7 +1315,7 @@ fn exec_compiled_test(config: &Config, props: &TestProps,
testpaths,
make_run_args(config, props, testpaths),
env,
&config.run_lib_path,
config.run_lib_path.to_str().unwrap(),
Some(aux_dir.to_str().unwrap()),
None)
}
Expand Down Expand Up @@ -1387,7 +1387,7 @@ fn compose_and_run_compiler(config: &Config, props: &TestProps,
&aux_testpaths,
aux_args,
Vec::new(),
&config.compile_lib_path,
config.compile_lib_path.to_str().unwrap(),
Some(aux_dir.to_str().unwrap()),
None);
if !auxres.status.success() {
Expand All @@ -1410,7 +1410,7 @@ fn compose_and_run_compiler(config: &Config, props: &TestProps,
testpaths,
args,
props.rustc_env.clone(),
&config.compile_lib_path,
config.compile_lib_path.to_str().unwrap(),
Some(aux_dir.to_str().unwrap()),
input)
}
Expand Down
4 changes: 2 additions & 2 deletions src/librbml/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,15 +166,15 @@ impl<'doc> Doc<'doc> {
}
}

pub fn get<'a>(&'a self, tag: usize) -> Doc<'a> {
pub fn get(&self, tag: usize) -> Doc<'doc> {
reader::get_doc(*self, tag)
}

pub fn is_empty(&self) -> bool {
self.start == self.end
}

pub fn as_str_slice<'a>(&'a self) -> &'a str {
pub fn as_str_slice(&self) -> &'doc str {
str::from_utf8(&self.data[self.start..self.end]).unwrap()
}

Expand Down
36 changes: 24 additions & 12 deletions src/librustc/front/map/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use super::MapEntry::*;
use rustc_front::hir::*;
use rustc_front::util;
use rustc_front::intravisit::{self, Visitor};
use middle::def_id::{CRATE_DEF_INDEX, DefIndex};
use middle::def_id::{CRATE_DEF_INDEX, DefId, DefIndex};
use std::iter::repeat;
use syntax::ast::{NodeId, CRATE_NODE_ID, DUMMY_NODE_ID};
use syntax::codemap::Span;
Expand Down Expand Up @@ -50,6 +50,7 @@ impl<'ast> NodeCollector<'ast> {
parent: &'ast InlinedParent,
parent_node: NodeId,
parent_def_path: DefPath,
parent_def_id: DefId,
map: Vec<MapEntry<'ast>>,
definitions: Definitions)
-> NodeCollector<'ast> {
Expand All @@ -60,8 +61,14 @@ impl<'ast> NodeCollector<'ast> {
definitions: definitions,
};

assert_eq!(parent_def_path.krate, parent_def_id.krate);
let root_path = Box::new(InlinedRootPath {
data: parent_def_path.data,
def_id: parent_def_id,
});

collector.insert_entry(parent_node, RootInlinedParent(parent));
collector.create_def(parent_node, DefPathData::InlinedRoot(parent_def_path));
collector.create_def(parent_node, DefPathData::InlinedRoot(root_path));

collector
}
Expand Down Expand Up @@ -126,11 +133,16 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> {
// Pick the def data. This need not be unique, but the more
// information we encapsulate into
let def_data = match i.node {
ItemDefaultImpl(..) | ItemImpl(..) => DefPathData::Impl(i.name),
ItemEnum(..) | ItemStruct(..) | ItemTrait(..) => DefPathData::Type(i.name),
ItemExternCrate(..) | ItemMod(..) => DefPathData::Mod(i.name),
ItemStatic(..) | ItemConst(..) | ItemFn(..) => DefPathData::Value(i.name),
_ => DefPathData::Misc,
ItemDefaultImpl(..) | ItemImpl(..) =>
DefPathData::Impl,
ItemEnum(..) | ItemStruct(..) | ItemTrait(..) |
ItemExternCrate(..) | ItemMod(..) | ItemForeignMod(..) |
ItemTy(..) =>
DefPathData::TypeNs(i.name),
ItemStatic(..) | ItemConst(..) | ItemFn(..) =>
DefPathData::ValueNs(i.name),
ItemUse(..) =>
DefPathData::Misc,
};

self.insert_def(i.id, NodeItem(i), def_data);
Expand Down Expand Up @@ -195,7 +207,7 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> {
fn visit_foreign_item(&mut self, foreign_item: &'ast ForeignItem) {
self.insert_def(foreign_item.id,
NodeForeignItem(foreign_item),
DefPathData::Value(foreign_item.name));
DefPathData::ValueNs(foreign_item.name));

let parent_node = self.parent_node;
self.parent_node = foreign_item.id;
Expand All @@ -215,8 +227,8 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> {

fn visit_trait_item(&mut self, ti: &'ast TraitItem) {
let def_data = match ti.node {
MethodTraitItem(..) | ConstTraitItem(..) => DefPathData::Value(ti.name),
TypeTraitItem(..) => DefPathData::Type(ti.name),
MethodTraitItem(..) | ConstTraitItem(..) => DefPathData::ValueNs(ti.name),
TypeTraitItem(..) => DefPathData::TypeNs(ti.name),
};

self.insert(ti.id, NodeTraitItem(ti));
Expand All @@ -239,8 +251,8 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> {

fn visit_impl_item(&mut self, ii: &'ast ImplItem) {
let def_data = match ii.node {
ImplItemKind::Method(..) | ImplItemKind::Const(..) => DefPathData::Value(ii.name),
ImplItemKind::Type(..) => DefPathData::Type(ii.name),
ImplItemKind::Method(..) | ImplItemKind::Const(..) => DefPathData::ValueNs(ii.name),
ImplItemKind::Type(..) => DefPathData::TypeNs(ii.name),
};

self.insert_def(ii.id, NodeImplItem(ii), def_data);
Expand Down
126 changes: 84 additions & 42 deletions src/librustc/front/map/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,23 +59,94 @@ pub struct DefData {
pub node_id: ast::NodeId,
}

pub type DefPath = Vec<DisambiguatedDefPathData>;
#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub struct DefPath {
/// the path leading from the crate root to the item
pub data: Vec<DisambiguatedDefPathData>,

/// what krate root is this path relative to?
pub krate: ast::CrateNum,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

}

impl DefPath {
pub fn is_local(&self) -> bool {
self.krate == LOCAL_CRATE
}

pub fn make<FN>(start_krate: ast::CrateNum,
start_index: DefIndex,
mut get_key: FN) -> DefPath
where FN: FnMut(DefIndex) -> DefKey
{
let mut krate = start_krate;
let mut data = vec![];
let mut index = Some(start_index);
loop {
let p = index.unwrap();
let key = get_key(p);
match key.disambiguated_data.data {
DefPathData::CrateRoot => {
assert!(key.parent.is_none());
break;
}
DefPathData::InlinedRoot(ref p) => {
assert!(key.parent.is_none());
assert!(!p.def_id.is_local());
data.extend(p.data.iter().cloned().rev());
krate = p.def_id.krate;
break;
}
_ => {
data.push(key.disambiguated_data);
index = key.parent;
}
}
}
data.reverse();
DefPath { data: data, krate: krate }
}
}

/// Root of an inlined item. We track the `DefPath` of the item within
/// 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.
Copy link
Member

Choose a reason for hiding this comment

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

MIR can't come soon enough 😞.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

///
/// Some of the constraints that led to the current approach:
///
/// - I don't want to have a `DefId` in the main `DefPath` because
/// that gets serialized for incr. comp., and when reloaded the
/// `DefId` is no longer valid. I'd rather maintain the invariant
/// that every `DefId` is valid, and a potentially outdated `DefId` is
/// represented as a `DefPath`.
/// - (We don't serialize def-paths from inlined items, so it's ok to have one here.)
/// - We need to be able to extract the def-id from inline items to
/// make the symbol name. In theory we could retrace it from the
/// data, but the metadata doesn't have the required indices, and I
/// don't want to write the code to create one just for this.
/// - It may be that we don't actually need `data` at all. We'll have
/// to see about that.
#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub struct InlinedRootPath {
pub data: Vec<DisambiguatedDefPathData>,
pub def_id: DefId,
}

#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub enum DefPathData {
// Root: these should only be used for the root nodes, because
// they are treated specially by the `def_path` function.
CrateRoot,
InlinedRoot(DefPath),
InlinedRoot(Box<InlinedRootPath>),

// Catch-all for random DefId things like DUMMY_NODE_ID
Misc,

// Different kinds of items and item-like things:
Impl(ast::Name),
Type(ast::Name),
Mod(ast::Name),
Value(ast::Name),
Impl,
TypeNs(ast::Name), // something in the type NS
ValueNs(ast::Name), // something in the value NS
MacroDef(ast::Name),
ClosureExpr,

Expand All @@ -87,10 +158,6 @@ pub enum DefPathData {
StructCtor, // implicit ctor for a tuple-like struct
Initializer, // initializer for a const
Binding(ast::Name), // pattern binding

// An external crate that does not have an `extern crate` in this
// crate.
DetachedCrate(ast::Name),
}

impl Definitions {
Expand All @@ -116,7 +183,7 @@ impl Definitions {
/// will be the path of the item in the external crate (but the
/// path will begin with the path to the external crate).
pub fn def_path(&self, index: DefIndex) -> DefPath {
make_def_path(index, |p| self.def_key(p))
DefPath::make(LOCAL_CRATE, index, |p| self.def_key(p))
}

pub fn opt_def_index(&self, node: ast::NodeId) -> Option<DefIndex> {
Expand Down Expand Up @@ -175,20 +242,21 @@ impl DefPathData {
pub fn as_interned_str(&self) -> InternedString {
use self::DefPathData::*;
match *self {
Impl(name) |
Type(name) |
Mod(name) |
Value(name) |
TypeNs(name) |
ValueNs(name) |
MacroDef(name) |
TypeParam(name) |
LifetimeDef(name) |
EnumVariant(name) |
DetachedCrate(name) |
Binding(name) |
Field(name) => {
name.as_str()
}

Impl => {
InternedString::new("{{impl}}")
}

// note that this does not show up in user printouts
CrateRoot => {
InternedString::new("{{root}}")
Expand Down Expand Up @@ -222,29 +290,3 @@ impl DefPathData {
}
}

pub fn make_def_path<FN>(start_index: DefIndex, mut get_key: FN) -> DefPath
where FN: FnMut(DefIndex) -> DefKey
{
let mut result = vec![];
let mut index = Some(start_index);
while let Some(p) = index {
let key = get_key(p);
match key.disambiguated_data.data {
DefPathData::CrateRoot => {
assert!(key.parent.is_none());
break;
}
DefPathData::InlinedRoot(ref p) => {
assert!(key.parent.is_none());
result.extend(p.iter().cloned().rev());
break;
}
_ => {
result.push(key.disambiguated_data);
index = key.parent;
}
}
}
result.reverse();
result
}
Loading