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

NatVis support in Cargo #9027

Closed
wants to merge 1 commit into from
Closed

Conversation

sivadeilra
Copy link

This allows developers to write NatVis descriptions for their crates.
If a Cargo package contains NatVis files, then they will be written
into the PDB file. Debuggers that support NatVis will automatically
locate these files and use them. Currently, Visual Studio 2019,
VS Code, and WinDbg support NatVis. (NatVis is a Windows-only feature.)

The easiest way to add a NatVis file to a Cargo package is to create a
natvis subdirectory and place the file in that directory. The file
must have a .natvis extension. Any (reasonable) number of NatVis
files can be provided.

Optionally, you can give explicit paths by setting the natvis-files
key in Cargo.toml. For example:

[package]
name = ...
natvis-files = ["my_types.natvis", "more_types.natvis"]

This key can also be set to [] in order to prevent Cargo from
searching the natvis directory.

The Rust standard library already contains NatVis files, as a
special case. After this change is committed to Cargo, the
special-case logic in Rust can be removed; then the
standard library crates can just use this facility.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 29, 2020
@joshtriplett
Copy link
Member

As discussed on zulip, this seems like a reasonable approach to me.

Looking at the implementation, one potential issue: is it possible to be on an MSVC target, but set a clang or other linker? That might cause an issue with the use of link-arg directly.

This allows developers to write NatVis descriptions for their crates.
If a Cargo package contains NatVis files, then they will be written
into the PDB file. Debuggers that support NatVis will automatically
locate these files and use them. Currently, Visual Studio 2019,
VS Code, and WinDbg support NatVis. (NatVis is a Windows-only feature.)

The easiest way to add a NatVis file to a Cargo package is to create a
`natvis` subdirectory and place the file in that directory. The file
must have a `.natvis` extension. Any (reasonable) number of NatVis
files can be provided.

Optionally, you can give explicit paths by setting the `natvis-files`
key in `Cargo.toml`. For example:

```
[package]
name = ...
natvis-files = ["my_types.natvis", "more_types.natvis"]
```

This key can also be set to `[]` in order to prevent Cargo from
searching the `natvis` directory.
@sivadeilra
Copy link
Author

Fixed break in unit tests, caused by not handling -gnu target correctly.

Looking at the implementation, one potential issue: is it possible to be on an MSVC target, but set a clang or other linker? That might cause an issue with the use of link-arg directly.

That seems unlikely to me, but it's always possible that someone is doing something exotic. It seems like the final ident in tuples is sort of used as a target, and sort of used as a toolset identifier. That is, there's no difference in the actual target platform for x86_64-pc-windows-msvc and x86_64-pc-windows-gnu, only the tools used to build the thing.

Without evidence, my guess is that the checks for -msvc are going to be good enough to protect us from a lot of strange scenarios.

We could check for a CARGO_DISABLE_NATVIS environment variable, just to provide an escape hatch in case someone did use -msvc with a different linker.

@sivadeilra
Copy link
Author

I just realized a problem with this. This is only going to collect NatVis files for the top-most manifest. It's not going to walk the dependency graph and find all relevant NatVis files. So this will only work for targets that produce a DLL or EXE, not .rlib targets.

I'm going to wait for the CI jobs to finish, and then convert this to a draft PR.

@sivadeilra sivadeilra marked this pull request as draft December 30, 2020 15:33
@nagisa
Copy link
Member

nagisa commented Jan 4, 2021

is it possible to be on an MSVC target, but set a clang or other linker?

Possible with -Clinker-flavour. It would probably make sense to more directly support embedding natvis files in rustc itself. Then cargo could use that functionality without having to worry about weird things.

@alexcrichton
Copy link
Member

I'm not super familiar with natvis myself, but I'm not sure that what's currently implemented here would be sufficient if it works the way I'm thinking. Do we want to pass /nativs:... arguments for all natvis files for all transitive dependencies including the standard library? Currently this only passes it for the local package itself. I'd want to confirm though because I'm not entirely sure how natvis works.

We don't currently have a ton of support for target-specific features like this in Cargo at this time unfortunately. I do agree, though, that it would be nice for Cargo to automatically handle this. Ideally it would also handle it for other platforms if possible to, although I'm not sure if there's an equivalent to this elsewhere.

@sivadeilra
Copy link
Author

@alexcrichton writes:

Do we want to pass /natvis:... arguments for all natvis files for all transitive dependencies including the standard library?

I'm working on implementing that now. I think it's a reasonable approach.

At the same time, I wonder if this feature might be better supported by implementing it directly in rustc, for several reasons:

  • If we do it in rustc, then it "just works" even for environments that are executing rustc directly, rather than using cargo.
  • I'm working on auto-generating NatVis <Type> visualizers for enum types, since their representation depends on codegen (niches, null-pointer optimization, etc.). That means rustc is already dealing with NatVis, and so it might make sense to unify this in rustc, rather than doing it in two places.

Another hybrid approach would be to pass NatVis filenames to rustc, and then it can decide what to do with them -- package them inside .rlib files, pass them to linker invocations, etc.

I'm open to ideas, here. My main priority is just "Make debugging Rust on Windows be a great experience".

@alexcrichton
Copy link
Member

Passing natvis files to rustc directly may end up being the best option here. As you mention then rustc could handle passing all the files around, and it would relieve Cargo from having to understand what linker rustc is invoking (and rustc would figure out how to pass the natvis files into the linker). That'd be a new command line option for rustc, though, and you're right that it may involve storing the files into rlibs or similar.

@bors
Copy link
Contributor

bors commented Mar 5, 2021

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

@joshtriplett
Copy link
Member

It sounds like the right answer for this is to first implement native natvis support in rustc, and then plumb that rustc support through Cargo. Given that, I'm going to close this for now. When there's native support in rustc, we'd love to see that support plumbed through Cargo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants