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

Move to BreakpadLine.cpp/.h?

source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h

20–23

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

26

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

187

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

233–240

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);

246

If we make a Line class this code would become:

parse(bp_line);

[clayborg]

source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

232

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)) {
}

240

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

26

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.

232

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/ObjectFileBreakpad.h

 
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "llvm/ADT/Triple.h"
 
 namespace lldb_private {
 namespace breakpad {
 
+enum class Token { Unknown, Module, Info, File, Func, Public, Stack };
+
+Token toToken(llvm::StringRef str);
+llvm::StringRef toString(Token t);
+
 class ObjectFileBreakpad : public ObjectFile {
 public:
   //------------------------------------------------------------------
   // Static Functions
   //------------------------------------------------------------------
   static void Initialize();
   static void Terminate();
 

source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp

 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) {
+Token breakpad::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) {
+llvm::StringRef breakpad::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";
   }

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, Token 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) {
+                                                Token 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"))) {
-    // PUBLIC [m] address param_size name
-    // skip PUBLIC keyword
-    line = getToken(line).second;
+  llvm::DenseMap<addr_t, Symbol> symbols;
+
+  auto parse = [&](llvm::StringRef line, bool is_func) {
+    // [m] address {size} param_size name
+    // {size} is present in FUNC records.
     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))
-      continue;
+      return;
     address += base;
 
+    addr_t size = 0;
+    bool size_is_valid = false;
+    if (is_func) {
+      std::tie(token, line) = getToken(line);
+      if (!to_integer(token, size, 16))
+        return;
+      size_is_valid = true;
+    }
+
     // skip param_size
     line = getToken(line).second;
 
     llvm::StringRef name = line.trim();
 
     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?",
                name, address);
-      continue;
+      return;
     }
 
-    symtab.AddSymbol(Symbol(
-        /*symID*/ 0, Mangled(name, /*is_mangled*/ false), eSymbolTypeCode,
+    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(), 0),
-        /*size_is_valid*/ 0, /*contains_linker_annotations*/ false,
-        /*flags*/ 0));
+        AddressRange(section_sp, address - section_sp->GetFileAddress(), size),
+        size_is_valid, /*contains_linker_annotations*/ false,
+        /*flags*/ 0);
+  };
+  for (llvm::StringRef line: lines(*m_obj_file, Token::Func)) {
+    // Here we can get either FUNC records (starting with FUNC), or line records
+    // (starting with a hex number).
+    llvm::StringRef token_str;
+    std::tie(token_str, line) = getToken(line);
+    if (toToken(token_str) != Token::Func)
+      continue; // Skip line records.
+
+    parse(line, true);
   }
 
-  // TODO: Process FUNC records as well.
+  for (llvm::StringRef line : lines(*m_obj_file, Token::Public)) {
+    // PUBLIC [m] address param_size name
+    // skip PUBLIC keyword
+    parse(getToken(line).second, false);
+  }
 
+  for (auto &KV : symbols)
+    symtab.AddSymbol(std::move(KV.second));
   symtab.CalculateSymbolSizes();
 }