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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:78
@@ +77,3 @@
+  if (Obj.getError())
+    return make_error<RawError>(raw_error_code::corrupt_file,
+                                "Corrupt TPI hash table.");
----------------
zturner wrote:
> 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`.
Done.

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:81
@@ +80,3 @@
+
+  auto Opts = static_cast<uint16_t>(Obj->getOptions());
+  if (Opts & static_cast<uint16_t>(codeview::ClassOptions::ForwardReference)) {
----------------
zturner wrote:
> zturner wrote:
> > Is the `static_cast` necessary?  The enum should have overloaded `&` operator so you can do the operation directly.
> 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
`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.)

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:90
@@ +89,3 @@
+  if (!(Opts & static_cast<uint16_t>(codeview::ClassOptions::Scoped))) {
+    Hash = hashStringV1(Obj->getName());
+    return Error::success();
----------------
zturner wrote:
> 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`?
Yes, the reference hard-codes hashStringV1.

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:118
@@ -85,7 +117,3 @@
     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))
----------------
zturner wrote:
> 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.
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.


http://reviews.llvm.org/D21361