Skip to content

Commit

Permalink
Auto merge of #87153 - michaelwoerister:debuginfo-names-dyn-trait-pro…
Browse files Browse the repository at this point in the history 
…jection-bounds, r=wesleywiser

[debuginfo] Emit associated type bindings in trait object type names.

This PR updates debuginfo type name generation for trait objects to include associated type bindings and auto trait bounds -- so that, for example, the debuginfo type name of `&dyn Iterator<Item=Foo>` and `&dyn Iterator<Item=Bar>` don't both map to just `&dyn Iterator` anymore.

The following table shows examples of debuginfo type names before and after the PR:
| type | before |  after |
|------|---------|-------|
| `&dyn Iterator<Item=u32>>` | `&dyn Iterator` | `&dyn Iterator<Item=u32>` |
| `&(dyn Iterator<Item=u32>> + Sync)` | `&dyn Iterator` | `&(dyn Iterator<Item=u32> + Sync)` |
| `&(dyn SomeTrait<bool, i8, Bar=u32>> + Send)` | `&dyn SomeTrait<bool, i8>` | `&(dyn SomeTrait<bool, i8, Bar=u32>> + Send)`  |

For targets that need C++-like type names, we use `assoc$<Item,u32>` instead of `Item=u32`:
| type | before |  after |
|------|---------|-------|
| `&dyn Iterator<Item=u32>>` | `ref$<dyn$<Iterator> >` | `ref$<dyn$<Iterator<assoc$<Item,u32> > > >` |
| `&(dyn Iterator<Item=u32>> + Sync)` | `ref$<dyn$<Iterator> >` | `ref$<dyn$<Iterator<assoc$<Item,u32> >,Sync> >` |
| `&(dyn SomeTrait<bool, i8, Bar=u32>> + Send)` | `ref$<dyn$<SomeTrait<bool, i8> > >` | `ref$<dyn$<SomeTrait<bool,i8,assoc$<Bar,u32> > >,Send> >`  |

The PR also adds self-profiling measurements for debuginfo type name generation (re. #86431). It looks like the compiler spends up to 0.5% of its time in that task, so the potential for optimizing it via caching seems limited.

However, the perf run also shows [the biggest regression](https://perf.rust-lang.org/detailed-query.html?commit=585e91c718b0b2c5319e1fffd0ff1e62aaf7ccc2&base_commit=b9197978a90be6f7570741eabe2da175fec75375&benchmark=tokio-webpush-simple-debug&run_name=incr-unchanged) in a test case that does not even invoke the code in question. This suggests that the length of the names we generate here can affect performance by influencing how much data the linker has to copy around.

Fixes #86134.
  • Loading branch information
@bors
bors committed Jul 19, 2021
2 parents d5af634 + 5b1bfae commit 014026d
Show file tree
Hide file tree
Showing 10 changed files with 182 additions and 108 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3747,6 +3747,7 @@ dependencies = [
"rustc_span",
"rustc_symbol_mangling",
"rustc_target",
"smallvec",
"tempfile",
"tracing",
]
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ libc = "0.2.50"
jobserver = "0.1.22"
tempfile = "3.2"
pathdiff = "0.2.0"
smallvec = { version = "1.6.1", features = ["union", "may_dangle"] }

rustc_serialize = { path = "../rustc_serialize" }
rustc_ast = { path = "../rustc_ast" }
Expand Down
210 changes: 139 additions & 71 deletions compiler/rustc_codegen_ssa/src/debuginfo/type_names.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ use rustc_hir::definitions::{DefPathData, DefPathDataName, DisambiguatedDefPathD
use rustc_middle::ich::NodeIdHashingMode;
use rustc_middle::ty::layout::IntegerExt;
use rustc_middle::ty::subst::{GenericArgKind, SubstsRef};
use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt};
use rustc_middle::ty::{self, AdtDef, ExistentialProjection, Ty, TyCtxt};
use rustc_target::abi::{Integer, TagEncoding, Variants};
use smallvec::SmallVec;

use std::fmt::Write;

Expand All @@ -33,6 +34,8 @@ pub fn compute_debuginfo_type_name<'tcx>(
t: Ty<'tcx>,
qualified: bool,
) -> String {
let _prof = tcx.prof.generic_activity("compute_debuginfo_type_name");

let mut result = String::with_capacity(64);
let mut visited = FxHashSet::default();
push_debuginfo_type_name(tcx, t, qualified, &mut result, &mut visited);
Expand All @@ -41,7 +44,7 @@ pub fn compute_debuginfo_type_name<'tcx>(

// Pushes the name of the type as it should be stored in debuginfo on the
// `output` String. See also compute_debuginfo_type_name().
pub fn push_debuginfo_type_name<'tcx>(
fn push_debuginfo_type_name<'tcx>(
tcx: TyCtxt<'tcx>,
t: Ty<'tcx>,
qualified: bool,
Expand Down Expand Up @@ -84,25 +87,14 @@ pub fn push_debuginfo_type_name<'tcx>(

for component_type in component_types {
push_debuginfo_type_name(tcx, component_type.expect_ty(), true, output, visited);
output.push(',');

// Natvis does not always like having spaces between parts of the type name
// and this causes issues when we need to write a typename in natvis, for example
// as part of a cast like the `HashMap` visualizer does.
if !cpp_like_names {
output.push(' ');
}
push_arg_separator(cpp_like_names, output);
}
if !component_types.is_empty() {
output.pop();

if !cpp_like_names {
output.pop();
}
pop_arg_separator(output);
}

if cpp_like_names {
push_close_angle_bracket(tcx, output);
push_close_angle_bracket(cpp_like_names, output);
} else {
output.push(')');
}
Expand All @@ -124,7 +116,7 @@ pub fn push_debuginfo_type_name<'tcx>(
push_debuginfo_type_name(tcx, inner_type, qualified, output, visited);

if cpp_like_names {
push_close_angle_bracket(tcx, output);
push_close_angle_bracket(cpp_like_names, output);
}
}
ty::Ref(_, inner_type, mutbl) => {
Expand All @@ -150,7 +142,7 @@ pub fn push_debuginfo_type_name<'tcx>(
push_debuginfo_type_name(tcx, inner_type, qualified, output, visited);

if cpp_like_names && !is_slice_or_str {
push_close_angle_bracket(tcx, output);
push_close_angle_bracket(cpp_like_names, output);
}
}
ty::Array(inner_type, len) => {
Expand Down Expand Up @@ -182,69 +174,97 @@ pub fn push_debuginfo_type_name<'tcx>(
push_debuginfo_type_name(tcx, inner_type, true, output, visited);

if cpp_like_names {
push_close_angle_bracket(tcx, output);
push_close_angle_bracket(cpp_like_names, output);
} else {
output.push(']');
}
}
ty::Dynamic(ref trait_data, ..) => {
if cpp_like_names {
let auto_traits: SmallVec<[DefId; 4]> = trait_data.auto_traits().collect();

let has_enclosing_parens = if cpp_like_names {
output.push_str("dyn$<");
false
} else {
output.push_str("dyn ");
}
if trait_data.len() > 1 && auto_traits.len() != 0 {
// We need enclosing parens because there is more than one trait
output.push_str("(dyn ");
true
} else {
output.push_str("dyn ");
false
}
};

if let Some(principal) = trait_data.principal() {
let principal =
tcx.normalize_erasing_late_bound_regions(ty::ParamEnv::reveal_all(), principal);
push_item_name(tcx, principal.def_id, qualified, output);
push_generic_params_internal(tcx, principal.substs, output, visited);
} else {
// The auto traits come ordered by `DefPathHash`, which guarantees stability if the
// environment is stable (e.g., incremental builds) but not otherwise (e.g.,
// updated compiler version, different target).
//
// To avoid that causing instabilities in test output, sort the auto-traits
// alphabetically.
let mut auto_traits: Vec<_> = trait_data
.iter()
.filter_map(|predicate| {
match tcx.normalize_erasing_late_bound_regions(
ty::ParamEnv::reveal_all(),
predicate,
) {
ty::ExistentialPredicate::AutoTrait(def_id) => {
let mut name = String::new();
push_item_name(tcx, def_id, true, &mut name);
Some(name)
}
_ => None,
}
let principal_has_generic_params =
push_generic_params_internal(tcx, principal.substs, output, visited);

let projection_bounds: SmallVec<[_; 4]> = trait_data
.projection_bounds()
.map(|bound| {
let ExistentialProjection { item_def_id, ty, .. } = bound.skip_binder();
(item_def_id, ty)
})
.collect();
auto_traits.sort();

for name in auto_traits {
output.push_str(&name);
if projection_bounds.len() != 0 {
if principal_has_generic_params {
// push_generic_params_internal() above added a `>` but we actually
// want to add more items to that list, so remove that again.
pop_close_angle_bracket(output);
}

if cpp_like_names {
output.push_str(", ");
} else {
output.push_str(" + ");
for (item_def_id, ty) in projection_bounds {
push_arg_separator(cpp_like_names, output);

if cpp_like_names {
output.push_str("assoc$<");
push_item_name(tcx, item_def_id, false, output);
push_arg_separator(cpp_like_names, output);
push_debuginfo_type_name(tcx, ty, true, output, visited);
push_close_angle_bracket(cpp_like_names, output);
} else {
push_item_name(tcx, item_def_id, false, output);
output.push('=');
push_debuginfo_type_name(tcx, ty, true, output, visited);
}
}

push_close_angle_bracket(cpp_like_names, output);
}

// Remove the trailing joining characters. For cpp_like_names
// this is `, ` otherwise ` + `.
output.pop();
output.pop();
if !cpp_like_names {
output.pop();
if auto_traits.len() != 0 {
push_auto_trait_separator(cpp_like_names, output);
}
}

if auto_traits.len() != 0 {
let mut auto_traits: SmallVec<[String; 4]> = auto_traits
.into_iter()
.map(|def_id| {
let mut name = String::with_capacity(20);
push_item_name(tcx, def_id, true, &mut name);
name
})
.collect();
auto_traits.sort_unstable();

for auto_trait in auto_traits {
output.push_str(&auto_trait);
push_auto_trait_separator(cpp_like_names, output);
}

pop_auto_trait_separator(output);
}

if cpp_like_names {
push_close_angle_bracket(tcx, output);
push_close_angle_bracket(cpp_like_names, output);
} else if has_enclosing_parens {
output.push(')');
}
}
ty::FnDef(..) | ty::FnPtr(_) => {
Expand Down Expand Up @@ -296,10 +316,9 @@ pub fn push_debuginfo_type_name<'tcx>(
if !sig.inputs().is_empty() {
for &parameter_type in sig.inputs() {
push_debuginfo_type_name(tcx, parameter_type, true, output, visited);
output.push_str(", ");
push_arg_separator(cpp_like_names, output);
}
output.pop();
output.pop();
pop_arg_separator(output);
}

if sig.c_variadic {
Expand Down Expand Up @@ -405,7 +424,25 @@ pub fn push_debuginfo_type_name<'tcx>(
output.push_str(&format!(", {}", variant));
}
}
push_close_angle_bracket(tcx, output);
push_close_angle_bracket(true, output);
}

const NON_CPP_AUTO_TRAIT_SEPARATOR: &str = " + ";

fn push_auto_trait_separator(cpp_like_names: bool, output: &mut String) {
if cpp_like_names {
push_arg_separator(cpp_like_names, output);
} else {
output.push_str(NON_CPP_AUTO_TRAIT_SEPARATOR);
}
}

fn pop_auto_trait_separator(output: &mut String) {
if output.ends_with(NON_CPP_AUTO_TRAIT_SEPARATOR) {
output.truncate(output.len() - NON_CPP_AUTO_TRAIT_SEPARATOR.len());
} else {
pop_arg_separator(output);
}
}
}

Expand Down Expand Up @@ -466,13 +503,15 @@ fn push_generic_params_internal<'tcx>(
substs: SubstsRef<'tcx>,
output: &mut String,
visited: &mut FxHashSet<Ty<'tcx>>,
) {
) -> bool {
if substs.non_erasable_generics().next().is_none() {
return;
return false;
}

debug_assert_eq!(substs, tcx.normalize_erasing_regions(ty::ParamEnv::reveal_all(), substs));

let cpp_like_names = cpp_like_names(tcx);

output.push('<');

for type_parameter in substs.non_erasable_generics() {
Expand All @@ -486,13 +525,12 @@ fn push_generic_params_internal<'tcx>(
other => bug!("Unexpected non-erasable generic: {:?}", other),
}

output.push_str(", ");
push_arg_separator(cpp_like_names, output);
}
pop_arg_separator(output);
push_close_angle_bracket(cpp_like_names, output);

output.pop();
output.pop();

push_close_angle_bracket(tcx, output);
true
}

fn push_const_param<'tcx>(tcx: TyCtxt<'tcx>, ct: &'tcx ty::Const<'tcx>, output: &mut String) {
Expand Down Expand Up @@ -541,20 +579,50 @@ fn push_const_param<'tcx>(tcx: TyCtxt<'tcx>, ct: &'tcx ty::Const<'tcx>, output:
}

pub fn push_generic_params<'tcx>(tcx: TyCtxt<'tcx>, substs: SubstsRef<'tcx>, output: &mut String) {
let _prof = tcx.prof.generic_activity("compute_debuginfo_type_name");
let mut visited = FxHashSet::default();
push_generic_params_internal(tcx, substs, output, &mut visited);
}

fn push_close_angle_bracket<'tcx>(tcx: TyCtxt<'tcx>, output: &mut String) {
fn push_close_angle_bracket(cpp_like_names: bool, output: &mut String) {
// MSVC debugger always treats `>>` as a shift, even when parsing templates,
// so add a space to avoid confusion.
if cpp_like_names(tcx) && output.ends_with('>') {
if cpp_like_names && output.ends_with('>') {
output.push(' ')
};

output.push('>');
}

fn pop_close_angle_bracket(output: &mut String) {
assert!(output.ends_with('>'), "'output' does not end with '>': {}", output);
output.pop();
if output.ends_with(' ') {
output.pop();
}
}

fn push_arg_separator(cpp_like_names: bool, output: &mut String) {
// Natvis does not always like having spaces between parts of the type name
// and this causes issues when we need to write a typename in natvis, for example
// as part of a cast like the `HashMap` visualizer does.
if cpp_like_names {
output.push(',');
} else {
output.push_str(", ");
};
}

fn pop_arg_separator(output: &mut String) {
if output.ends_with(' ') {
output.pop();
}

assert!(output.ends_with(','));

output.pop();
}

fn cpp_like_names(tcx: TyCtxt<'_>) -> bool {
tcx.sess.target.is_like_msvc
}
6 changes: 3 additions & 3 deletions src/test/debuginfo/function-names.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,21 @@
// cdb-command:x a!function_names::*::impl_function*
// cdb-check:[...] a!function_names::Mod1::TestStruct2::impl_function (void)
// cdb-check:[...] a!function_names::TestStruct1::impl_function (void)
// cdb-check:[...] a!function_names::GenericStruct<i32, i32>::impl_function<i32, i32> (void)
// cdb-check:[...] a!function_names::GenericStruct<i32,i32>::impl_function<i32,i32> (void)

// Trait implementations
// cdb-command:x a!function_names::*::trait_function*
// cdb-check:[...] a!function_names::impl$3::trait_function<i32> (void)
// cdb-check:[...] a!function_names::impl$6::trait_function<i32,1> (void)
// cdb-check:[...] a!function_names::impl$1::trait_function (void)
// cdb-check:[...] a!function_names::impl$6::trait_function<i32, 1> (void)
// cdb-check:[...] a!function_names::impl$5::trait_function3<function_names::TestStruct1> (void)
// cdb-check:[...] a!function_names::Mod1::impl$1::trait_function (void)

// Closure
// cdb-command:x a!function_names::*::closure*
// cdb-check:[...] a!function_names::impl$2::impl_function::closure$0<i32,i32> (void)
// cdb-check:[...] a!function_names::main::closure$0 (void)
// cdb-check:[...] a!function_names::generic_func::closure$0<i32> (void)
// cdb-check:[...] a!function_names::impl$2::impl_function::closure$0<i32, i32> (void)

// Generator
// cdb-command:x a!function_names::*::generator*
Expand Down
Loading

0 comments on commit 014026d

Please sign in to comment.