[PATCH] D20138: Refactor CodeView TypeRecord visitors to use common structures

[Reid Kleckner via llvm-commits]

rnk added a comment.

There's some common patterns we should fix up for safety. I'll re-review all the .slice call sites again after the common patterns are fixed up.


================
Comment at: include/llvm/DebugInfo/CodeView/CVTypeVisitor.h:73
@@ -73,1 +72,3 @@
+    if (Result.getError())                                                     \
       return;                                                                  \
+    DerivedThis->visit##ClassName(Record.Type, *Result);                       \
----------------
This should be `return parseError();`, and if we were really cool we'd propagate out the Error. I'm fine with the existing bool.

================
Comment at: include/llvm/DebugInfo/CodeView/CodeView.h:429
@@ -428,2 +428,3 @@
   OneMethod = 0x1511,
+  TypeServer2 = 0x1515,
   VirtualFunctionTable = 0x151d,
----------------
Eventually we should unify this with CVLeafTypes, but today is not that day.

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:18
@@ -16,1 +17,3 @@
 #include "llvm/DebugInfo/CodeView/TypeIndex.h"
+#include "llvm/Object/Error.h"
+#include "llvm/Support/ErrorOr.h"
----------------
lib/DebugInfo/CodeView shouldn't depend on lib/Object, maybe go for std::errc::illegal_byte_sequence or something for now.

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:115-118
@@ +114,6 @@
+  static ErrorOr<MemberPointerInfo> deserialize(ArrayRef<uint8_t> &Data) {
+    if (Data.size() < sizeof(Layout))
+      return object::make_error_code(object::object_error::parse_failed);
+    const Layout *L = reinterpret_cast<const Layout *>(Data.data());
+    Data = Data.slice(sizeof(Layout));
+    TypeIndex T = L->ClassType;
----------------
Is there a reason to not use `consumeObject` here?

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:164
@@ +163,3 @@
+    const Layout *L = reinterpret_cast<const Layout *>(Data.data());
+    Data = Data.slice(sizeof(Layout));
+    TypeIndex M = L->ModifiedType;
----------------
ditto, consumeObject?

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:299
@@ +298,3 @@
+      return object::make_error_code(object::object_error::parse_failed);
+    StringRef Name = StringRef(reinterpret_cast<const char *>(Data.data()));
+    Data = Data.slice(Name.size() + 1);
----------------
This needs handling to avoid buffer overrun when the string is not null terminated. I think you should add a helper like this and then use it everywhere you use `reinterpret_cast<const char *>`:
  StringRef consumeCString(ArrayRef<uint8_t> &Data) {
    StringRef Str, Rest;
    std::tie(Str, Rest) = getBytesAsCharacters(Data).split('\0');
    Data = ArrayRef<uint8_t>(reinterpret_cast<const uint8_t*>(Rest.data()), Rest.size());
    return Str;
  }

Alternatively, we could treat lack of null termination as an error, but at least truncating is better than running over.

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:337
@@ +336,3 @@
+        reinterpret_cast<const TypeIndex *>(Data.data()), L->NumArgs);
+    Data = Data.slice(L->NumArgs * sizeof(TypeIndex));
+    return StringListRecord(Kind, Indices);
----------------
This slice should be checked against Data.size() or we'll abort

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:480
@@ +479,3 @@
+      return object::make_error_code(object::object_error::parse_failed);
+    StringRef Name = StringRef(reinterpret_cast<const char *>(Data.data()));
+    Data = Data.slice(Name.size() + 1);
----------------
getBytesAsCString

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:519
@@ +518,3 @@
+      return object::make_error_code(object::object_error::parse_failed);
+    StringRef Name(reinterpret_cast<const char *>(Data.data()));
+    Data = Data.slice(Name.size() + 1);
----------------
getBytesAsCString

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:582
@@ -216,1 +581,3 @@
 
+  static ErrorOr<AggregateRecord> deserialize(TypeRecordKind Kind,
+                                              ArrayRef<uint8_t> &Data) {
----------------
I agree, splitting this seems useful.

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:751
@@ +750,3 @@
+    Data = Data.slice(sizeof(Layout));
+    for (int I = 0; I < L->VFEntryCount; ++I) {
+      if (Data.empty())
----------------
Woo, nibble decoding. I think the dumper doesn't even print these because they are so uninteresting in a post 16 bit world.

You should check up front that there are at least `(VFEntryCount +1) / 2` bytes in Data, or the slices will abort.


http://reviews.llvm.org/D20138