breakpad: Add FUNC records to the symtab

[labath]

Summary

This patch extends SymbolFileBreakpad::AddSymbols to include the symbols
from the FUNC records too. These symbols come from the debug info and
have a size associated with them, so they are given preference in case
there is a PUBLIC record for the same address.

To achieve this, I first pre-process the symbols into a temporary
DenseMap, and then insert the uniqued symbols into the module's symtab.

[clayborg]

source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp

29–48 ↗

(On Diff #181246)

Move to BreakpadLine.cpp/.h?

source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h

20–23 ↗

(On Diff #181246)

Should we have a "Line" class in lldb_private::breakpad::Line? All functions that parse a breakpad line could be in this new class? Seeing as we have symbol files, object files and the process plug-in might need to parse these lines, seems like it would be cleaner? Maybe in BreakpadLine.cpp/.h?

namespace lldb_private {
namespace breakpad {
class Line {
  enum class Token { Unknown, Module, Info, File, Func, Public, Stack };
  Line(llvm::StringRef s);
  Token parseToken();
  static llvm::StringRef tokenToString(Token t);
};

source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

27

Move this functionality into llvm::breakpad::Line?

186

change "llvm::StringRef line" to "lldb_private::breakpad::Line" and remove second parameter as the "Line" class can know its token type?

202–209

If we make a Line class this code would become:

lldb_private::breakpad::Line bp_line(line);
if (bp_line.GetToken() != Token::Func)
  continue;
parse(bp_line);

214

If we make a Line class this code would become:

parse(bp_line);

[clayborg]

source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

201

Should the iterator for Token::Func just return "FUNC" objects only? Maybe we add a Token::Line to the Token enumeration and then add an optional second parameter to the iterator? So any code that would want a "FUNC" and its lines, would do:

for (llvm::StringRef line: lines(*m_obj_file, Token::Func, Token::Line)) {
}

209

If we modify the iterator this would become:

parse(lldb_private::breakpad::Line(line));

[labath]

Thanks for the review. I've refactored the code to separate (and centralize) the
breakpad parsing from the part which does presents the information to lldb.

I've done this slightly differently than suggested (the new file is called
BreakpadRecords, as that's how breakpad calls them, and there as separate class
for each record type instead of just a function), but I believe it addresses the
root problem.

The main reason I created classes for everything is because I thought I'd make
the iterator class return those. I may still do that at some point but now it
did not seem worth it now because it makes it harder to report parsing errors,
and it did not make the call sites much cleaner.

[labath]

source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

27

I haven't moved this part to the new file because (unlike everything else in that file) this depends on the ObjectFile class. Theoretically it can be moved to ObjectFileBreapad.h (or a new file) if needed, but ideally I'd like to avoid anyone else needing to parse the breakpad file contents. If the minidump process plugin for instance needed to access some of this information, I would have it go through the SymbolFile interface, which can present it in a nicer fashion.

201

With the new parse functions this should not be necessary as you can just say "try to parse this line as a FUNC record", and that will automatically reject LINE record as well as any other malformed lines. I think that would make sense if I made the iterator perform the parsing internally and return already parsed records.

I didn't do that (for now) because returning parse errors from an iterator gets weird, and it didn't seem worth the small amount of code it would save.

[clayborg]

Much cleaner! Thanks

lit/SymbolFile/Breakpad/Inputs/symtab.syms

 MODULE Linux x86_64 761550E08086333960A9074A9CE2895C0 a.out
 INFO CODE_ID E05015768680393360A9074A9CE2895C
 FILE 0 /tmp/a.c
 PUBLIC b0 0 f1
 PUBLIC m c0 0 f2
 PUBLIC d0 0 _start
 PUBLIC ff 0 _out_of_range_ignored
+FUNC b0 c 0 f1_func
+FUNC m a0 d 0 func_only

lit/SymbolFile/Breakpad/symtab.test

 # RUN: yaml2obj %S/Inputs/basic-elf.yaml > %T/symtab.out
 # RUN: %lldb %T/symtab.out -o "target symbols add -s symtab.out %S/Inputs/symtab.syms" \
 # RUN:   -s %s | FileCheck %s
 
 # CHECK-LABEL: (lldb) image dump symtab symtab.out
-# CHECK: Symtab, file = {{.*}}symtab.out, num_symbols = 3:
+# CHECK: Symtab, file = {{.*}}symtab.out, num_symbols = 4:
 # CHECK: Index   UserID DSX Type            File Address/Value Load Address       Size               Flags      Name
-# CHECK: [    0]      0   X Code            0x00000000004000b0                    0x0000000000000010 0x00000000 f1
-# CHECK: [    1]      0   X Code            0x00000000004000c0                    0x0000000000000010 0x00000000 f2
-# CHECK: [    2]      0   X Code            0x00000000004000d0                    0x0000000000000022 0x00000000 _start
+# CHECK: [    0]      0   X Code            0x00000000004000c0                    0x0000000000000010 0x00000000 f2
+# CHECK: [    1]      0   X Code            0x00000000004000d0                    0x0000000000000022 0x00000000 _start
+# CHECK: [    2]      0   X Code            0x00000000004000a0                    0x000000000000000d 0x00000000 func_only
+# CHECK: [    3]      0   X Code            0x00000000004000b0                    0x000000000000000c 0x00000000 f1_func
 
 # CHECK-LABEL: (lldb) image lookup -a 0x4000b0 -v
 # CHECK: Address: symtab.out[0x00000000004000b0] (symtab.out.PT_LOAD[0]..text2 + 0)
-# CHECK: Symbol: id = {0x00000000}, range = [0x00000000004000b0-0x00000000004000c0), name="f1"
+# CHECK: Symbol: id = {0x00000000}, range = [0x00000000004000b0-0x00000000004000bc), name="f1_func"
 
 # CHECK-LABEL: (lldb) image lookup -n f2 -v
 # CHECK: Address: symtab.out[0x00000000004000c0] (symtab.out.PT_LOAD[0]..text2 + 16)
 # CHECK: Symbol: id = {0x00000000}, range = [0x00000000004000c0-0x00000000004000d0), name="f2"
 
 image dump symtab symtab.out
 image lookup -a 0x4000b0 -v
 image lookup -n f2 -v
 exit

source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h

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 FuncRecord : public Record {
+public:
+  static llvm::Optional<FuncRecord> parse(llvm::StringRef Line);
+  FuncRecord(bool Multiple, lldb::addr_t Address, lldb::addr_t Size,
+             lldb::addr_t ParamSize, llvm::StringRef Name)
+      : 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;
+  lldb::addr_t ParamSize;
+  llvm::StringRef Name;
+};
+
+bool operator==(const FuncRecord &L, const FuncRecord &R);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const FuncRecord &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) {}
 

source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp

llvm::Optional<InfoRecord> InfoRecord::parse(llvm::StringRef Line) {
   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) {
+static bool parsePublicOrFunc(llvm::StringRef Line, bool &Multiple,
+                              lldb::addr_t &Address, lldb::addr_t *Size,
+                              lldb::addr_t &ParamSize, llvm::StringRef &Name) {
   // PUBLIC [m] address param_size name
+  // or
+  // FUNC [m] address size param_size name
+
+  Token Tok = Size ? Token::Func : Token::Public;
+
   llvm::StringRef Str;
   std::tie(Str, Line) = getToken(Line);
-  if (toToken(Str) != Token::Public)
-    return llvm::None;
+  if (toToken(Str) != Tok)
+    return false;
 
   std::tie(Str, Line) = getToken(Line);
-  bool Multiple = Str == "m";
+  Multiple = Str == "m";
 
   if (Multiple)
     std::tie(Str, Line) = getToken(Line);
-  lldb::addr_t Address;
   if (!to_integer(Str, Address, 16))
-    return llvm::None;
+    return false;
+
+  if (Tok == Token::Func) {
+    std::tie(Str, Line) = getToken(Line);
+    if (!to_integer(Str, *Size, 16))
+      return false;
+  }
 
   std::tie(Str, Line) = getToken(Line);
-  lldb::addr_t ParamSize;
   if (!to_integer(Str, ParamSize, 16))
-    return llvm::None;
+    return false;
 
-  llvm::StringRef Name = Line.trim();
+  Name = Line.trim();
   if (Name.empty())
-    return llvm::None;
+    return false;
+
+  return true;
+}
+
+llvm::Optional<FuncRecord> FuncRecord::parse(llvm::StringRef Line) {
+  bool Multiple;
+  lldb::addr_t Address, Size, ParamSize;
+  llvm::StringRef Name;
+
+  if (parsePublicOrFunc(Line, Multiple, Address, &Size, ParamSize, Name))
+    return FuncRecord(Multiple, Address, Size, ParamSize, Name);
+
+}
 
+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();
+}
+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());
+}
+
+llvm::Optional<PublicRecord> PublicRecord::parse(llvm::StringRef Line) {
+  bool Multiple;
+  lldb::addr_t Address, ParamSize;
+  llvm::StringRef Name;
+
+  if (parsePublicOrFunc(Line, Multiple, Address, nullptr, ParamSize, Name))
     return PublicRecord(Multiple, Address, ParamSize, Name);
+
+  return llvm::None;
 }
 
 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,

source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

 #include "lldb/Utility/Log.h"
 #include "llvm/ADT/StringExtras.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::breakpad;
 
 namespace {
 class LineIterator {
 public:
   // begin iterator for sections of given type
-  LineIterator(ObjectFile &obj, ConstString section_type)
-      : m_obj(&obj), m_section_type(section_type), m_next_section_idx(0) {
+  LineIterator(ObjectFile &obj, Record::Kind section_type)
+      : m_obj(&obj), m_section_type(toString(section_type)),
+        m_next_section_idx(0) {
     ++*this;
   }
 
   // end iterator
   explicit LineIterator(ObjectFile &obj)
       : m_obj(&obj),
         m_next_section_idx(m_obj->GetSectionList()->GetNumSections(0)) {}
 
m_next_text =
         llvm::StringRef(reinterpret_cast<const char *>(data.GetDataStart()),
                         data.GetByteSize());
   }
   std::tie(m_current_text, m_next_text) = m_next_text.split('\n');
   return *this;
 }
 
 static llvm::iterator_range<LineIterator> lines(ObjectFile &obj,
-                                                ConstString section_type) {
+                                                Record::Kind section_type) {
   return llvm::make_range(LineIterator(obj, section_type), LineIterator(obj));
 }
 
 void SymbolFileBreakpad::Initialize() {
   PluginManager::RegisterPlugin(GetPluginNameStatic(),
                                 GetPluginDescriptionStatic(), CreateInstance,
                                 DebuggerInitialize);
 }
void SymbolFileBreakpad::AddSymbols(Symtab &symtab) {
   addr_t base = module.GetObjectFile()->GetBaseAddress().GetFileAddress();
   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"))) {
-    auto record = PublicRecord::parse(line);
-    if (!record) {
-      LLDB_LOG(log, "Failed to parse: {0}. Skipping record.", line);
-      continue;
-    }
-    addr_t file_address = base + record->getAddress();
-
-    SectionSP section_sp = list.FindSectionContainingFileAddress(file_address);
+  llvm::DenseMap<addr_t, Symbol> symbols;
+  auto add_symbol = [&](addr_t address, llvm::Optional<addr_t> size,
+                        llvm::StringRef name) {
+    address += base;
+    SectionSP section_sp = list.FindSectionContainingFileAddress(address);
     if (!section_sp) {
       LLDB_LOG(log,
                "Ignoring symbol {0}, whose address ({1}) is outside of the "
                "object file. Mismatched symbol file?",
-               record->getName(), file_address);
-      continue;
+               name, address);
+      return;
     }
+    symbols.try_emplace(
+        address, /*symID*/ 0, Mangled(name, /*is_mangled*/ false),
+        eSymbolTypeCode, /*is_global*/ true, /*is_debug*/ false,
+        /*is_trampoline*/ false, /*is_artificial*/ false,
+        AddressRange(section_sp, address - section_sp->GetFileAddress(),
+                     size.getValueOr(0)),
+        size.hasValue(), /*contains_linker_annotations*/ false, /*flags*/ 0);
+  };
 
-    symtab.AddSymbol(Symbol(
-        /*symID*/ 0, Mangled(record->getName(), /*is_mangled*/ false),
-        eSymbolTypeCode,
-        /*is_global*/ true, /*is_debug*/ false, /*is_trampoline*/ false,
-        /*is_artificial*/ false,
-        AddressRange(section_sp, file_address - section_sp->GetFileAddress(),
-                     0),
-        /*size_is_valid*/ 0, /*contains_linker_annotations*/ false,
-        /*flags*/ 0));
+  for (llvm::StringRef line : lines(*m_obj_file, Record::Func)) {
+    if (auto record = FuncRecord::parse(line))
+      add_symbol(record->getAddress(), record->getSize(), record->getName());
   }
 
-  // TODO: Process FUNC records as well.
+  for (llvm::StringRef line : lines(*m_obj_file, Record::Public)) {
+    if (auto record = PublicRecord::parse(line))
+      add_symbol(record->getAddress(), llvm::None, record->getName());
+    else
+      LLDB_LOG(log, "Failed to parse: {0}. Skipping record.", line);
+  }
 
+  for (auto &KV : symbols)
+    symtab.AddSymbol(std::move(KV.second));
   symtab.CalculateSymbolSizes();
 }

unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp

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(FuncRecord, parse) {
+  EXPECT_EQ(FuncRecord(true, 0x47, 0x7, 0x8, "foo"),
+            FuncRecord::parse("FUNC m 47 7 8 foo"));
+  EXPECT_EQ(FuncRecord(false, 0x47, 0x7, 0x8, "foo"),
+            FuncRecord::parse("FUNC 47 7 8 foo"));
+
+  EXPECT_EQ(llvm::None, FuncRecord::parse("PUBLIC 47 7 8 foo"));
+  EXPECT_EQ(llvm::None, FuncRecord::parse("FUNC 47 7 8"));
+  EXPECT_EQ(llvm::None, FuncRecord::parse("FUNC 47 7"));
+  EXPECT_EQ(llvm::None, FuncRecord::parse("FUNC 47"));
+  EXPECT_EQ(llvm::None, FuncRecord::parse("FUNC m"));
+  EXPECT_EQ(llvm::None, FuncRecord::parse("FUNC"));
+}
+
 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("FUNC 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"));
 }