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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/NativeEnumSymbol.h:39-64
+#if 0
+  PDB_BuiltinType getBuiltinType() const override;
+#endif
+  uint32_t getClassParentId() const override;
+#if 0
+  uint32_t getUnmodifiedTypeId() const override;
+  bool hasConstructor() const override;
----------------
Can we get rid of the ifdefs?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumSymbol.cpp:23-24
+      Record(codeview::TypeRecordKind::Enum) {
+  if (auto EC = visitTypeRecord(CV, *this))
+    (void)EC;
+}
----------------
How about `cantFail(visitTypeRecord(CV, *this));`


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumSymbol.cpp:61
+
+#if 0
+PDB_BuiltinType NativeEnumSymbol::getBuiltinType() const {
----------------
Remove ifdefs?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumTypes.cpp:27-29
+  // This is expensive and doesn't really return what we want.  Do we need
+  // this functionality?
+  return static_cast<uint32_t>(std::distance(Range.begin(), Range.end()));
----------------
This is incorrect.  `getChildCount()` is supposed to return the number of values that the enumerator is going to enumerate.  The value being returned here doesn't account for the fact that not everything is an enum.  Also, I'm pretty sure we use this method in `llvm-pdbutil pretty`.  In any case, it seems pretty important to me.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumTypes.cpp:34
+NativeEnumTypes::getChildAtIndex(uint32_t Index) const {
+  // Not supported.  Random access is slow and currently unimportant.
+  return nullptr;
----------------
Not crazy about leaving this unsupported, but if we're going to do it, at the very least it needs a `llvm_unreachable`.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeExeSymbol.cpp:47
+      bool HadError = false;
+      const auto &Types = Tpi->types(&HadError);
+      if (HadError)
----------------
If you call `Tpi->typeCollection()` instead of `Tpi->types()`, then you can store a `LazyRandomTypeCollection` in the `NativeEnumTypes`, which should be more efficient.  This also solves the problem of making the session own the `TpiStream`, and should enable you to easily create an enumerator for the values of the enum.

In fact, I'd like to eventually delete the `types()` method now that we have the random access type collection.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp:109
+NativeSession::createEnumSymbol(const codeview::CVType &I) {
+  const auto Id = static_cast<SymIndexId>(SymbolCache.size());
+  SymbolCache.push_back(llvm::make_unique<NativeEnumSymbol>(*this, Id, I));
----------------
Should we be asserting that `I.Kind == LF_ENUM`?


https://reviews.llvm.org/D35738