[pdbdump] Verify LF_{CLASS,ENUM,INTERFACE,STRUCTURE,UNION} records.

[ruiu]

Summary

This patch is to verify more patterns.

[ruiu]

I tried to use the visitor, but it didn't fit well this case. If I had used the visitor,
we would have had much more code here. So I used a simple template function
instead of the visitor class.

[zturner]

lib/DebugInfo/PDB/Raw/TpiStream.cpp

78

I'm not sure if corrupt_file is the best error code to return here, because you could construct one of these out of thin air instead of coming from a file. Maybe we should have a new code called invalid_type_record.

81

Is the static_cast necessary? The enum should have overloaded & operator so you can do the operation directly.

81

What if T doesn't have a getOptions method? Will this be used for other values of T in the future? If so I think we should use the visitor instead, since it isolates the handling of each type in a separate function

82

Are these two static_casts necessary? The enum should have overloaded & operator so you can do the operation directly.

90

Is it always hashStringV1? I would think it depends on the version of the Tpi stream. I don't think we have a way to test this as I don't know how to generate a PDB file with a newer tpi stream version. Does the reference implementation hardcode a call to this function or does it go through a function pointer which might be hashStringV1 or hashStringV2?

118

If there were less code required to use the visitor, would that be preferable? I have an idea in mind to drastically reduce the amount of code needed to use it which I've been putting off. If you think we're going to add more values to this switch statement in subsequent patches, it's probably worth improving the visitor, but if you think this will be all, then maybe it's not necessary.

[ruiu]

lib/DebugInfo/PDB/Raw/TpiStream.cpp

78

Done.

81

static_cast is necessary since ClassOptions is a enum class.

If T doesn't have a getOptions method, it fails to compile, but it should be fine because the classes we have here are only ones that we need to handle (unless Microsoft extends the PDB file format in future.)

90

Yes, the reference hard-codes hashStringV1.

118

I believe this will be all. The problems with using the visitor here are, first, it needs more code to do the same thing, and second, we are not only reading type records but also hash records. We are reading parallel arrays. That's doable with the visitor, but it's more complicated than usual.

[ruiu]

• Address review comments.

[zturner]

looks good. After posting this review I found out we already have cv_error_code::corrupt_record. So probably we should use that as follows:

return llvm::make_error<CodeViewError>(cv_error_code::corrupt_record);

lib/DebugInfo/PDB/Raw/TpiStream.cpp

 };
 
 TpiStream::TpiStream(const PDBFile &File,
                      std::unique_ptr<MappedBlockStream> Stream)
     : Pdb(File), Stream(std::move(Stream)), HashFunction(nullptr) {}
 
 TpiStream::~TpiStream() {}
 
+// Computes a hash for a given TPI record.
+template <typename T, codeview::TypeRecordKind K>
+static Error getTpiHash(const codeview::CVType &Rec, uint32_t &Hash) {
+  ArrayRef<uint8_t> Data = Rec.Data;
+  ErrorOr<T> Obj = T::deserialize(K, Data);
+  if (Obj.getError())
+    return make_error<RawError>(raw_error_code::corrupt_file,
+                                "Corrupt TPI hash table.");
+
+  auto Opts = static_cast<uint16_t>(Obj->getOptions());
+  if (Opts & static_cast<uint16_t>(codeview::ClassOptions::ForwardReference)) {
+    // We don't know how to calculate a hash value for this yet.
+    // Currently we just skip it.
+    Hash = 0;
+    return Error::success();
+  }
+
+  if (!(Opts & static_cast<uint16_t>(codeview::ClassOptions::Scoped))) {
+    Hash = hashStringV1(Obj->getName());
+    return Error::success();
+  }
+
+  if (Opts & static_cast<uint16_t>(codeview::ClassOptions::HasUniqueName)) {
+    Hash = hashStringV1(Obj->getUniqueName());
+    return Error::success();
+  }
+
+  // This case is not implemented yet.
+  Hash = 0;
+  return Error::success();
+}
+
 // Verifies that a given type record matches with a given hash value.
 // Currently we only verify SRC_LINE records.
 static Error verifyTIHash(const codeview::CVType &Rec, uint32_t Expected,
                           uint32_t NumHashBuckets) {
   using namespace codeview;
 
   ArrayRef<uint8_t> D = Rec.Data;
   uint32_t Hash;
 
   switch (Rec.Type) {
   case LF_UDT_SRC_LINE:
   case LF_UDT_MOD_SRC_LINE:
     Hash = hashStringV1(StringRef((const char *)D.data(), 4));
     break;
-  case LF_ENUM: {
-    ErrorOr<EnumRecord> Enum = EnumRecord::deserialize(TypeRecordKind::Enum, D);
-    if (Enum.getError())
-      return make_error<RawError>(raw_error_code::corrupt_file,
-                                  "Corrupt TPI hash table.");
-    Hash = hashStringV1(Enum->getName());
+  case LF_CLASS:
+    if (auto EC = getTpiHash<ClassRecord, TypeRecordKind::Class>(Rec, Hash))
+      return EC;
+    break;
+  case LF_ENUM:
+    if (auto EC = getTpiHash<EnumRecord, TypeRecordKind::Enum>(Rec, Hash))
+      return EC;
+    break;
+  case LF_INTERFACE:
+    if (auto EC = getTpiHash<ClassRecord, TypeRecordKind::Interface>(Rec, Hash))
+      return EC;
+    break;
+  case LF_STRUCTURE:
+    if (auto EC = getTpiHash<ClassRecord, TypeRecordKind::Struct>(Rec, Hash))
+      return EC;
+    break;
+  case LF_UNION:
+    if (auto EC = getTpiHash<UnionRecord, TypeRecordKind::Union>(Rec, Hash))
+      return EC;
     break;
-  }
   default:
+    // This pattern is not implemented yet.
     return Error::success();
   }
 
-  if ((Hash % NumHashBuckets) != Expected)
+  if (Hash && (Hash % NumHashBuckets) != Expected)
     return make_error<RawError>(raw_error_code::corrupt_file,
                                 "Corrupt TPI hash table.");
   return Error::success();
 }
 
 Error TpiStream::reload() {
   codeview::StreamReader Reader(*Stream);