BreakpadRecords: Address post-commit feedback

Summary:
This addresses the issues raised in D56844. It removes the accessors from the
breakpad record structures by making the fields public. Also, I refactor the
UUID parsing code to remove hard-coded constants.

Reviewers: lemo

Subscribers: clayborg, lldb-commits

Differential Revision: https://reviews.llvm.org/D57037

llvm-svn: 352021

3 files changed, 53 insertions, 67 deletions

diff --git a/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp b/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
index 0dfa6fc3bbe..c0b2ac9e6d9 100644
--- a/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
+++ b/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
@@ -56,17 +56,31 @@ static llvm::Triple::ArchType toArch(llvm::StringRef str) {
       .Default(Triple::UnknownArch);
 }
 
-static llvm::StringRef consume_front(llvm::StringRef &str, size_t n) {
-  llvm::StringRef result = str.take_front(n);
-  str = str.drop_front(n);
-  return result;
+/// Return the number of hex digits needed to encode an (POD) object of a given
+/// type.
+template <typename T> static constexpr size_t hex_digits() {
+  return 2 * sizeof(T);
+}
+
+/// Consume the right number of digits from the input StringRef and convert it
+/// to the endian-specific integer N. Return true on success.
+template <typename T> static bool consume_hex_integer(llvm::StringRef &str, T &N) {
+  llvm::StringRef chunk = str.take_front(hex_digits<T>());
+  uintmax_t t;
+  if (!to_integer(chunk, t, 16))
+    return false;
+  N = t;
+  str = str.drop_front(hex_digits<T>());
+  return true;
 }
 
 static UUID parseModuleId(llvm::Triple::OSType os, llvm::StringRef str) {
-  struct uuid_data {
-    llvm::support::ulittle32_t uuid1;
-    llvm::support::ulittle16_t uuid2[2];
-    uint8_t uuid3[8];
+  struct data_t {
+    struct uuid_t {
+      llvm::support::ulittle32_t part1;
+      llvm::support::ulittle16_t part2[2];
+      uint8_t part3[8];
+    } uuid;
     llvm::support::ulittle32_t age;
   } data;
   static_assert(sizeof(data) == 20, "");
@@ -74,31 +88,28 @@ static UUID parseModuleId(llvm::Triple::OSType os, llvm::StringRef str) {
   // depending on the size of the age field, which is of variable length.
   // The first three chunks of the id are encoded in big endian, so we need to
   // byte-swap those.
-  if (str.size() < 33 || str.size() > 40)
+  if (str.size() <= hex_digits<data_t::uuid_t>() ||
+      str.size() > hex_digits<data_t>())
     return UUID();
-  uint32_t t;
-  if (to_integer(consume_front(str, 8), t, 16))
-    data.uuid1 = t;
-  else
+  if (!consume_hex_integer(str, data.uuid.part1))
     return UUID();
-  for (int i = 0; i < 2; ++i) {
-    if (to_integer(consume_front(str, 4), t, 16))
-      data.uuid2[i] = t;
-    else
+  for (auto &t : data.uuid.part2) {
+    if (!consume_hex_integer(str, t))
       return UUID();
   }
-  for (int i = 0; i < 8; ++i) {
-    if (!to_integer(consume_front(str, 2), data.uuid3[i], 16))
+  for (auto &t : data.uuid.part3) {
+    if (!consume_hex_integer(str, t))
       return UUID();
   }
-  if (to_integer(str, t, 16))
-    data.age = t;
-  else
+  uint32_t age;
+  if (!to_integer(str, age, 16))
     return UUID();
+  data.age = age;
 
   // On non-windows, the age field should always be zero, so we don't include to
   // match the native uuid format of these platforms.
-  return UUID::fromData(&data, os == llvm::Triple::Win32 ? 20 : 16);
+  return UUID::fromData(&data, os == llvm::Triple::Win32 ? sizeof(data)
+                                                         : sizeof(data.uuid));
 }
 
 Record::Kind Record::classify(llvm::StringRef Line) {
@@ -153,15 +164,11 @@ llvm::Optional<ModuleRecord> ModuleRecord::parse(llvm::StringRef Line) {
   return ModuleRecord(OS, Arch, std::move(ID));
 }
 
-bool breakpad::operator==(const ModuleRecord &L, const ModuleRecord &R) {
-  return L.getOS() == R.getOS() && L.getArch() == R.getArch() &&
-         L.getID() == R.getID();
-}
 llvm::raw_ostream &breakpad::operator<<(llvm::raw_ostream &OS,
                                         const ModuleRecord &R) {
-  return OS << "MODULE " << llvm::Triple::getOSTypeName(R.getOS()) << " "
-            << llvm::Triple::getArchTypeName(R.getArch()) << " "
-            << R.getID().GetAsString();
+  return OS << "MODULE " << llvm::Triple::getOSTypeName(R.OS) << " "
+            << llvm::Triple::getArchTypeName(R.Arch) << " "
+            << R.ID.GetAsString();
 }
 
 llvm::Optional<InfoRecord> InfoRecord::parse(llvm::StringRef Line) {
@@ -188,7 +195,7 @@ llvm::Optional<InfoRecord> InfoRecord::parse(llvm::StringRef Line) {
 
 llvm::raw_ostream &breakpad::operator<<(llvm::raw_ostream &OS,
                                         const InfoRecord &R) {
-  return OS << "INFO CODE_ID " << R.getID().GetAsString();
+  return OS << "INFO CODE_ID " << R.ID.GetAsString();
 }
 
 static bool parsePublicOrFunc(llvm::StringRef Line, bool &Multiple,
@@ -242,15 +249,14 @@ llvm::Optional<FuncRecord> FuncRecord::parse(llvm::StringRef Line) {
 }
 
 bool breakpad::operator==(const FuncRecord &L, const FuncRecord &R) {
-  return L.getMultiple() == R.getMultiple() &&
-         L.getAddress() == R.getAddress() && L.getSize() == R.getSize() &&
-         L.getParamSize() == R.getParamSize() && L.getName() == R.getName();
+  return L.Multiple == R.Multiple && L.Address == R.Address &&
+         L.Size == R.Size && L.ParamSize == R.ParamSize && L.Name == R.Name;
 }
 llvm::raw_ostream &breakpad::operator<<(llvm::raw_ostream &OS,
                                         const FuncRecord &R) {
   return OS << llvm::formatv("FUNC {0}{1:x-} {2:x-} {3:x-} {4}",
-                             R.getMultiple() ? "m " : "", R.getAddress(),
-                             R.getSize(), R.getParamSize(), R.getName());
+                             R.Multiple ? "m " : "", R.Address, R.Size,
+                             R.ParamSize, R.Name);
 }
 
 llvm::Optional<PublicRecord> PublicRecord::parse(llvm::StringRef Line) {
@@ -265,15 +271,14 @@ llvm::Optional<PublicRecord> PublicRecord::parse(llvm::StringRef Line) {
 }
 
 bool breakpad::operator==(const PublicRecord &L, const PublicRecord &R) {
-  return L.getMultiple() == R.getMultiple() &&
-         L.getAddress() == R.getAddress() &&
-         L.getParamSize() == R.getParamSize() && L.getName() == R.getName();
+  return L.Multiple == R.Multiple && L.Address == R.Address &&
+         L.ParamSize == R.ParamSize && L.Name == R.Name;
 }
 llvm::raw_ostream &breakpad::operator<<(llvm::raw_ostream &OS,
                                         const PublicRecord &R) {
   return OS << llvm::formatv("PUBLIC {0}{1:x-} {2:x-} {3}",
-                             R.getMultiple() ? "m " : "", R.getAddress(),
-                             R.getParamSize(), R.getName());
+                             R.Multiple ? "m " : "", R.Address, R.ParamSize,
+                             R.Name);
 }
 
 llvm::StringRef breakpad::toString(Record::Kind K) {
diff --git a/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h b/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
index d1fb9a1cdbe..472b2e25e65 100644
--- a/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
+++ b/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
@@ -51,17 +51,14 @@ public:
   ModuleRecord(llvm::Triple::OSType OS, llvm::Triple::ArchType Arch, UUID ID)
       : Record(Module), OS(OS), Arch(Arch), ID(std::move(ID)) {}
 
-  llvm::Triple::OSType getOS() const { return OS; }
-  llvm::Triple::ArchType getArch() const { return Arch; }
-  const UUID &getID() const { return ID; }
-
-private:
   llvm::Triple::OSType OS;
   llvm::Triple::ArchType Arch;
   UUID ID;
 };
 
-bool operator==(const ModuleRecord &L, const ModuleRecord &R);
+bool operator==(const ModuleRecord &L, const ModuleRecord &R) {
+  return L.OS == R.OS && L.Arch == R.Arch && L.ID == R.ID;
+}
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const ModuleRecord &R);
 
 class InfoRecord : public Record {
@@ -69,14 +66,11 @@ public:
   static llvm::Optional<InfoRecord> parse(llvm::StringRef Line);
   InfoRecord(UUID ID) : Record(Info), ID(std::move(ID)) {}
 
-  const UUID &getID() const { return ID; }
-
-private:
   UUID ID;
 };
 
 inline bool operator==(const InfoRecord &L, const InfoRecord &R) {
-  return L.getID() == R.getID();
+  return L.ID == R.ID;
 }
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const InfoRecord &R);
 
@@ -88,13 +82,6 @@ public:
       : Record(Module), Multiple(Multiple), Address(Address), Size(Size),
         ParamSize(ParamSize), Name(Name) {}
 
-  bool getMultiple() const { return Multiple; }
-  lldb::addr_t getAddress() const { return Address; }
-  lldb::addr_t getSize() const { return Size; }
-  lldb::addr_t getParamSize() const { return ParamSize; }
-  llvm::StringRef getName() const { return Name; }
-
-private:
   bool Multiple;
   lldb::addr_t Address;
   lldb::addr_t Size;
@@ -113,12 +100,6 @@ public:
       : Record(Module), Multiple(Multiple), Address(Address),
         ParamSize(ParamSize), Name(Name) {}
 
-  bool getMultiple() const { return Multiple; }
-  lldb::addr_t getAddress() const { return Address; }
-  lldb::addr_t getParamSize() const { return ParamSize; }
-  llvm::StringRef getName() const { return Name; }
-
-private:
   bool Multiple;
   lldb::addr_t Address;
   lldb::addr_t ParamSize;
diff --git a/lldb/source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp b/lldb/source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
index c7b5a9943a5..8825c83f747 100644
--- a/lldb/source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
+++ b/lldb/source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
@@ -32,13 +32,13 @@ llvm::Optional<Header> Header::parse(llvm::StringRef text) {
     return llvm::None;
 
   llvm::Triple triple;
-  triple.setArch(Module->getArch());
-  triple.setOS(Module->getOS());
+  triple.setArch(Module->Arch);
+  triple.setOS(Module->OS);
 
   std::tie(line, text) = text.split('\n');
 
   auto Info = InfoRecord::parse(line);
-  UUID uuid = Info && Info->getID() ? Info->getID() : Module->getID();
+  UUID uuid = Info && Info->ID ? Info->ID : Module->ID;
   return Header{ArchSpec(triple), std::move(uuid)};
 }