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

Improve debugging experience for enums on windows-msvc #85292

Merged
merged 10 commits into from
Jun 3, 2021

Conversation

wesleywiser
Copy link
Member

This PR makes significant improvements over the status quo of debugging enums on the windows-msvc platform with either WinDbg or Visual Studio in three ways:

  1. Improves the debugger experience for directly tagged enums.
  2. Fixes a bug which caused the debugger to sometimes show the wrong debug info for niche layout enums. For example, Option<&u32> could sometimes use the debug info for Option<&f64> instead leading to nonsensical variable values in the debugger.
  3. Significantly improves the debugger experience for niche-layout enums.

Let's look at a few examples:

pub enum CStyleEnum {
    Base = 2,
    Exponent = 16,
}

pub enum NicheLayoutEnum {
    Tag1,
    Data { my_data: CStyleEnum },
    Tag2,
    Tag3,
    Tag4,
}

pub enum OtherEnum<T> {
    Case1(T),
    Case2(T),
}

fn main() {
    let a = Some(CStyleEnum::Base);
    let b = Option::<CStyleEnum>::None;
    let c = NicheLayoutEnum::Tag1;
    let d = NicheLayoutEnum::Data { my_data: CStyleEnum::Exponent };
    let e = NicheLayoutEnum::Tag2;
    let f = Some(&1u32);
    let g = Option::<&'static u32>::None;
    let h = Some(&2u64);
    let i = Option::<&'static u64>::None;
    let j = Some(12u32);
    let k = Option::<u32>::None;
    let l = Some(12.34f64);
    let m = Option::<f64>::None;
    let n = CStyleEnum::Base;
    let o = CStyleEnum::Exponent;
    let p = Some("IAMA optional string!".to_string());
    let q = OtherEnum::Case1(42u32);
}

This is what WinDbg Preview shows using the latest rustc nightly:

image

Most of the variables don't show a meaningful value expect for a few cases that we have targeted natvis definitions covering. Even worse, drilling into many of these variables shows information that can be difficult to interpret without an understanding of the layout of Rust types:

image

With the changes in this PR, we're able to write two natvis definitions that cover all enum cases generally. After building with these changes, WinDbg now shows this instead:

image

Drilling into the same variables, we can see much more useful information:

image

Fixes #84670
Fixes #84671

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2021
@wesleywiser
Copy link
Member Author

As a question for reviewers, if you look at the screenshots, you'll notice that sometimes Option::Some is visualized as Some({...}) and sometimes it's just Some. This is because the niche-layout visualization knows the Some variant has data but the general enum visualization does not know which variants have data and which do not. I chose to do it this way because it does improve the experience of niche-layout enums in general but it also causes this inconsistency.

I think it is possible to further improve the general enum visualization further in follow up PRs. Please let me know if you have an opinion on the current state.

@rust-log-analyzer

This comment has been minimized.

@wesleywiser wesleywiser added A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) O-windows-msvc Toolchain: MSVC, Operating system: Windows labels May 14, 2021
Copy link
Contributor

@vadimcn vadimcn 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 tackling this!

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/debuginfo/type_names.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
@wesleywiser
Copy link
Member Author

Assigning to @michaelwoerister since he's looking at #85269 as well.

r? @michaelwoerister

Copy link
Member

@michaelwoerister michaelwoerister 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 the PR, @wesleywiser! This looks like it should really improve the debugging experience for enums. It's great that you found an encoding that NatVis can handle robustly. I'm sure it was a lot of work to get there!

I've left some comments below. I'd also like to suggest to make the compiler generated names more consistent:

  • type names should start with an uppercase letter (e.g. Tag$ instead of tag$),
  • field names should start with a lowercase letter (e.g. variant0 instead of Variant0),
  • either all generated names should end with $ or only those that could potentially conflict with a non-generated name.
  • use only one of the terms tag$, discriminant$, and variant$. Right now all three of them seem to be in use but refer to the same thing.

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
src/etc/natvis/intrinsic.natvis Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
@wesleywiser wesleywiser added A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) and removed A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) labels May 26, 2021
@wesleywiser wesleywiser force-pushed the enum_debuginfo branch 2 times, most recently from b1b8328 to 1c36b00 Compare May 28, 2021 20:01
@wesleywiser
Copy link
Member Author

Thanks for the review @michaelwoerister! I think I've addressed all your feedback in the latest commit.

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

Thanks, @wesleywiser! I think this is almost ready to merge. I left a few comments below. I'm being nitpicky about typo because, for someone who does not know the code, it can be very hard to tell if something is just an oversight or if it has some kind of special meaning.

Otherwise, consider the PR approved by me 😀

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
// ```c
// union enum$<{name}> {
// struct {variant 0 name} {
// tag$ variant$;
Copy link
Member

Choose a reason for hiding this comment

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

I still think it would be more consistent if the variant$ field here and the discriminant field in the niche-layout union would use the same name (e.g. discriminant$ or tag$) 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

Eventually, I would like to have all enums use the [variant] synthetic item to name the variant they are currently in and hide all of the discriminant fields. This is how it currently works for niche-layout enums but not for directly tagged ones. This will require some more work on the directly tagged case and I'd like to go ahead and get this merged since I don't think it's worth holding this improvement up for that as variant$ is still better than RUST$ENUM$DISR which we have today.

I would also rather not change the niche-layout ones as discriminant is the correct terminology to use in that case. Feel free to r- if you think we should still do something different here!

Copy link
Contributor

Choose a reason for hiding this comment

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

@wesleywiser So are you planning more changes for enums?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm hoping to have time this week to get to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, it'd be nice to move discriminant out of the variant structs. Otherwise it can be seen when displaying fields of the variant struct, and hiding it is kinda annoying.

Also, I notice that tuple struct variants are not tagged as such, which means that tuple visualizers won't be applied to them.
(i.e. enum$<EnumName>::VariantX should be tuple<enum$<EnumName>::VariantX>?)

@wesleywiser
Copy link
Member Author

@bors r=michaelwoerister

@bors
Copy link
Contributor

bors commented Jun 1, 2021

📌 Commit 6d0afc473bca02cbbc528b12116a0cb5320a832b has been approved by michaelwoerister

@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 Jun 1, 2021
@bors
Copy link
Contributor

bors commented Jun 1, 2021

⌛ Testing commit 6d0afc473bca02cbbc528b12116a0cb5320a832b with merge 4b3463e626e30ed451eeadabed0e44099cf9402e...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 1, 2021

💔 Test failed - checks-actions

@wesleywiser
Copy link
Member Author

@bors r=michaelwoerister

@bors
Copy link
Contributor

bors commented Jun 2, 2021

📌 Commit 94c45ef has been approved by michaelwoerister

@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 Jun 2, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 3, 2021
…elwoerister

Improve debugging experience for enums on windows-msvc

This PR makes significant improvements over the status quo of debugging enums on the windows-msvc platform with either WinDbg or Visual Studio in three ways:

1. Improves the debugger experience for directly tagged enums.
2. Fixes a bug which caused the debugger to sometimes show the wrong debug info for niche layout enums. For example, `Option<&u32>` could sometimes use the debug info for `Option<&f64>` instead leading to nonsensical variable values in the debugger.
3. Significantly improves the debugger experience for niche-layout enums.

Let's look at a few examples:

```rust
pub enum CStyleEnum {
    Base = 2,
    Exponent = 16,
}

pub enum NicheLayoutEnum {
    Tag1,
    Data { my_data: CStyleEnum },
    Tag2,
    Tag3,
    Tag4,
}

pub enum OtherEnum<T> {
    Case1(T),
    Case2(T),
}

fn main() {
    let a = Some(CStyleEnum::Base);
    let b = Option::<CStyleEnum>::None;
    let c = NicheLayoutEnum::Tag1;
    let d = NicheLayoutEnum::Data { my_data: CStyleEnum::Exponent };
    let e = NicheLayoutEnum::Tag2;
    let f = Some(&1u32);
    let g = Option::<&'static u32>::None;
    let h = Some(&2u64);
    let i = Option::<&'static u64>::None;
    let j = Some(12u32);
    let k = Option::<u32>::None;
    let l = Some(12.34f64);
    let m = Option::<f64>::None;
    let n = CStyleEnum::Base;
    let o = CStyleEnum::Exponent;
    let p = Some("IAMA optional string!".to_string());
    let q = OtherEnum::Case1(42u32);
}
```

This is what WinDbg Preview shows using the latest rustc nightly:

![image](https://user-images.githubusercontent.com/831192/118285353-57c10780-b49f-11eb-97aa-db3abfc09508.png)

Most of the variables don't show a meaningful value expect for a few cases that we have targeted natvis definitions covering. Even worse, drilling into many of these variables shows information that can be difficult to interpret without an understanding of the layout of Rust types:

![image](https://user-images.githubusercontent.com/831192/118285609-a1a9ed80-b49f-11eb-9c29-b14576984647.png)

With the changes in this PR, we're able to write two natvis definitions that cover all enum cases generally. After building with these changes, WinDbg now shows this instead:

![image](https://user-images.githubusercontent.com/831192/118287730-be472500-b4a1-11eb-8cad-8f6a91c7516b.png)

Drilling into the same variables, we can see much more useful information:

![image](https://user-images.githubusercontent.com/831192/118287888-e20a6b00-b4a1-11eb-927f-32cf33a31c16.png)

Fixes rust-lang#84670
Fixes rust-lang#84671
@bors
Copy link
Contributor

bors commented Jun 3, 2021

⌛ Testing commit 94c45ef with merge 431ea5ab6bee9d04b74e56a0c24c65a3e524b7ea...

@bors
Copy link
Contributor

bors commented Jun 3, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 3, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@wesleywiser
Copy link
Member Author

@bors retry

@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 Jun 3, 2021
@bors
Copy link
Contributor

bors commented Jun 3, 2021

⌛ Testing commit 94c45ef with merge 2577825...

@bors
Copy link
Contributor

bors commented Jun 3, 2021

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 2577825 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2021
@bors bors merged commit 2577825 into rust-lang:master Jun 3, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 3, 2021
wesleywiser added a commit to wesleywiser/rust that referenced this pull request Jul 3, 2021
This isn't used anymore after rust-lang#85292
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Aug 12, 2021
Pkgsrc changes:
 * Bump bootstrap requirements to 1.53.0.
 * Adjust patches, adapt to upstream changes, adjust cargo checksums
 * If using an external llvm, require >= 10.0

Upsteream changes:

Version 1.54.0 (2021-07-29)
============================

Language
-----------------------

- [You can now use macros for values in built-in attribute macros.][83366]
  While a seemingly minor addition on its own, this enables a lot of
  powerful functionality when combined correctly. Most notably you can
  now include external documentation in your crate by writing the following.
  ```rust
  #![doc = include_str!("README.md")]
  ```
  You can also use this to include auto-generated modules:
  ```rust
  #[path = concat!(env!("OUT_DIR"), "/generated.rs")]
  mod generated;
  ```

- [You can now cast between unsized slice types (and types which contain
  unsized slices) in `const fn`.][85078]
- [You can now use multiple generic lifetimes with `impl Trait` where the
   lifetimes don't explicitly outlive another.][84701] In code this means
   that you can now have `impl Trait<'a, 'b>` where as before you could
   only have `impl Trait<'a, 'b> where 'b: 'a`.

Compiler
-----------------------

- [Rustc will now search for custom JSON targets in
  `/lib/rustlib/<target-triple>/target.json` where `/` is the "sysroot"
  directory.][83800] You can find your sysroot directory by running
  `rustc --print sysroot`.
- [Added `wasm` as a `target_family` for WebAssembly platforms.][84072]
- [You can now use `#[target_feature]` on safe functions when targeting
  WebAssembly platforms.][84988]
- [Improved debugger output for enums on Windows MSVC platforms.][85292]
- [Added tier 3\* support for `bpfel-unknown-none`
   and `bpfeb-unknown-none`.][79608]

\* Refer to Rust's [platform support page][platform-support-doc] for more
   information on Rust's tiered platform support.

Libraries
-----------------------

- [`panic::panic_any` will now `#[track_caller]`.][85745]
- [Added `OutOfMemory` as a variant of `io::ErrorKind`.][84744]
- [ `proc_macro::Literal` now implements `FromStr`.][84717]
- [The implementations of vendor intrinsics in core::arch have been
   significantly refactored.][83278] The main user-visible changes are
   a 50% reduction in the size of libcore.rlib and stricter validation
   of constant operands passed to intrinsics. The latter is technically
   a breaking change, but allows Rust to more closely match the C vendor
   intrinsics API.

Stabilized APIs
---------------

- [`BTreeMap::into_keys`]
- [`BTreeMap::into_values`]
- [`HashMap::into_keys`]
- [`HashMap::into_values`]
- [`arch::wasm32`]
- [`VecDeque::binary_search`]
- [`VecDeque::binary_search_by`]
- [`VecDeque::binary_search_by_key`]
- [`VecDeque::partition_point`]

Cargo
-----

- [Added the `--prune <spec>` option to `cargo-tree` to remove a package from
  the dependency graph.][cargo/9520]
- [Added the `--depth` option to `cargo-tree` to print only to a certain depth
  in the tree ][cargo/9499]
- [Added the `no-proc-macro` value to `cargo-tree --edges` to hide procedural
  macro dependencies.][cargo/9488]
- [A new environment variable named `CARGO_TARGET_TMPDIR` is
  available.][cargo/9375]
  This variable points to a directory that integration tests and
  benches can use as a "scratchpad" for testing filesystem operations.

Compatibility Notes
-------------------
- [Mixing Option and Result via `?` is no longer permitted in
  closures for inferred types.][86831]
- [Previously unsound code is no longer permitted where different
  constructors in branches could require different lifetimes.][85574]
- As previously mentioned the [`std::arch` instrinsics now uses
  stricter const checking][83278] than before and may reject some
  previously accepted code.
- [`i128` multiplication on Cortex M0+ platforms currently
  unconditionally causes overflow when compiled with `codegen-units
  = 1`.][86063]

[85574]: rust-lang/rust#85574
[86831]: rust-lang/rust#86831
[86063]: rust-lang/rust#86063
[86831]: rust-lang/rust#86831
[79608]: rust-lang/rust#79608
[84988]: rust-lang/rust#84988
[84701]: rust-lang/rust#84701
[84072]: rust-lang/rust#84072
[85745]: rust-lang/rust#85745
[84744]: rust-lang/rust#84744
[85078]: rust-lang/rust#85078
[84717]: rust-lang/rust#84717
[83800]: rust-lang/rust#83800
[83366]: rust-lang/rust#83366
[83278]: rust-lang/rust#83278
[85292]: rust-lang/rust#85292
[cargo/9520]: rust-lang/cargo#9520
[cargo/9499]: rust-lang/cargo#9499
[cargo/9488]: rust-lang/cargo#9488
[cargo/9375]: rust-lang/cargo#9375
[`BTreeMap::into_keys`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.into_keys
[`BTreeMap::into_values`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.into_values
[`HashMap::into_keys`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.into_keys
[`HashMap::into_values`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.into_values
[`arch::wasm32`]: https://doc.rust-lang.org/core/arch/wasm32/index.html
[`VecDeque::binary_search`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search
[`VecDeque::binary_search_by`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search_by
[`VecDeque::binary_search_by_key`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search_by_key
[`VecDeque::partition_point`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.partition_point
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) merged-by-bors This PR was explicitly merged by bors. O-windows-msvc Toolchain: MSVC, Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
9 participants