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

Embed MSVC .natvis files into .pdbs and mangle debuginfo for &str, *T, and [T]. #43221

Merged
merged 5 commits into from Jul 28, 2017

Conversation

Projects
None yet
@MaulingMonkey
Contributor

MaulingMonkey commented Jul 13, 2017

No idea if these changes are reasonable - please feel free to suggest changes/rewrites. And these are some of my first real commits to any rust codebase - don't be gentle, and nitpick away, I need to learn! ;)

Overview

Embedding .natvis files into .pdbs allows MSVC (and potentially other debuggers) to automatically pick up the visualizers without having to do any additional configuration (other than to perhaps add the relevant .pdb paths to symbol search paths.)

The native debug engine for MSVC parses the type names, making various C++ish assumptions about what they mean and adding various limitations to valid type names. &str cannot be matched against a visualizer, but if we emit str& instead, it'll be recognized as a reference to a str, solving the problem. [T] is similarly problematic, but emitting slice<T> instead works fine as it looks like a template. I've been unable to get e.g. slice<u32>& to match visualizers in VS2015u3, so I've gone with str* and slice<u32>* instead.

Possible Issues

  • I'm not sure if slice<T> is a great mangling for [T] or if I should worry about name collisions.
  • I'm not sure if linker.rs is the right place to be enumerating natvis files.
  • I'm not sure if these type name mangling changes should actually be MSVC specific. I recall seeing gdb visualizer tests that might be broken if made more general? I'm hesitant to mess with them without a gdb install. But perhaps I'm just wracking up technical debt.
    Should I try pacman -S mingw-w64-x86_64-gdb and to make things consistent?
  • I haven't touched const / mut yet, and I'm worried MSVC might trip up on mut or their placement.
  • I may like terse oneliners too much.
  • I don't know if there's broader implications for messing with debug type names here.
  • I may have been mistaken about bellow test failures being ignorable / unrelated to this changelist.

Test Failures on x86_64-pc-windows-gnu

---- [debuginfo-gdb] debuginfo-gdb\associated-types.rs stdout ----
        thread '[debuginfo-gdb] debuginfo-gdb\associated-types.rs' panicked at 'gdb not available but debuginfo gdb debuginfo test requested', src\tools\compiletest\src\runtest.rs:48:16
note: Run with `RUST_BACKTRACE=1` for a backtrace.

[...identical panic causes omitted...]

---- [debuginfo-gdb] debuginfo-gdb\vec.rs stdout ----
        thread '[debuginfo-gdb] debuginfo-gdb\vec.rs' panicked at 'gdb not available but debuginfo gdb debuginfo test requested', src\tools\compiletest\src\runtest.rs:48:16

Relevant Issues

Pretty Pictures

Collapsed Watch Window
Expanded Watch Window

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jul 13, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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.

Collaborator

rust-highfive commented Jul 13, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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.

@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster

shepmaster Jul 14, 2017

Member

Thanks so much for the PR @MaulingMonkey! I can't speak to the content, but I must say that that is a top-notch commit message! We'll get someone knowledgable to review this shortly!

Member

shepmaster commented Jul 14, 2017

Thanks so much for the PR @MaulingMonkey! I can't speak to the content, but I must say that that is a top-notch commit message! We'll get someone knowledgable to review this shortly!

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum Jul 14, 2017

Member

r? @brson I think

Member

Mark-Simulacrum commented Jul 14, 2017

r? @brson I think

@Mark-Simulacrum Mark-Simulacrum assigned brson and unassigned eddyb Jul 14, 2017

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1
Contributor

arielb1 commented Jul 18, 2017

@arielb1 arielb1 assigned michaelwoerister and unassigned brson Jul 18, 2017

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Jul 19, 2017

Contributor

Thanks a lot for the PR, @MaulingMonkey! This looks like a great contribution!

  • I'm not sure if slice is a great mangling for [T] or if I should worry about name collisions.

slice<T> does seem a little problematic but it should actually be fine. User-defined types normally start with an uppercase letter so we can leave it for now, I think.

  • I'm not sure if linker.rs is the right place to be enumerating natvis files.

At a first glance, this looks like the right place.

  • I'm not sure if these type name mangling changes should actually be MSVC specific. I recall seeing gdb visualizer tests that might be broken if made more general? I'm hesitant to mess with them without a gdb install. But perhaps I'm just wracking up technical debt.

Staying MSVC specific for this is the way to go, I'd say.

  • I haven't touched const / mut yet, and I'm worried MSVC might trip up on mut or their placement.

You can worry about that in another PR :)

  • I may like terse oneliners too much.

Yeah, they are in conflict with rustc's current code style guide.

  • I don't know if there's broader implications for messing with debug type names here.

As long as this is limited to MSVC and that still works as good or better than before, this looks good to me.

  • I may have been mistaken about bellow test failures being ignorable / unrelated to this changelist.

There are no tests for MSVC debugging yet. There might be a handful of other tests that depend debuginfo type names, but CI will tell us about those soon enough.

Now for a question of mine: Is there a way to transform the mangled name back to something more Rust-like when they are displayed?

Contributor

michaelwoerister commented Jul 19, 2017

Thanks a lot for the PR, @MaulingMonkey! This looks like a great contribution!

  • I'm not sure if slice is a great mangling for [T] or if I should worry about name collisions.

slice<T> does seem a little problematic but it should actually be fine. User-defined types normally start with an uppercase letter so we can leave it for now, I think.

  • I'm not sure if linker.rs is the right place to be enumerating natvis files.

At a first glance, this looks like the right place.

  • I'm not sure if these type name mangling changes should actually be MSVC specific. I recall seeing gdb visualizer tests that might be broken if made more general? I'm hesitant to mess with them without a gdb install. But perhaps I'm just wracking up technical debt.

Staying MSVC specific for this is the way to go, I'd say.

  • I haven't touched const / mut yet, and I'm worried MSVC might trip up on mut or their placement.

You can worry about that in another PR :)

  • I may like terse oneliners too much.

Yeah, they are in conflict with rustc's current code style guide.

  • I don't know if there's broader implications for messing with debug type names here.

As long as this is limited to MSVC and that still works as good or better than before, this looks good to me.

  • I may have been mistaken about bellow test failures being ignorable / unrelated to this changelist.

There are no tests for MSVC debugging yet. There might be a handful of other tests that depend debuginfo type names, but CI will tell us about those soon enough.

Now for a question of mine: Is there a way to transform the mangled name back to something more Rust-like when they are displayed?

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Jul 19, 2017

Contributor

Thanks for the kind words all!

  • I may like terse oneliners too much.

Yeah, they are in conflict with rustc's current code style guide.

Okay, I've got a followup changelist fixing this, that I've got the build and test scripts chewing through. Any other style issues while I'm in there?

I'm also inclined to rename, comment on, and move this to the top of push_debuginfo_type_name, to better describe "what" and "why":

    // When targeting MSVC, emit C++ style type names for compatability with
    // .natvis visualizers (and perhaps other existing native debuggers?)
    let cpp_like_names = cx.sess().target.target.options.is_like_msvc;

As long as this is limited to MSVC and that still works as good or better than before, this looks good to me.

The only regression I can think of is if someone built with MSVC but then wanted to debug with GDB instead for some reason (debugging binaries that someone else built?)

Now for a question of mine: Is there a way to transform the mangled name back to something more Rust-like when they are displayed?

Not that I'm aware of, short of writing a full blown debug engine (e.g. maybe on VisualRust's far future roadmap?) Such a .natvis feature would be useful to unexpand C++ templates too, if it were an option (e.g. std::basic_string<char, ...> -> std::string)

Contributor

MaulingMonkey commented Jul 19, 2017

Thanks for the kind words all!

  • I may like terse oneliners too much.

Yeah, they are in conflict with rustc's current code style guide.

Okay, I've got a followup changelist fixing this, that I've got the build and test scripts chewing through. Any other style issues while I'm in there?

I'm also inclined to rename, comment on, and move this to the top of push_debuginfo_type_name, to better describe "what" and "why":

    // When targeting MSVC, emit C++ style type names for compatability with
    // .natvis visualizers (and perhaps other existing native debuggers?)
    let cpp_like_names = cx.sess().target.target.options.is_like_msvc;

As long as this is limited to MSVC and that still works as good or better than before, this looks good to me.

The only regression I can think of is if someone built with MSVC but then wanted to debug with GDB instead for some reason (debugging binaries that someone else built?)

Now for a question of mine: Is there a way to transform the mangled name back to something more Rust-like when they are displayed?

Not that I'm aware of, short of writing a full blown debug engine (e.g. maybe on VisualRust's far future roadmap?) Such a .natvis feature would be useful to unexpand C++ templates too, if it were an option (e.g. std::basic_string<char, ...> -> std::string)

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Jul 19, 2017

Member

The only regression I can think of is if someone built with MSVC but then wanted to debug with GDB instead for some reason

This is already incredibly unsupported and doesn't work due to vastly different debuginfo formats. I wouldn't be worried about this.

Member

retep998 commented Jul 19, 2017

The only regression I can think of is if someone built with MSVC but then wanted to debug with GDB instead for some reason

This is already incredibly unsupported and doesn't work due to vastly different debuginfo formats. I wouldn't be worried about this.

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Jul 20, 2017

Contributor

Any other style issues while I'm in there?

Didn't see any.

If you support non-unicode paths too (see comment) then I'd be happy to r+ this.

Contributor

michaelwoerister commented Jul 20, 2017

Any other style issues while I'm in there?

Didn't see any.

If you support non-unicode paths too (see comment) then I'd be happy to r+ this.

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Jul 21, 2017

Contributor

If you support non-unicode paths too (see comment) then I'd be happy to r+ this.

Good catch - using traits for "overloading" was new to me, so I'd mistakenly assumed Command::arg() took &str. Latest changelist fixes this.

Contributor

MaulingMonkey commented Jul 21, 2017

If you support non-unicode paths too (see comment) then I'd be happy to r+ this.

Good catch - using traits for "overloading" was new to me, so I'd mistakenly assumed Command::arg() took &str. Latest changelist fixes this.

@Boddlnagg

This comment has been minimized.

Show comment
Hide comment
@Boddlnagg

Boddlnagg Jul 21, 2017

Contributor

@MaulingMonkey: Have you tested the string visualizers with non-ASCII strings? Since Rust strings are defined to be UTF-8, we should use the s8 modifier in the natvis file, as in:

<Type Name="str">
    <DisplayString>{data_ptr,[length]s8}</DisplayString>
    <StringView>data_ptr,[length]s8</StringView>
    ...

I found out about this some time ago, but the official documentation is sparse unfortunately. It's listed on https://msdn.microsoft.com/en-us/library/75w45ekt.aspx, but it doesn't say that it means UTF-8. It does say, however, that s32 means UTF-32.
NatVis seems to be not very well documented in general, the \NATVIS linker switch is also basically undocumented ...

Contributor

Boddlnagg commented Jul 21, 2017

@MaulingMonkey: Have you tested the string visualizers with non-ASCII strings? Since Rust strings are defined to be UTF-8, we should use the s8 modifier in the natvis file, as in:

<Type Name="str">
    <DisplayString>{data_ptr,[length]s8}</DisplayString>
    <StringView>data_ptr,[length]s8</StringView>
    ...

I found out about this some time ago, but the official documentation is sparse unfortunately. It's listed on https://msdn.microsoft.com/en-us/library/75w45ekt.aspx, but it doesn't say that it means UTF-8. It does say, however, that s32 means UTF-32.
NatVis seems to be not very well documented in general, the \NATVIS linker switch is also basically undocumented ...

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Jul 21, 2017

Contributor

@MaulingMonkey: Have you tested the string visualizers with non-ASCII strings?

I have now. With your suggested changes, I grabbed https://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-demo.txt . Breakpointing slice2 of:

use std::io::BufReader;
use std::io::BufRead;
use std::fs::File;
use std::path::Path;

fn main() {
    let f = File::open("I:\\home\\projects\\test\\rust\\UTF-8-demo.txt").unwrap();
    let mut file = BufReader::new(&f);
    for line in file.lines() {
        let line = line.unwrap();
        let slice : &str = &line;
        let slice2 = slice;

        println!("{}", slice);
    }
}

And stepping to this line:

  ∀x∈ℝ: ⌈x⌉ = −⌊−x⌋, α ∧ ¬β = ¬(¬α ∨ β),    ⎪⎢⎜│───── ⎟⎥⎪

Shows me the following with the s8 postfix, which looks accurate. The enums could perhaps use some improvement, and there's a few placeholder boxes for characters my visualizer font is missing, but it's much better than what was spewed out without the s8 postfixes for these lines.

Screenshot with s8 postfixed string visualizers

The combining mark for the second T in " STARGΛ̊TE" is also misplaced to the left of the T in the Value column, but not in the full blown Text Visualizer. Then again, Chrome also shows it misplaced to the left of the T in this Chrome tab, but not in the original text file location, so this might just be more font issues?

I can roll those changes into this pull request.

Contributor

MaulingMonkey commented Jul 21, 2017

@MaulingMonkey: Have you tested the string visualizers with non-ASCII strings?

I have now. With your suggested changes, I grabbed https://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-demo.txt . Breakpointing slice2 of:

use std::io::BufReader;
use std::io::BufRead;
use std::fs::File;
use std::path::Path;

fn main() {
    let f = File::open("I:\\home\\projects\\test\\rust\\UTF-8-demo.txt").unwrap();
    let mut file = BufReader::new(&f);
    for line in file.lines() {
        let line = line.unwrap();
        let slice : &str = &line;
        let slice2 = slice;

        println!("{}", slice);
    }
}

And stepping to this line:

  ∀x∈ℝ: ⌈x⌉ = −⌊−x⌋, α ∧ ¬β = ¬(¬α ∨ β),    ⎪⎢⎜│───── ⎟⎥⎪

Shows me the following with the s8 postfix, which looks accurate. The enums could perhaps use some improvement, and there's a few placeholder boxes for characters my visualizer font is missing, but it's much better than what was spewed out without the s8 postfixes for these lines.

Screenshot with s8 postfixed string visualizers

The combining mark for the second T in " STARGΛ̊TE" is also misplaced to the left of the T in the Value column, but not in the full blown Text Visualizer. Then again, Chrome also shows it misplaced to the left of the T in this Chrome tab, but not in the original text file location, so this might just be more font issues?

I can roll those changes into this pull request.

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Jul 21, 2017

Contributor

One caveat is that the expanded array representation is still individual ASCII-ish characters - ArrayItems/ValuePointer will just plain ignore the postfix if added - but that might be a feature anyways.

Contributor

MaulingMonkey commented Jul 21, 2017

One caveat is that the expanded array representation is still individual ASCII-ish characters - ArrayItems/ValuePointer will just plain ignore the postfix if added - but that might be a feature anyways.

@Boddlnagg

This comment has been minimized.

Show comment
Hide comment
@Boddlnagg

Boddlnagg Jul 25, 2017

Contributor

Just as an addendum, I found the link where s8 is documented a little bit more: https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/dx--display-visualizer-variables-

Contributor

Boddlnagg commented Jul 25, 2017

Just as an addendum, I found the link where s8 is documented a little bit more: https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/dx--display-visualizer-variables-

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 27, 2017

Member

ping @michaelwoerister for a follow-up review, this looks ready to go!

Member

alexcrichton commented Jul 27, 2017

ping @michaelwoerister for a follow-up review, this looks ready to go!

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister
Contributor

michaelwoerister commented Jul 28, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 28, 2017

Contributor

📌 Commit 90a7cac has been approved by michaelwoerister

Contributor

bors commented Jul 28, 2017

📌 Commit 90a7cac has been approved by michaelwoerister

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 28, 2017

Contributor

⌛️ Testing commit 90a7cac with merge 6f815ca...

Contributor

bors commented Jul 28, 2017

⌛️ Testing commit 90a7cac with merge 6f815ca...

bors added a commit that referenced this pull request Jul 28, 2017

Auto merge of #43221 - MaulingMonkey:natvis-improvements, r=michaelwo…
…erister

Embed MSVC .natvis files into .pdbs and mangle debuginfo for &str, *T, and [T].

No idea if these changes are reasonable - please feel free to suggest changes/rewrites.  And these are some of my first real commits to any rust codebase - *don't* be gentle, and nitpick away, I need to learn! ;)

### Overview
Embedding `.natvis` files into `.pdb`s allows MSVC (and potentially other debuggers) to automatically pick up the visualizers without having to do any additional configuration (other than to perhaps add the relevant .pdb paths to symbol search paths.)

The native debug engine for MSVC parses the type names, making various C++ish assumptions about what they mean and adding various limitations to valid type names.  `&str` cannot be matched against a visualizer, but if we emit `str&` instead, it'll be recognized as a reference to a `str`, solving the problem.  `[T]` is similarly problematic, but emitting `slice<T>` instead works fine as it looks like a template.  I've been unable to get e.g. `slice<u32>&` to match visualizers in VS2015u3, so I've gone with `str*` and `slice<u32>*` instead.

### Possible Issues
* I'm not sure if `slice<T>` is a great mangling for `[T]` or if I should worry about name collisions.
* I'm not sure if `linker.rs` is the right place to be enumerating natvis files.
* I'm not sure if these type name mangling changes should actually be MSVC specific.  I recall seeing gdb visualizer tests that might be broken if made more general?  I'm hesitant to mess with them without a gdb install.  But perhaps I'm just wracking up technical debt.
  Should I try `pacman -S mingw-w64-x86_64-gdb` and to make things consistent?
* I haven't touched `const` / `mut` yet, and I'm worried MSVC might trip up on `mut` or their placement.
* I may like terse oneliners too much.
* I don't know if there's broader implications for messing with debug type names here.
* I may have been mistaken about bellow test failures being ignorable / unrelated to this changelist.

### Test Failures on `x86_64-pc-windows-gnu`

```
---- [debuginfo-gdb] debuginfo-gdb\associated-types.rs stdout ----
        thread '[debuginfo-gdb] debuginfo-gdb\associated-types.rs' panicked at 'gdb not available but debuginfo gdb debuginfo test requested', src\tools\compiletest\src\runtest.rs:48:16
note: Run with `RUST_BACKTRACE=1` for a backtrace.

[...identical panic causes omitted...]

---- [debuginfo-gdb] debuginfo-gdb\vec.rs stdout ----
        thread '[debuginfo-gdb] debuginfo-gdb\vec.rs' panicked at 'gdb not available but debuginfo gdb debuginfo test requested', src\tools\compiletest\src\runtest.rs:48:16
```

### Relevant Issues
* #40460 Metaissue for Visual Studio debugging Rust
* #36503 Investigate natvis for improved msvc debugging
* PistonDevelopers/VisualRust#160 Debug visualization of Rust data structures

### Pretty Pictures
![Collapsed Watch Window](https://user-images.githubusercontent.com/75894/28180998-e44c7516-67bb-11e7-8b48-d4f9605973ae.png)
![Expanded Watch Window](https://user-images.githubusercontent.com/75894/28181000-e8da252e-67bb-11e7-96b8-d613310c04dc.png)
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 28, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 6f815ca to master...

Contributor

bors commented Jul 28, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 6f815ca to master...

@bors bors merged commit 90a7cac into rust-lang:master Jul 28, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
homu Test successful
Details
@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Jul 31, 2017

Contributor

Thanks again, @MaulingMonkey, for kicking this off!

Contributor

michaelwoerister commented Jul 31, 2017

Thanks again, @MaulingMonkey, for kicking this off!

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Jul 31, 2017

Contributor

My pleasure. Thanks for the review, @michaelwoerister ! And thanks @Boddlnagg (and more!) for cluing me into the /NATVIS flag and type name issues. Your research turned this into low hanging fruit, convincing me I could help 😃

Contributor

MaulingMonkey commented Jul 31, 2017

My pleasure. Thanks for the review, @michaelwoerister ! And thanks @Boddlnagg (and more!) for cluing me into the /NATVIS flag and type name issues. Your research turned this into low hanging fruit, convincing me I could help 😃

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Sep 9, 2017

Member

For the record, this breaks people trying to use LLD with msvc because LLD doesn't yet understand /NATVIS.

Member

retep998 commented Sep 9, 2017

For the record, this breaks people trying to use LLD with msvc because LLD doesn't yet understand /NATVIS.

@Boddlnagg

This comment has been minimized.

Show comment
Hide comment
@Boddlnagg

Boddlnagg Sep 9, 2017

Contributor

Does rustc know that LLD is being used? Then we should just disable /NATVIS in that case.

Contributor

Boddlnagg commented Sep 9, 2017

Does rustc know that LLD is being used? Then we should just disable /NATVIS in that case.

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Sep 9, 2017

Member

Currently I'm just passing -Clinker="C:\Program Files\LLVM\bin\lld-link.exe" to try out LLD. I'm not sure what the "correct" way to use LLD is.

Member

retep998 commented Sep 9, 2017

Currently I'm just passing -Clinker="C:\Program Files\LLVM\bin\lld-link.exe" to try out LLD. I'm not sure what the "correct" way to use LLD is.

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Sep 10, 2017

Contributor

Possible temporary workarounds to unblock your testing:

  • Temporarily move the .natvis files to another path where rustc won't find them.
  • -Z linker-flavor=ld ?

I imagine -Clinker=... sets TargetOptions::linker, which can probably be checked as a more permanent workaround (e.g. skip natvis files if the linker contains "lld-link"?) Is LLD still missing debuginfo support per #39915 (comment) ?

Fixing this on LLD's end might be as simple as adding def natvis : QF<"natvis">; to https://github.com/llvm-mirror/lld/blob/master/COFF/Options.td#L113-L144 which might be preferred (e.g. if LLD does eventually add this feature, it'd be preferable not to have a workaround on rust's end hanging around in need of removal)

If I do take a stab at a workaround on rustc's end, would http://releases.llvm.org/5.0.0/LLVM-5.0.0-win64.exe work for testing, or have you installed LLVM via another method that I should prefer?

Contributor

MaulingMonkey commented Sep 10, 2017

Possible temporary workarounds to unblock your testing:

  • Temporarily move the .natvis files to another path where rustc won't find them.
  • -Z linker-flavor=ld ?

I imagine -Clinker=... sets TargetOptions::linker, which can probably be checked as a more permanent workaround (e.g. skip natvis files if the linker contains "lld-link"?) Is LLD still missing debuginfo support per #39915 (comment) ?

Fixing this on LLD's end might be as simple as adding def natvis : QF<"natvis">; to https://github.com/llvm-mirror/lld/blob/master/COFF/Options.td#L113-L144 which might be preferred (e.g. if LLD does eventually add this feature, it'd be preferable not to have a workaround on rust's end hanging around in need of removal)

If I do take a stab at a workaround on rustc's end, would http://releases.llvm.org/5.0.0/LLVM-5.0.0-win64.exe work for testing, or have you installed LLVM via another method that I should prefer?

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Sep 10, 2017

Contributor

A similar patch for /GUARDSYM: llvm-mirror/lld@96a6f2c .

Tempted to find someone who's already set up for committing to/testing LLVM - perhaps on one of their mailing lists if nobody here already is - a job has eaten my free time. Maybe the pcc fellow who implemented https://reviews.llvm.org/D37607 ?

Contributor

MaulingMonkey commented Sep 10, 2017

A similar patch for /GUARDSYM: llvm-mirror/lld@96a6f2c .

Tempted to find someone who's already set up for committing to/testing LLVM - perhaps on one of their mailing lists if nobody here already is - a job has eaten my free time. Maybe the pcc fellow who implemented https://reviews.llvm.org/D37607 ?

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Sep 10, 2017

Member

@MaulingMonkey

I went with the former workaround. I don't know why anyone would use the latter workaround considering how horrifying ld is.

LLD does support emitting debuginfo now (http://blog.llvm.org/2017/08/llvm-on-windows-now-supports-pdb-debug.html). It's definitely a lot closer to the point of being a legitimate alternative.

I would prefer LLD supporting the flag or at least ignoring it and not erroring. But until then it would probably be better to not pass /NATVIS to lld for the time being.

That is a valid way to acquire LLVM, although I personally use http://llvm.org/builds/ to get more bleeding edge builds.

Member

retep998 commented Sep 10, 2017

@MaulingMonkey

I went with the former workaround. I don't know why anyone would use the latter workaround considering how horrifying ld is.

LLD does support emitting debuginfo now (http://blog.llvm.org/2017/08/llvm-on-windows-now-supports-pdb-debug.html). It's definitely a lot closer to the point of being a legitimate alternative.

I would prefer LLD supporting the flag or at least ignoring it and not erroring. But until then it would probably be better to not pass /NATVIS to lld for the time being.

That is a valid way to acquire LLVM, although I personally use http://llvm.org/builds/ to get more bleeding edge builds.

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Sep 11, 2017

Contributor

I've created a potential workaround patch for rustc, although I can't yet test it as ./x.py build is generating millions of warnings (literally) building LLVM, which is taking awhile. EDIT: I've made it public here while waiting to build: MaulingMonkey@d953c5a - I'm half tempted to make an untested pull request and let the build servers sort it out, although that feels like bad juju.

I've also created a patch for lld-link which works, now I just need to hand it off for submission somehow. EDIT: I've sent this off to llvm-commits@lists.llvm.org as

[lld] Small patch to ignore "/natvis:..." flag in the COFF / lld-link.exe frontend.

although it's been held in moderation thanks to screwing up my email aliases.

Contributor

MaulingMonkey commented Sep 11, 2017

I've created a potential workaround patch for rustc, although I can't yet test it as ./x.py build is generating millions of warnings (literally) building LLVM, which is taking awhile. EDIT: I've made it public here while waiting to build: MaulingMonkey@d953c5a - I'm half tempted to make an untested pull request and let the build servers sort it out, although that feels like bad juju.

I've also created a patch for lld-link which works, now I just need to hand it off for submission somehow. EDIT: I've sent this off to llvm-commits@lists.llvm.org as

[lld] Small patch to ignore "/natvis:..." flag in the COFF / lld-link.exe frontend.

although it's been held in moderation thanks to screwing up my email aliases.

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Sep 12, 2017

Contributor

LLVM patch landed:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170911/486071.html
llvm-mirror/lld@27b9c42

Still stuck in trying to resolve build hell for the rustc patch.

Contributor

MaulingMonkey commented Sep 12, 2017

LLVM patch landed:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170911/486071.html
llvm-mirror/lld@27b9c42

Still stuck in trying to resolve build hell for the rustc patch.

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Sep 12, 2017

Contributor

Awesome, thanks @MaulingMonkey!

Contributor

michaelwoerister commented Sep 12, 2017

Awesome, thanks @MaulingMonkey!

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Sep 13, 2017

Contributor

I've updated https://github.com/MaulingMonkey/rust/tree/lld-link-natvis-regression-fix which at least builds on x86_64-pc-windows-gnu. I suspect it probably works. I still cannot make a x86_64-pc-windows-msvc build to actually test on however - the last build I canceled accumulated 1.2GB of stdout logs over 24 hours and showed no signs of being anywhere close to done. I'll avoid canceling my current build, although I don't know how long it will take. This doesn't look like it's been resolved by my fresh clone either.

I'll submit a pull request once any of the following happen:

  • My msvc build finishes, allowing me to test and verify the workaround works.
  • Someone else manages to build, test, and verify the workaround works.
  • Someone tells me testing isn't important :trollface:
Contributor

MaulingMonkey commented Sep 13, 2017

I've updated https://github.com/MaulingMonkey/rust/tree/lld-link-natvis-regression-fix which at least builds on x86_64-pc-windows-gnu. I suspect it probably works. I still cannot make a x86_64-pc-windows-msvc build to actually test on however - the last build I canceled accumulated 1.2GB of stdout logs over 24 hours and showed no signs of being anywhere close to done. I'll avoid canceling my current build, although I don't know how long it will take. This doesn't look like it's been resolved by my fresh clone either.

I'll submit a pull request once any of the following happen:

  • My msvc build finishes, allowing me to test and verify the workaround works.
  • Someone else manages to build, test, and verify the workaround works.
  • Someone tells me testing isn't important :trollface:
@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Sep 13, 2017

Member

The massive amounts of logs are due to a gcc regression which was fixed and will be landing in Rust soon #44531

Member

retep998 commented Sep 13, 2017

The massive amounts of logs are due to a gcc regression which was fixed and will be landing in Rust soon #44531

@MaulingMonkey

This comment has been minimized.

Show comment
Hide comment
@MaulingMonkey

MaulingMonkey Sep 16, 2017

Contributor

@retep998 thanks for the link - I was able to merge that for testing before it landed in trunk. Work has eaten a lot of my free time, but I've managed to cobble together a pull request that actually works when tested, not just works in my head 😄

Contributor

MaulingMonkey commented Sep 16, 2017

@retep998 thanks for the link - I was able to merge that for testing before it landed in trunk. Work has eaten a lot of my free time, but I've managed to cobble together a pull request that actually works when tested, not just works in my head 😄

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 19, 2017

Rollup merge of rust-lang#44626 - MaulingMonkey:lld-link-natvis-regre…
…ssion-fix, r=michaelwoerister

Skip passing /natvis to lld-link until supported.

### Overview

Teaching rustc about MSVC's undocumented linker flag, /NATVIS, broke rustc's compatability with LLVM's `lld-link` frontend, as it does not recognize the flag.  This pull request works around the problem by excluding `lld-link` by name.  @retep998 discovered this regression.

### Possible Issues

- Other linkers that try to be compatible with the MSVC linker flavor may also be broken and in need of workarounds.
- Warning about the workaround may be overkill for a minor reduction in debug functionality.
- Depending on how long this workaround sticks around, it may eventually be preferred to version check `lld-link` instead of assuming all versions are incompatible.

### Relevant issues
* Broke in rust-lang#43221 Embed MSVC .natvis files into .pdbs and mangle debuginfo for &str, *T, and [T].
* LLVM patched in llvm-mirror/lld@27b9c42 to ignore the flag instead of erroring.

r? @michaelwoerister
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment