Breakpad: Extract parsing code into a separate file

[labath]

Summary

This centralizes parsing of breakpad records, which was previously
spread out over ObjectFileBreakpad and SymbolFileBreakpad.

For each record type X there is a separate breakpad::XRecord class, and
an associated parse function. The classes just store the information in
the breakpad records in a more accessible form. It is up to the users to
determine what to do with that data.

This separation also made it possible to write some targeted tests for
the parsing code, which was previously unaccessible, so I write a couple
of those too.

[clayborg]

Are we missing parsing of FUNC and line entry lines in this patch?

[clayborg]

Never mind, just read the update to your other patch. Would be nice to identify line records in toToken, but that will probably cost more CPU cycles that it is worth, so I think this is a good start.

[lemo]

Looks good. A few questions/suggestions inline.

lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp

74

the comment is great, but I think we should still have symbolic constants for all these magic values

lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h

34

should this be virtual? (even though the class doesn't have other virtual members, the class hierarchy introduces the possibility for consumers to treat them a polymorphic types - ex. storing as Record* and use the kind type to figure out the concrete type)

40

Just curious, what is the definitive convention for naming data members? A lot of LLDB code uses the m_camelCase convention.

84

most of these types are just plain data containers, so why not make them structs? (and avoid all the boilerplate associated with public accessors, repetitive constructors, ...)

[labath]

Thanks for the review. I'll create another review with the changes stemming from this.

lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp

74

I just had an idea on how to remove the numbers altogether. I'll create a separate patch to see what you think.

lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h

34

Yes, but they won't be able to polymorphically delete these objects through a base pointer, as the base destructor is protected. With a protected destructor, the compiler wouldn't warn here even if the class had other virtual members.

(BTW, this is one of the reasons I gave up on a generic "parse" function - it'd force heap allocations for polymorphic treatment without any big benefit).

40

There isn't one. :/

The reason I used the llvm naming convention here, is because this was a fresh class, which didn't have to interact with lldb at all. That's why I could (and wanted to) follow llvm coding practices as best as I could. Using lldb names for things felt weird here.

84

I thought about that too, but I couldn't really make up my mind. In the end I did this, but I can't say I had a good reason for it. I think I can change the members to be public, given that you had the same thought.

lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h

+//===-- BreakpadRecords.h ------------------------------------- -*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_PLUGINS_OBJECTFILE_BREAKPAD_BREAKPADRECORDS_H
+#define LLDB_PLUGINS_OBJECTFILE_BREAKPAD_BREAKPADRECORDS_H
+
+#include "lldb/Utility/UUID.h"
+#include "lldb/lldb-types.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/FormatProviders.h"
+
+namespace lldb_private {
+namespace breakpad {
+
+class Record {
+public:
+  enum Kind { Module, Info, File, Func, Line, Public, Stack };
+
+  /// Attempt to guess the kind of the record present in the argument without
+  /// doing a full parse. The returned kind will always be correct for valid
+  /// records, but the full parse can still fail in case of corrupted input.
+  static Kind classify(llvm::StringRef Line);
+
+protected:
+  Record(Kind K) : TheKind(K) {}
+
+  ~Record() = default;
+
+public:
+  Kind getKind() { return TheKind; }
+
+private:
+  Kind TheKind;
+};
+
+llvm::StringRef toString(Record::Kind K);
+inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, Record::Kind K) {
+  OS << toString(K);
+  return OS;
+}
+
+class ModuleRecord : public Record {
+public:
+  static llvm::Optional<ModuleRecord> parse(llvm::StringRef Line);
+  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);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const ModuleRecord &R);
+
+class InfoRecord : public Record {
+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();
+}
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const InfoRecord &R);
+
+class PublicRecord : public Record {
+public:
+  static llvm::Optional<PublicRecord> parse(llvm::StringRef Line);
+  PublicRecord(bool Multiple, lldb::addr_t Address, lldb::addr_t ParamSize,
+               llvm::StringRef Name)
+      : 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;
+  llvm::StringRef Name;
+};
+
+bool operator==(const PublicRecord &L, const PublicRecord &R);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const PublicRecord &R);
+
+} // namespace breakpad
+} // namespace lldb_private
+
+#endif // LLDB_PLUGINS_OBJECTFILE_BREAKPAD_BREAKPADRECORDS_H

lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp

+//===-- BreakpadRecords.cpp ----------------------------------- -*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/ObjectFile/Breakpad/BreakpadRecords.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/Endian.h"
+#include "llvm/Support/FormatVariadic.h"
+
+using namespace lldb_private;
+using namespace lldb_private::breakpad;
+
+namespace {
+enum class Token { Unknown, Module, Info, CodeID, File, Func, Public, Stack };
+}
+
+static Token toToken(llvm::StringRef str) {
+  return llvm::StringSwitch<Token>(str)
+      .Case("MODULE", Token::Module)
+      .Case("INFO", Token::Info)
+      .Case("CODE_ID", Token::CodeID)
+      .Case("FILE", Token::File)
+      .Case("FUNC", Token::Func)
+      .Case("PUBLIC", Token::Public)
+      .Case("STACK", Token::Stack)
+      .Default(Token::Unknown);
+}
+
+static llvm::Triple::OSType toOS(llvm::StringRef str) {
+  using llvm::Triple;
+  return llvm::StringSwitch<Triple::OSType>(str)
+      .Case("Linux", Triple::Linux)
+      .Case("mac", Triple::MacOSX)
+      .Case("windows", Triple::Win32)
+      .Default(Triple::UnknownOS);
+}
+
+static llvm::Triple::ArchType toArch(llvm::StringRef str) {
+  using llvm::Triple;
+  return llvm::StringSwitch<Triple::ArchType>(str)
+      .Case("arm", Triple::arm)
+      .Case("arm64", Triple::aarch64)
+      .Case("mips", Triple::mips)
+      .Case("ppc", Triple::ppc)
+      .Case("ppc64", Triple::ppc64)
+      .Case("s390", Triple::systemz)
+      .Case("sparc", Triple::sparc)
+      .Case("sparcv9", Triple::sparcv9)
+      .Case("x86", Triple::x86)
+      .Case("x86_64", Triple::x86_64)
+      .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;
+}
+
+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];
+    llvm::support::ulittle32_t age;
+  } data;
+  static_assert(sizeof(data) == 20, "");
+  // The textual module id encoding should be between 33 and 40 bytes long,
+  // 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)
+    return UUID();
+  uint32_t t;
+  if (to_integer(consume_front(str, 8), t, 16))
+    data.uuid1 = t;
+  else
+    return UUID();
+  for (int i = 0; i < 2; ++i) {
+    if (to_integer(consume_front(str, 4), t, 16))
+      data.uuid2[i] = t;
+    else
+      return UUID();
+  }
+  for (int i = 0; i < 8; ++i) {
+    if (!to_integer(consume_front(str, 2), data.uuid3[i], 16))
+      return UUID();
+  }
+  if (to_integer(str, t, 16))
+    data.age = t;
+  else
+    return UUID();
+
+  // 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);
+}
+
+Record::Kind Record::classify(llvm::StringRef Line) {
+  Token Tok = toToken(getToken(Line).first);
+  switch (Tok) {
+  case Token::Module:
+    return Record::Module;
+  case Token::Info:
+    return Record::Info;
+  case Token::File:
+    return Record::File;
+  case Token::Func:
+    return Record::Func;
+  case Token::Public:
+    return Record::Public;
+  case Token::Stack:
+    return Record::Stack;
+
+  case Token::CodeID:
+  case Token::Unknown:
+    // Optimistically assume that any unrecognised token means this is a line
+    // record, those don't have a special keyword and start directly with a
+    // hex number. CODE_ID should never be at the start of a line, but if it
+    // is, it can be treated the same way as a garbled line record.
+    return Record::Line;
+  }
+  llvm_unreachable("Fully covered switch above!");
+}
+
+llvm::Optional<ModuleRecord> ModuleRecord::parse(llvm::StringRef Line) {
+  // MODULE Linux x86_64 E5894855C35DCCCCCCCCCCCCCCCCCCCC0 a.out
+  llvm::StringRef Str;
+  std::tie(Str, Line) = getToken(Line);
+  if (toToken(Str) != Token::Module)
+    return llvm::None;
+
+  std::tie(Str, Line) = getToken(Line);
+  llvm::Triple::OSType OS = toOS(Str);
+  if (OS == llvm::Triple::UnknownOS)
+    return llvm::None;
+
+  std::tie(Str, Line) = getToken(Line);
+  llvm::Triple::ArchType Arch = toArch(Str);
+  if (Arch == llvm::Triple::UnknownArch)
+    return llvm::None;
+
+  std::tie(Str, Line) = getToken(Line);
+  UUID ID = parseModuleId(OS, Str);
+  if (!ID)
+    return llvm::None;
+
+  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();
+}
+
+llvm::Optional<InfoRecord> InfoRecord::parse(llvm::StringRef Line) {
+  // INFO CODE_ID 554889E55DC3CCCCCCCCCCCCCCCCCCCC [a.exe]
+  llvm::StringRef Str;
+  std::tie(Str, Line) = getToken(Line);
+  if (toToken(Str) != Token::Info)
+    return llvm::None;
+
+  std::tie(Str, Line) = getToken(Line);
+  if (toToken(Str) != Token::CodeID)
+    return llvm::None;
+
+  std::tie(Str, Line) = getToken(Line);
+  // If we don't have any text following the code ID (e.g. on linux), we should
+  // use this as the UUID. Otherwise, we should revert back to the module ID.
+  UUID ID;
+  if (Line.trim().empty()) {
+    if (Str.empty() || ID.SetFromStringRef(Str, Str.size() / 2) != Str.size())
+      return llvm::None;
+  }
+  return InfoRecord(std::move(ID));
+}
+
+llvm::raw_ostream &breakpad::operator<<(llvm::raw_ostream &OS,
+                                        const InfoRecord &R) {
+  return OS << "INFO CODE_ID " << R.getID().GetAsString();
+}
+
+llvm::Optional<PublicRecord> PublicRecord::parse(llvm::StringRef Line) {
+  // PUBLIC [m] address param_size name
+  llvm::StringRef Str;
+  std::tie(Str, Line) = getToken(Line);
+  if (toToken(Str) != Token::Public)
+    return llvm::None;
+
+  std::tie(Str, Line) = getToken(Line);
+  bool Multiple = Str == "m";
+
+  if (Multiple)
+    std::tie(Str, Line) = getToken(Line);
+  lldb::addr_t Address;
+  if (!to_integer(Str, Address, 16))
+    return llvm::None;
+
+  std::tie(Str, Line) = getToken(Line);
+  lldb::addr_t ParamSize;
+  if (!to_integer(Str, ParamSize, 16))
+    return llvm::None;
+
+  llvm::StringRef Name = Line.trim();
+  if (Name.empty())
+    return llvm::None;
+
+  return PublicRecord(Multiple, Address, ParamSize, Name);
+}
+
+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();
+}
+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());
+}
+
+llvm::StringRef breakpad::toString(Record::Kind K) {
+  switch (K) {
+  case Record::Module:
+    return "MODULE";
+  case Record::Info:
+    return "INFO";
+  case Record::File:
+    return "FILE";
+  case Record::Func:
+    return "FUNC";
+  case Record::Line:
+    return "LINE";
+  case Record::Public:
+    return "PUBLIC";
+  case Record::Stack:
+    return "STACK";
+  }
+  llvm_unreachable("Unknown record kind!");
+}

lldb/trunk/source/Plugins/ObjectFile/Breakpad/CMakeLists.txt

 add_lldb_library(lldbPluginObjectFileBreakpad PLUGIN
+  BreakpadRecords.cpp
   ObjectFileBreakpad.cpp
 
   LINK_LIBS
     lldbCore
     lldbHost
     lldbSymbol
     lldbUtility
   LINK_COMPONENTS
     Support
   )

lldb/trunk/source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h

 //===-- ObjectFileBreakpad.h ---------------------------------- -*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_PLUGINS_OBJECTFILE_BREAKPAD_OBJECTFILEBREAKPAD_H
 #define LLDB_PLUGINS_OBJECTFILE_BREAKPAD_OBJECTFILEBREAKPAD_H
 
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
-#include "llvm/ADT/Triple.h"
 
 namespace lldb_private {
 namespace breakpad {
 
 class ObjectFileBreakpad : public ObjectFile {
 public:
   //------------------------------------------------------------------
   // Static Functions

lldb/trunk/source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp

 //===-- ObjectFileBreakpad.cpp -------------------------------- -*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h"
+#include "Plugins/ObjectFile/Breakpad/BreakpadRecords.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
-#include "lldb/Utility/DataBuffer.h"
-#include "llvm/ADT/StringExtras.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::breakpad;
 
 namespace {
 struct Header {
   ArchSpec arch;
   UUID uuid;
   static llvm::Optional<Header> parse(llvm::StringRef text);
 };
-
-enum class Token { Unknown, Module, Info, File, Func, Public, Stack };
 } // namespace
 
-static Token toToken(llvm::StringRef str) {
-  return llvm::StringSwitch<Token>(str)
-      .Case("MODULE", Token::Module)
-      .Case("INFO", Token::Info)
-      .Case("FILE", Token::File)
-      .Case("FUNC", Token::Func)
-      .Case("PUBLIC", Token::Public)
-      .Case("STACK", Token::Stack)
-      .Default(Token::Unknown);
-}
-
-static llvm::StringRef toString(Token t) {
-  switch (t) {
-  case Token::Unknown:
-    return "";
-  case Token::Module:
-    return "MODULE";
-  case Token::Info:
-    return "INFO";
-  case Token::File:
-    return "FILE";
-  case Token::Func:
-    return "FUNC";
-  case Token::Public:
-    return "PUBLIC";
-  case Token::Stack:
-    return "STACK";
-  }
-  llvm_unreachable("Unknown token!");
-}
-
-static llvm::Triple::OSType toOS(llvm::StringRef str) {
-  using llvm::Triple;
-  return llvm::StringSwitch<Triple::OSType>(str)
-      .Case("Linux", Triple::Linux)
-      .Case("mac", Triple::MacOSX)
-      .Case("windows", Triple::Win32)
-      .Default(Triple::UnknownOS);
-}
-
-static llvm::Triple::ArchType toArch(llvm::StringRef str) {
-  using llvm::Triple;
-  return llvm::StringSwitch<Triple::ArchType>(str)
-      .Case("arm", Triple::arm)
-      .Case("arm64", Triple::aarch64)
-      .Case("mips", Triple::mips)
-      .Case("ppc", Triple::ppc)
-      .Case("ppc64", Triple::ppc64)
-      .Case("s390", Triple::systemz)
-      .Case("sparc", Triple::sparc)
-      .Case("sparcv9", Triple::sparcv9)
-      .Case("x86", Triple::x86)
-      .Case("x86_64", Triple::x86_64)
-      .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;
-}
-
-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];
-    llvm::support::ulittle32_t age;
-  } data;
-  static_assert(sizeof(data) == 20, "");
-  // The textual module id encoding should be between 33 and 40 bytes long,
-  // 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)
-    return UUID();
-  uint32_t t;
-  if (to_integer(consume_front(str, 8), t, 16))
-    data.uuid1 = t;
-  else
-    return UUID();
-  for (int i = 0; i < 2; ++i) {
-    if (to_integer(consume_front(str, 4), t, 16))
-      data.uuid2[i] = t;
-    else
-      return UUID();
-  }
-  for (int i = 0; i < 8; ++i) {
-    if (!to_integer(consume_front(str, 2), data.uuid3[i], 16))
-      return UUID();
-  }
-  if (to_integer(str, t, 16))
-    data.age = t;
-  else
-    return UUID();
-
-  // 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);
-}
-
 llvm::Optional<Header> Header::parse(llvm::StringRef text) {
-  // A valid module should start with something like:
-  // MODULE Linux x86_64 E5894855C35DCCCCCCCCCCCCCCCCCCCC0 a.out
-  // optionally followed by
-  // INFO CODE_ID 554889E55DC3CCCCCCCCCCCCCCCCCCCC [a.exe]
-  llvm::StringRef token, line;
+  llvm::StringRef line;
   std::tie(line, text) = text.split('\n');
-  std::tie(token, line) = getToken(line);
-  if (toToken(token) != Token::Module)
+  auto Module = ModuleRecord::parse(line);
+  if (!Module)
     return llvm::None;
 
-  std::tie(token, line) = getToken(line);
   llvm::Triple triple;
-  triple.setOS(toOS(token));
-  if (triple.getOS() == llvm::Triple::UnknownOS)
-    return llvm::None;
-
-  std::tie(token, line) = getToken(line);
-  triple.setArch(toArch(token));
-  if (triple.getArch() == llvm::Triple::UnknownArch)
-    return llvm::None;
-
-  llvm::StringRef module_id;
-  std::tie(module_id, line) = getToken(line);
+  triple.setArch(Module->getArch());
+  triple.setOS(Module->getOS());
 
   std::tie(line, text) = text.split('\n');
-  std::tie(token, line) = getToken(line);
-  if (token == "INFO") {
-    std::tie(token, line) = getToken(line);
-    if (token != "CODE_ID")
-      return llvm::None;
-
-    std::tie(token, line) = getToken(line);
-    // If we don't have any text following the code id (e.g. on linux), we
-    // should use the module id as UUID. Otherwise, we revert back to the module
-    // id.
-    if (line.trim().empty()) {
-      UUID uuid;
-      if (uuid.SetFromStringRef(token, token.size() / 2) != token.size())
-        return llvm::None;
-
-      return Header{ArchSpec(triple), uuid};
-    }
-  }
-
-  // We reach here if we don't have a INFO CODE_ID section, or we chose not to
-  // use it. In either case, we need to properly decode the module id, whose
-  // fields are encoded in big-endian.
-  UUID uuid = parseModuleId(triple.getOS(), module_id);
-  if (!uuid)
-    return llvm::None;
 
-  return Header{ArchSpec(triple), uuid};
+  auto Info = InfoRecord::parse(line);
+  UUID uuid = Info && Info->getID() ? Info->getID() : Module->getID();
+  return Header{ArchSpec(triple), std::move(uuid)};
 }
 
 void ObjectFileBreakpad::Initialize() {
   PluginManager::RegisterPlugin(GetPluginNameStatic(),
                                 GetPluginDescriptionStatic(), CreateInstance,
                                 CreateMemoryInstance, GetModuleSpecifications);
 }
 
bool ObjectFileBreakpad::GetUUID(UUID *uuid) {
   return true;
 }
 
 void ObjectFileBreakpad::CreateSections(SectionList &unified_section_list) {
   if (m_sections_ap)
     return;
   m_sections_ap = llvm::make_unique<SectionList>();
 
-  Token current_section = Token::Unknown;
+  llvm::Optional<Record::Kind> current_section;
   offset_t section_start;
   llvm::StringRef text = toStringRef(m_data.GetData());
   uint32_t next_section_id = 1;
   auto maybe_add_section = [&](const uint8_t *end_ptr) {
-    if (current_section == Token::Unknown)
+    if (!current_section)
       return; // We have been called before parsing the first line.
 
     offset_t end_offset = end_ptr - m_data.GetDataStart();
     auto section_sp = std::make_shared<Section>(
         GetModule(), this, next_section_id++,
-        ConstString(toString(current_section)), eSectionTypeOther,
+        ConstString(toString(*current_section)), eSectionTypeOther,
         /*file_vm_addr*/ 0, /*vm_size*/ 0, section_start,
         end_offset - section_start, /*log2align*/ 0, /*flags*/ 0);
     m_sections_ap->AddSection(section_sp);
     unified_section_list.AddSection(section_sp);
   };
   while (!text.empty()) {
     llvm::StringRef line;
     std::tie(line, text) = text.split('\n');
 
-    Token token = toToken(getToken(line).first);
-    if (token == Token::Unknown) {
-      // We assume this is a line record, which logically belongs to the Func
-      // section. Errors will be handled when parsing the Func section.
-      token = Token::Func;
+    Record::Kind next_section = Record::classify(line);
+    if (next_section == Record::Line) {
+      // Line records logically belong to the preceding Func record, so we put
+      // them in the same section.
+      next_section = Record::Func;
     }
-    if (token == current_section)
+    if (next_section == current_section)
       continue;
 
     // Changing sections, finish off the previous one, if there was any.
     maybe_add_section(line.bytes_begin());
     // And start a new one.
-    current_section = token;
+    current_section = next_section;
     section_start = line.bytes_begin() - m_data.GetDataStart();
   }
   // Finally, add the last section.
   maybe_add_section(m_data.GetDataEnd());
 }

lldb/trunk/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

 //===-- SymbolFileBreakpad.cpp ----------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h"
+#include "Plugins/ObjectFile/Breakpad/BreakpadRecords.h"
 #include "Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/TypeMap.h"
 #include "lldb/Utility/Log.h"
void SymbolFileBreakpad::AddSymbols(Symtab &symtab) {
   if (base == LLDB_INVALID_ADDRESS) {
     LLDB_LOG(log, "Unable to fetch the base address of object file. Skipping "
                   "symtab population.");
     return;
   }
 
   const SectionList &list = *module.GetSectionList();
   for (llvm::StringRef line : lines(*m_obj_file, ConstString("PUBLIC"))) {
-    // PUBLIC [m] address param_size name
-    // skip PUBLIC keyword
-    line = getToken(line).second;
-    llvm::StringRef token;
-    std::tie(token, line) = getToken(line);
-    if (token == "m")
-      std::tie(token, line) = getToken(line);
-
-    addr_t address;
-    if (!to_integer(token, address, 16))
+    auto record = PublicRecord::parse(line);
+    if (!record) {
+      LLDB_LOG(log, "Failed to parse: {0}. Skipping record.", line);
       continue;
-    address += base;
-
-    // skip param_size
-    line = getToken(line).second;
-
-    llvm::StringRef name = line.trim();
+    }
+    addr_t file_address = base + record->getAddress();
 
-    SectionSP section_sp = list.FindSectionContainingFileAddress(address);
+    SectionSP section_sp = list.FindSectionContainingFileAddress(file_address);
     if (!section_sp) {
       LLDB_LOG(log,
                "Ignoring symbol {0}, whose address ({1}) is outside of the "
                "object file. Mismatched symbol file?",
-               name, address);
+               record->getName(), file_address);
       continue;
     }
 
     symtab.AddSymbol(Symbol(
-        /*symID*/ 0, Mangled(name, /*is_mangled*/ false), eSymbolTypeCode,
+        /*symID*/ 0, Mangled(record->getName(), /*is_mangled*/ false),
+        eSymbolTypeCode,
         /*is_global*/ true, /*is_debug*/ false, /*is_trampoline*/ false,
         /*is_artificial*/ false,
-        AddressRange(section_sp, address - section_sp->GetFileAddress(), 0),
+        AddressRange(section_sp, file_address - section_sp->GetFileAddress(),
+                     0),
         /*size_is_valid*/ 0, /*contains_linker_annotations*/ false,
         /*flags*/ 0));
   }
 
   // TODO: Process FUNC records as well.
 
   symtab.CalculateSymbolSizes();
 }

lldb/trunk/unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp

+//===-- BreakpadRecordsTest.cpp ---------------------------------*- C++ -*-===//
+//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/ObjectFile/Breakpad/BreakpadRecords.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::breakpad;
+
+TEST(Record, classify) {
+  EXPECT_EQ(Record::Module, Record::classify("MODULE"));
+  EXPECT_EQ(Record::Info, Record::classify("INFO"));
+  EXPECT_EQ(Record::File, Record::classify("FILE"));
+  EXPECT_EQ(Record::Func, Record::classify("FUNC"));
+  EXPECT_EQ(Record::Public, Record::classify("PUBLIC"));
+  EXPECT_EQ(Record::Stack, Record::classify("STACK"));
+
+  // Any line which does not start with a known keyword will be classified as a
+  // line record, as those are the only ones that start without a keyword.
+  EXPECT_EQ(Record::Line, Record::classify("deadbeef"));
+  EXPECT_EQ(Record::Line, Record::classify("12"));
+  EXPECT_EQ(Record::Line, Record::classify("CODE_ID"));
+}
+
+TEST(ModuleRecord, parse) {
+  EXPECT_EQ(ModuleRecord(llvm::Triple::Linux, llvm::Triple::x86_64,
+                         UUID::fromData("@ABCDEFGHIJKLMNO", 16)),
+            ModuleRecord::parse(
+                "MODULE Linux x86_64 434241404544474648494a4b4c4d4e4f0 a.out"));
+
+  EXPECT_EQ(llvm::None, ModuleRecord::parse("MODULE"));
+  EXPECT_EQ(llvm::None, ModuleRecord::parse("MODULE Linux"));
+  EXPECT_EQ(llvm::None, ModuleRecord::parse("MODULE Linux x86_64"));
+  EXPECT_EQ(llvm::None,
+            ModuleRecord::parse("MODULE Linux x86_64 deadbeefbaadf00d"));
+}
+
+TEST(InfoRecord, parse) {
+  EXPECT_EQ(InfoRecord(UUID::fromData("@ABCDEFGHIJKLMNO", 16)),
+            InfoRecord::parse("INFO CODE_ID 404142434445464748494a4b4c4d4e4f"));
+  EXPECT_EQ(InfoRecord(UUID()), InfoRecord::parse("INFO CODE_ID 47 a.exe"));
+
+  EXPECT_EQ(llvm::None, InfoRecord::parse("INFO"));
+  EXPECT_EQ(llvm::None, InfoRecord::parse("INFO CODE_ID"));
+}
+
+TEST(PublicRecord, parse) {
+  EXPECT_EQ(PublicRecord(true, 0x47, 0x8, "foo"),
+            PublicRecord::parse("PUBLIC m 47 8 foo"));
+  EXPECT_EQ(PublicRecord(false, 0x47, 0x8, "foo"),
+            PublicRecord::parse("PUBLIC 47 8 foo"));
+
+  EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC 47 8"));
+  EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC 47"));
+  EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC m"));
+  EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC"));
+}

lldb/trunk/unittests/ObjectFile/Breakpad/CMakeLists.txt

+add_lldb_unittest(ObjectFileBreakpadTests
+  BreakpadRecordsTest.cpp
+
+  LINK_LIBS
+    lldbPluginObjectFileBreakpad
+  )

lldb/trunk/unittests/ObjectFile/CMakeLists.txt

+add_subdirectory(Breakpad)
 add_subdirectory(ELF)