[PATCH] D35738: Enable llvm-pdbutil to list enumerations using native PDB reader

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

In https://reviews.llvm.org/D35738#817991, @zturner wrote:

> Is it that much extra work to implement children?  It would be nice if this could just be feature complete.  It seems like all you have to do is read the FieldList member of the CodeView record, load that item, and then create an enumerator that iterates each item and returns a NativeConstantSymbol or whatever we're calling it


I think I see how to do this, but I'd feel better doing it as a subsequent patch.  I'd need to make the NativeSession be the owner of the Tpi stream.



================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/NativeEnumTypes.h:22
+
+class NativeEnumTypes : public IPDBEnumChildren<PDBSymbol> {
+public:
----------------
rnk wrote:
> I'm wondering if we can get away with implementing these findChildren methods by building a vector of PDBSymbols up front, and then this class becomes a simple adapter from vector to IPDBEnumChildren. It seems to me if the caller calls findChildren, they're going to walk them all, so we can do O(n) work without slowing things down asymptotically.
Am I missing something?  I think this is still O(n), as calls to getNext continue through the types to the next one that matches.  It may be a small speedup in cases where the caller is planning to iterate over the entire range.  But it's overkill if the caller isn't going to iterate the entire list.  And I don't think it changes the complexity.

And a vector would require more memory, which may or may not be a big deal.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeExeSymbol.cpp:48-49
+      const auto &Types = Tpi->types(&HadError);
+      if (HadError)
+        return nullptr;
+      return std::unique_ptr<IPDBEnumSymbols>(
----------------
rnk wrote:
> This check will never fail. The optional HadError parameter is essentially an awkward way to thread error handling out of a range-based-for. The idea is that if the iterator encounters an error, it halts iteration and sets *HadError to true. The caller can check for failure outside the loop. We don't want to eagerly check for errors because that would require iterating over all the data.
> 
> Does this method get called often or early? I'm wondering if it would be better to iterate and filter the type stream up front and make a vector.
OK, I get what you've said about the error, and I can remove the unnecessary check.

In a dumper, this method is called once for the enums, so, no, it's not called frequently.  In a random-access scenario, I'd expect we'd be following TypeIndexes rather than asking the Exe scope for all of the enums it contains.


https://reviews.llvm.org/D35738