Skip to content
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

[debuginfo] Make cpp-like debuginfo type names for slices and str consistent. #103691

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Oct 28, 2022

Before this PR, the compiler would emit the debuginfo name slice$<T> for all kinds of slices, regardless of whether they are behind a reference or not and regardless of the kind of reference. As a consequence, the types Foo<&[T]>, Foo<[T]>, and Foo<&mut [T]> would end up with the same type name Foo<slice$<T> > in debuginfo, making it impossible to disambiguate between them by name. Similarly, &str would get the name str in debuginfo, so the debuginfo name for Foo<str> and Foo<&str> would be the same. In contrast, *const [bool] and *mut [bool] would be ptr_const$<slice$<bool> > and ptr_mut$<slice$<bool> >, i.e. the encoding does not lose information about the type.

This PR removes all special handling for slices and str. The types &[bool], &mut [bool], and &str thus get the names ref$<slice2$<bool> >, ref_mut$<slice2$<bool> >, and ref$<str$> respectively -- as one would expect.

The new special name for slices is slice2$ to differentiate it from the previous name slice$, which has different semantics. The same is true for str and str$. This kind of versioning already has a precedent with the case of enum$ and enum2$ and hopefully will make it easier to transition existing consumers of these names.

cc @rust-lang/wg-debugging @vadimcn

r? @wesleywiser

UPDATE: Here is a table to clarify the changes

Rust type DWARF name C++-like name (before) C++-like name (after)
[T] [T] slice$<T> slice2$<T>
&[T] &[T] slice$<T> ref$<slice2$<T> >
&mut [T] &mut [T] slice$<T> ref_mut$<slice2$<T> >
str str str str$
&str &str str ref$<str$>
&mut str &mut str str ref_mut$<str$>
*const [T] *const [T] ptr_const$<slice$<T> > ptr_const$<slice2$<T> >
*mut [T] *mut [T] ptr_mut$<slice$<T> > ptr_mut$<slice2$<T> >

As you can see, before the PR many types would end up with the same name, making it impossible to distinguish between them in NatVis or other places where types are matched or looked up by name. The DWARF version of names is not changed.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 28, 2022
@michaelwoerister michaelwoerister force-pushed the consistent-slice-and-str-cpp-like-debuginfo-names branch from 84986c2 to f285bc9 Compare October 28, 2022 15:12
@rust-log-analyzer

This comment has been minimized.

Before this PR, the compiler would emit the debuginfo name `slice$<T>`
for all kinds of slices, regardless of whether they are behind a
reference or not and regardless of the kind of reference. As a
consequence, the types `Foo<&[T]>`, `Foo<[T]>`, and `Foo<&mut [T]>`
would end up with the same type name `Foo<slice$<T> >` in debuginfo,
making it impossible to disambiguate between them by name. Similarly,
`&str` would get the name `str` in debuginfo, so the debuginfo name for
`Foo<str>` and `Foo<&str>` would be the same. In contrast,
`*const [bool]` and `*mut [bool]` would be `ptr_const$<slice$<bool> >`
and `ptr_mut$<slice$<bool> >`, i.e. the encoding does not lose
information about the type.

This PR removes all special handling for slices and `str`. The types
`&[bool]`, `&mut [bool]`, and `&str` thus get the names
`ref$<slice2$<bool> >`, `ref_mut$<slice2$<bool> >`, and
`ref$<str$>` respectively -- as one would expect.
@michaelwoerister michaelwoerister force-pushed the consistent-slice-and-str-cpp-like-debuginfo-names branch from f285bc9 to 0cd2dd7 Compare October 31, 2022 14:43
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @michaelwoerister!

@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 2, 2022

📌 Commit 0cd2dd7 has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors 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 Nov 2, 2022
@bors
Copy link
Contributor

bors commented Nov 5, 2022

⌛ Testing commit 0cd2dd7 with merge b0f3940...

@bors
Copy link
Contributor

bors commented Nov 5, 2022

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing b0f3940 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 5, 2022
@bors bors merged commit b0f3940 into rust-lang:master Nov 5, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 5, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b0f3940): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.4%, 0.4%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.6% [0.6%, 0.6%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.2% [-1.0%, 0.6%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.9% [-0.9%, -0.9%] 1
Improvements ✅
(secondary)
-2.6% [-3.0%, -2.1%] 4
All ❌✅ (primary) -0.2% [-0.9%, 0.5%] 2

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…d-str-cpp-like-debuginfo-names, r=wesleywiser

[debuginfo] Make cpp-like debuginfo type names for slices and str consistent.

Before this PR, the compiler would emit the debuginfo name `slice$<T>` for all kinds of slices, regardless of whether they are behind a reference or not and regardless of the kind of reference. As a consequence, the types `Foo<&[T]>`, `Foo<[T]>`, and `Foo<&mut [T]>` would end up with the same type name `Foo<slice$<T> >` in debuginfo, making it impossible to disambiguate between them by name. Similarly, `&str` would get the name `str` in debuginfo, so the debuginfo name for `Foo<str>` and `Foo<&str>` would be the same. In contrast, `*const [bool]` and `*mut [bool]` would be `ptr_const$<slice$<bool> >` and `ptr_mut$<slice$<bool> >`, i.e. the encoding does not lose information about the type.

This PR removes all special handling for slices and `str`. The types `&[bool]`, `&mut [bool]`, and `&str` thus get the names `ref$<slice2$<bool> >`, `ref_mut$<slice2$<bool> >`, and `ref$<str$>` respectively -- as one would expect.

The new special name for slices is `slice2$` to differentiate it from the previous name `slice$`, which has different semantics. The same is true for `str` and `str$`. This kind of versioning already has a precedent with the case of `enum$` and `enum2$` and hopefully will make it easier to transition existing consumers of these names.

cc `@rust-lang/wg-debugging` `@vadimcn`

r? `@wesleywiser`

UPDATE: Here is a table to clarify the changes

| Rust type | DWARF name | C++-like name (before) | C++-like name (after) |
|-----------|------------|------------------------|------------------------|
| `[T]`        | `[T]`        | `slice$<T>`              | `slice2$<T>`           |
| `&[T]`       | `&[T]`       | `slice$<T>`              | `ref$<slice2$<T> >`    |
| `&mut [T]`   | `&mut [T]`   | `slice$<T>`              | `ref_mut$<slice2$<T> >`|
| `str`        | `str`        | `str`                    | `str$`           |
| `&str`       | `&str`       | `str`                    | `ref$<str$>`    |
| `&mut str`   | `&mut str`   | `str`                    | `ref_mut$<str$>`|
| `*const [T]` | `*const [T]` | `ptr_const$<slice$<T> >` | `ptr_const$<slice2$<T> >` |
| `*mut [T]`   | `*mut [T]`   | `ptr_mut$<slice$<T> >`   | `ptr_mut$<slice2$<T> >` |

As you can see, before the PR many types would end up with the same name, making it impossible to distinguish between them in NatVis or other places where types are matched or looked up by name. The DWARF version of names is not changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants