Closed Bug 1408502 Opened 2 years ago Closed 2 years ago

Embed natvis info for Gecko types in our PDB files

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: ted, Assigned: ted)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

MSVC supports XML natvis files for pretty-printing types in the debugger:
https://docs.microsoft.com/en-us/visualstudio/debugger/create-custom-views-of-native-objects

vlad wrote one for some Gecko types a while ago:
https://github.com/mozilla/moz-dev-contrib/blob/master/windows/Gecko.natvis

MSVC can load these natvis files from a PDB file as well, and link.exe has a /NATVIS option to embed them:
https://docs.microsoft.com/en-us/cpp/build/reference/natvis-add-natvis-to-pdb

If we put that Gecko.natvis file in-tree (and presumably update it to match reality) and add `-NATVIS:path/to/Gecko.natvis` to our link lines, then when debugging Firefox using MSVC those pretty-printers will be used automatically, which sounds pretty great.

I learned about this because Rust added natvis files for its stdlib types, and will pass /NATVIS to embed them in PDB files in Rust 1.21:
https://github.com/rust-lang/rust/pull/43221

That doesn't help us because we don't use rustc for linking, but all it does under the hood is pass /NATVIS:path for each .natvis file in `$rust_sysroot/lib/rustlib/etc`, so we could easily add that to our link line as well and get pretty printing for Rust types in MSVC.
Cool!

I don't see /NATVIS the VS2015 documentation, but the switch does appear to be supported there too.
Nifty!
Product: Core → Firefox Build System
I was trying this today but Vlad's natvis file seems to largely still 'just work' on the important types. Particularly on nsTArray, and it makes the situation a lot better. This seems like fairly little work for a significant benefit. I can try to make this work although I don't know our build system well and it would probably take me a little more time than some others.
Flags: needinfo?(ted)
Given that this should be easy to do.
Assignee: nobody → ted
Flags: needinfo?(ted)
Hah, I threw nearly the same change into a try push I was doing for a different bug.

Would there be value in having this available in all links? (I'm thinking mozglue and firefox mostly) In any case libxul is the most important for sure.

Don't worry about excluding lld-link, it ignores the flag without an error: https://github.com/llvm-mirror/lld/blob/6c9fbd5a4d42e8d2d67d1c78edab20f0de2e1581/COFF/Driver.cpp#L303 and if/when the support comes it'll just start working.
(In reply to David Major [:dmajor] from comment #6)
> Hah, I threw nearly the same change into a try push I was doing for a
> different bug.

Hah!

> Would there be value in having this available in all links? (I'm thinking
> mozglue and firefox mostly) In any case libxul is the most important for
> sure.

It probably wouldn't hurt, but writing that patch seemed like more work and this one was easy. :) I would vote for getting the simple thing in and we can always improve on that.

> Don't worry about excluding lld-link, it ignores the flag without an error:
> https://github.com/llvm-mirror/lld/blob/
> 6c9fbd5a4d42e8d2d67d1c78edab20f0de2e1581/COFF/Driver.cpp#L303 and if/when
> the support comes it'll just start working.

Oh, great!
I was too lazy to actually test the results of my previous try push and apparently so was everyone else, but since I have working patches for bug 1437577 now I've rebased on top of those and pushed to try again, so it should be easier to test now!
Comment on attachment 8973966 [details]
bug 1408502 - embed natvis info for Gecko types in our PDB files.

https://reviewboard.mozilla.org/r/242306/#review248266

::: toolkit/library/moz.build:80
(Diff revision 1)
>      # Rust 1.12 has been released.
>      # We're also linking against libresolv to solve bug 1367932.
>      if CONFIG['OS_ARCH'] == 'Darwin':
>          LDFLAGS += ['-Wl,-no_compact_unwind,-lresolv']
>  
> +    # This actually wants to check that the linker is link.exe, but we don't have a

If this comment is about lld-link, the support exists: https://github.com/llvm-mirror/lld/commit/b6706ca4f487b4ab8e23694a302bc74c50d52cbe
Attachment #8973966 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8973966 [details]
bug 1408502 - embed natvis info for Gecko types in our PDB files.

https://reviewboard.mozilla.org/r/242308/#review248280

Please file a followup for supporting `mozilla::Vector`, and ambitiously, `PLDHashTable`.

I trust what dmajor says about lld supporting `-NATVIS`.

::: toolkit/library/gecko.natvis:38
(Diff revision 1)
> +  <Type Name="mozilla::ThreadSafeAutoRefCnt">
> +    <DisplayString>{mValue.mValue._My_val}</DisplayString>
> +  </Type>
> +
> +  <!-- smart pointer/refcount pointer things -->
> +  <Type Name="nsRefPtr&lt;*&gt;">

Presumably this should be `RefPtr` now (possibly `mozilla::`-qualified)?

::: toolkit/library/gecko.natvis:51
(Diff revision 1)
> +  <Type Name="nsACString">
> +    <AlternativeType Name="nsACString_internal" />
> +    <AlternativeType Name="nsCString" />
> +    <AlternativeType Name="nsLiteralCString" />

It would be nice if we supported `nsAutoCString` here, but that can be a followup, I suppose?

::: toolkit/library/gecko.natvis:64
(Diff revision 1)
> +  <Type Name="nsAString">
> +    <AlternativeType Name="nsAString_internal" />
> +    <AlternativeType Name="nsString" />
> +    <AlternativeType Name="nsLiteralString" />

Same thing for auto strings here.
Attachment #8973966 - Flags: review?(nfroyd) → review+
Blocks: 1459989
Blocks: 1459991
https://hg.mozilla.org/mozilla-central/rev/856384fbc255
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.