This is an archive of the discontinued LLVM Phabricator instance.

[pdb] Add support for writing Module Info and module symbols
ClosedPublic

Authored by zturner on Mar 14 2017, 3:50 PM.

Details

Summary
Previously we did not have support for writing detailed
module information for each module, as well as the symbol
records.  This patch adds support for this, and in doing
so enables the ability to construct minimal PDBs from
just a few lines of YAML.  A test is added to illustrate
this functionality.

Diff Detail

Event Timeline

zturner created this revision.Mar 14 2017, 3:50 PM

Thanks for making it easy to write minimal PDBs! I added a couple of inline comments about the code.

llvm/lib/DebugInfo/PDB/Native/ModInfoBuilder.cpp
56

Having this as mutable state increases the surface where bugs can develop. Can we handle this some other way? For example, can we set it in the constructor?

79

What does the comment here mean? What does this line do?

zturner added inline comments.Mar 15 2017, 11:49 AM
llvm/lib/DebugInfo/PDB/Native/ModInfoBuilder.cpp
56

That's the pattern I've been using for all of the other Builder classes. All of the fields can be set over and over until you call finalize / commit. I could perhaps change it, but then that begs the question of what to do about all the other instances where i use this same pattern. It would be a bit awkward if you had to specify every single value to the constructor, but at the same time there's nothing particularly special about this field over the various other fields.

79

I was trying to make it clear that I wasn't forgetting to set any of the fields in this class, but that some of them should already have been set in other places.

inglorion accepted this revision.Mar 15 2017, 12:54 PM

Thanks for the clarifications! lgtm

This revision is now accepted and ready to land.Mar 15 2017, 12:54 PM
ruiu accepted this revision.Mar 16 2017, 4:03 PM

LGTM

llvm/lib/DebugInfo/PDB/Native/ModInfoBuilder.cpp
79

But it's a bit puzzling. At first, I thought that Layout.Mod was a volatile variable and reading from the variable is needed here for some reason. Maybe just leaving a comment is better.

llvm/tools/llvm-pdbdump/llvm-pdbdump.cpp
374

It may be nice to leave a comment about 4096. It's just a reasonable default block size for a PDB file and used if a YAML file doesn't specify any block size, right?

zturner added inline comments.Mar 16 2017, 4:30 PM
llvm/tools/llvm-pdbdump/llvm-pdbdump.cpp
374

Yes. It's also what you get in almost every Microsoft generated PDB that I've seen.

thakis closed this revision.Mar 18 2017, 9:06 AM
thakis added a subscriber: thakis.

I think this landed in r297900.