This is an archive of the discontinued LLVM Phabricator instance.

Support embedding natvis files in PDBs.
ClosedPublic

Authored by zturner on Mar 9 2018, 2:06 PM.

Details

Summary

A natvis file is a debug visualizer DSL that can be embedded inside of a PDB so that the debugger can do custom formatting of types at debug time.

The way this is implemented is that the linker piggybacks off of the existing "named stream" map in the PDB file. It seems like in theory you can store any kind of source file in here, so you could even archive your entire source tree inside of the PDB file. so you could debug without source. In any case, the linker needs to create a named stream named /src/files/<some unique name>, and then for the contents of that stream, just embed the contents of the xml. <some unique name> doesn't seem to matter as long as it's unique and appears to end in .natvis, so we just use the path to the file that was typed in on the command line.

There's no way currently to write a test that verifies that this actually works and causes the debugger to use our embedded natvis files, but I did confirm that part manually.

Diff Detail

Event Timeline

zturner created this revision.Mar 9 2018, 2:06 PM
rnk accepted this revision.Mar 9 2018, 2:22 PM

lgtm, but we should try to use .s tests when we can because they are smaller and easier to maintain.

lld/test/COFF/Inputs/generic.yaml
1

I don't think we need a codeview object to test natvis. I think you can do something like the pdb-lib.s test instead and get rid of this generic.yaml input file.

This revision is now accepted and ready to land.Mar 9 2018, 2:22 PM
rnk requested changes to this revision.Mar 9 2018, 2:25 PM
rnk added inline comments.
lld/COFF/PDB.cpp
984

Hm, won't this make a UAF? The file is allocated here, we pass in a stringref, and nobody keeps the file mapping alive.

This revision now requires changes to proceed.Mar 9 2018, 2:25 PM
zturner added a subscriber: ruiu.Mar 9 2018, 2:42 PM

addNamedStream takes ownership by making a copy, although I could perhaps
make this more explicit by making the function accept a std::string

rnk added a comment.Mar 9 2018, 2:46 PM

Taking std::string by value is probably better, since it allows the caller to move the string to arrange to avoid the copy if they want to.

rnk accepted this revision.Mar 9 2018, 3:01 PM

Looks good with the std::string parameter for clarity

This revision is now accepted and ready to land.Mar 9 2018, 3:01 PM
zturner planned changes to this revision.Mar 12 2018, 1:28 PM

Turns out this is not sufficient to allow the debugger to read the embedded natvis file. We need to figure out the format of the undocumented /src/headerblock stream first.

This revision was not accepted when it landed; it landed in state Changes Planned.Mar 19 2018, 12:56 PM
This revision was automatically updated to reflect the committed changes.