Introduce symbol cache to PDB NativeSession
AcceptedPublic

Authored by amccarth on Jun 22 2017, 4:37 PM.

Details

Reviewers
zturner
Summary

Instead of creating symbols directly in the findChildren methods of the native symbol implementations, they will rely on the NativeSession to act as a factory for these types. This lets NativeSession cache the NativeRawSymbols in its new symbol cache and makes that cache the source of unique IDs for the symbols.

Right now, this affects only NativeCompilandSymbols. There's no external change yet, so I think the existing tests are still sufficient. Coming soon are patches to extend this to built-in types and enums.

Diff Detail

amccarth created this revision.Jun 22 2017, 4:37 PM
zturner added inline comments.Jun 27 2017, 1:29 PM
llvm/include/llvm/DebugInfo/PDB/Native/NativeSession.h
35

Should this return a PDBSymbolCompiland?

llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp
75–76

push_back instead of emplace_back (same as below)

77–78

make_unique again.

88

make_unique? Also, since you're passing the value type directly, you should use push_back instead of emplace_back

amccarth marked 3 inline comments as done.Jun 27 2017, 2:39 PM
amccarth added inline comments.
llvm/include/llvm/DebugInfo/PDB/Native/NativeSession.h
35

That's a fair question. This factory method (and the others that are in the pipeline) are typically invoked in implementations of IPDBSymbol::getChildAtIndex, which return std::unique_ptr<PDBSymbol>, so I was going with the base type, but I suppose there's no harm in returning the derived type and letting the implicit conversion happen.

llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp
77–78

I need to construct an instance of the derived type but return it as a unique_ptr to the base type. I've had trouble getting VS to accept that with std::make_unique, which is why I was using new directly. But it seems to work with llvm::make_unique, so I guess that'll do.

amccarth updated this revision to Diff 104278.Jun 27 2017, 2:41 PM
amccarth marked an inline comment as done.

Addresses feedback from zturner.

zturner accepted this revision.Jun 27 2017, 3:03 PM
zturner added inline comments.
llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp
12–14

I think these are supposed to go below the llvm includes based on the style guide.

This revision is now accepted and ready to land.Jun 27 2017, 3:03 PM
amccarth marked an inline comment as done.Jun 27 2017, 3:15 PM
amccarth added inline comments.
llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp
12–14

You're right, of course. I spent too long in Google style to keep them straight now.

amccarth updated this revision to Diff 104285.Jun 27 2017, 3:30 PM
amccarth marked an inline comment as done.

Fixed order of includes to comply with LLVM style