Closed
Bug 1408502
Opened 7 years ago
Closed 6 years ago
Embed natvis info for Gecko types in our PDB files
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
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.
Comment 2•7 years ago
|
||
Nifty!
Updated•7 years ago
|
Product: Core → Firefox Build System
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
Given that this should be easy to do.
Assignee: nobody → ted
Flags: needinfo?(ted)
Assignee | ||
Comment 5•7 years ago
|
||
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.
Assignee | ||
Comment 7•7 years ago
|
||
(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!
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 11•6 years ago
|
||
mozreview-review |
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
Updated•6 years ago
|
Attachment #8973966 -
Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment 12•6 years ago
|
||
mozreview-review |
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<*>">
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+
Assignee | ||
Comment 13•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/856384fbc255ed1cb6dd35812e9094d8dac669b2
bug 1408502 - embed natvis info for Gecko types in our PDB files. r=froydnj
Comment 14•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•