[LLDB][Breakpad] Create a function for each compilation unit.

[zequanwu]

Summary

Since every FUNC record (in breakpad) is a compilation unit, creating the
function for the CU allows ResolveSymbolContext to resolve
eSymbolContextFunction.

[labath]

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

405–406

Can you check if we can remove this now?

I originally thought that we can remove this entire function, but I forgot about PUBLIC records -- we don't have functions or compile units for those, so they will have to stay.

lldb/test/Shell/SymbolFile/Breakpad/line-table.test

54

We should have a more explicit test for this. Some interesting things to check might be

• "image lookup -v" command should have a new Function entry in its output
• "image dump symfile" should contain some output about the functions that have been parsed

[zequanwu]

Add tests.

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

405–406

Removing it causes FUNC records not showing up in symtab when doing image dump symtab ... and fails some tests.
The Breakpad doc says (https://chromium.googlesource.com/breakpad/breakpad/+/HEAD/docs/symbol_files.md#records-4):

If a given address is covered by both a PUBLIC record and a FUNC record, the processor uses the FUNC data.

[labath]

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

405–406

It's expected that the tests verifying symtab contents need updating after you remove some things from it. It's also possible some other tests will need minor tweaks (like the one in line-table.test:54) because of small differences in output format.

Whether this is a reasonable change cannot be judged by failing tests alone. You also need to evaluate the overall quality of the debugger output. That will have to be a judgement call, but I'm hoping it won't be a hard one. For example, the change in line-table.test was definitely for the better.

If a given address is covered by both a PUBLIC record and a FUNC record, the processor uses the FUNC data.

And if an address is covered both by a Symtab Symbol, and an SymbolFile Function, lldb will preferentially (in backtraces, for instance) display information from the Function, so I think (hope) that this is going to work exactly as desired.

[zequanwu]

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

405–406

These two commands image lookup -n ... and image show-unwind -n .. also failed to give any information if the name is from FUNC records.

[zequanwu]

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

405–406

Oh, maybe those commands use FindFunctions to lookup for function by name?

[labath]

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

405–406

Yes, that will most likely be it.

If implementing FindFunctions ends up being non-trivial, we can do that (along with the symtab removal) in a separate patch.

[zequanwu]

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

405–406

After implementing FindFunctions and removing this, image lookup -a 0x4000b0 -v in (lldb/test/Shell/SymbolFile/Breakpad/symtab.test) shows:

Function: id = {0x00000001}, name = "f1_func", range = [0x00000000004000b0-0x00000000004000bc)
       Symbol: id = {0x00000000}, range = [0x00000000004000b0-0x00000000004000c0), name="f1"

The symbol name is different from function because symbol name is from symtab and function name is from function in CU.

And image lookup -n f1 and image lookup -n f1_func both give:

Address: symtab.out[0x00000000004000b0] (symtab.out.PT_LOAD[0]..text2 + 0)
Summary: symtab.out`f1_func

Looks like we don't want to remove this part.

[labath]

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

405–406

Function: id = {0x00000001}, name = "f1_func", range = [0x00000000004000b0-0x00000000004000bc)
Symbol: id = {0x00000000}, range = [0x00000000004000b0-0x00000000004000c0), name="f1"

I would say this is actually a good thing, because you get more information (in verbose mode). it's also precisely what would happen in the "regular" case if say elf symtab and dwarf for whatever reason disagreed on the name of the function.

In non-verbose contexts (the Summary field in image lookup, in backtraces, etc.) which just want a single name, lldb should prefer the one from the Function.

So all of this sounds WAI to me. In fact, it probably should have been implemented that way from the start, but I was too lazy to bother with creating Functions.

[zequanwu]

Add FindFunctions.
Remove the loop that adds FUNC to symtab.

[labath]

Sorry for the delay. Just a couple of (hopefully final) requests.

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

245

It would be nice to handle the case when this fails to find any sections. The most likely way for this to happen is a mismatched symbol file.

339

How did you come to pick this? I get that this is not critical functionality for your use case, but it seems rather strange to be claiming that you're looking up methods here. If you don't care what you look up exactly, then maybe you could just skip this check completely, possibly leaving a TODO to implement this in a better way. (The Symtab class has code (heuristics) which tries to classify symbols into functions, methods, etc., but I'm not sure if it actually works with breakpad symbols (them being demangled), nor am I sure that we should replicate that here.)

382

It'd probably be better to inline this lambda now. You can do that as a follow-up commit. No need for review.

lldb/test/Shell/Minidump/breakpad-symbols.test

18–19

It'd be better to modify the test inputs here to replace FUNC with PUBLIC records. This test checks that breakpad symbols files work with minidump cores, and checking for num_symbols = 0 is not very helpful, as that's precisely what would happen if the symbol files did _not_ work.

[zequanwu]

Address comments.

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

339

I just found that name_type_mask always has eFunctionNameTypeMethod.
Removed.

[labath]

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

339

This is probably the only value used by the tests, but one can definitely get other values too, for example through various arguments to the "breakpoint set" command. While a (very) brave soul could in theory try to set breakpoints and debug a live process with breakpad debug info, I think that's unlikely to happen in practice. So the usefulness of this function will probably remain limited to tests.

lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h

public:
   void InitializeObject() override {}
 
   // Compile Unit function calls
 
   lldb::LanguageType ParseLanguage(CompileUnit &comp_unit) override {
     return lldb::eLanguageTypeUnknown;
   }
 
+  lldb::FunctionSP GetOrCreateFunction(CompileUnit &comp_unit);
+
   size_t ParseFunctions(CompileUnit &comp_unit) override;
 
   bool ParseLineTable(CompileUnit &comp_unit) override;
 
   bool ParseDebugMacros(CompileUnit &comp_unit) override { return false; }
 
   bool ParseSupportFiles(CompileUnit &comp_unit,
                          FileSpecList &support_files) override;

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

auto cu_sp = std::make_shared<CompileUnit>(m_objfile_sp->GetModule(),
                                              /*user_data*/ nullptr, spec, index,
                                              eLanguageTypeUnknown,
                                              /*is_optimized*/ eLazyBoolNo);
 
   SetCompileUnitAtIndex(index, cu_sp);
   return cu_sp;
 }
 
+FunctionSP SymbolFileBreakpad::GetOrCreateFunction(CompileUnit &comp_unit) {
+  user_id_t id = comp_unit.GetID();
+  if (FunctionSP func_sp = comp_unit.FindFunctionByUID(id))
+    return func_sp;
+
+  Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_SYMBOLS);
+  FunctionSP func_sp;
+  addr_t base = GetBaseFileAddress();
+  if (base == LLDB_INVALID_ADDRESS) {
+    LLDB_LOG(log, "Unable to fetch the base address of object file. Skipping "
+                  "symtab population.");
+    return func_sp;
+  }
+
+  const SectionList *list = comp_unit.GetModule()->GetSectionList();
+  CompUnitData &data = m_cu_data->GetEntryRef(id).data;
+  LineIterator It(*m_objfile_sp, Record::Func, data.bookmark);
+  assert(Record::classify(*It) == Record::Func);
+
+  if (auto record = FuncRecord::parse(*It)) {
+    Mangled func_name;
+    func_name.SetValue(ConstString(record->Name), false);
+    addr_t address = record->Address + base;
+    SectionSP section_sp = list->FindSectionContainingFileAddress(address);
+    if (section_sp) {
+      AddressRange func_range(
+          section_sp, address - section_sp->GetFileAddress(), record->Size);
+      // Use the CU's id because every CU has only one function inside.
+      func_sp = std::make_shared<Function>(&comp_unit, id, 0, func_name,
+                                           nullptr, func_range);
+      comp_unit.AddFunction(func_sp);
+    }
+  }
+  return func_sp;
+}
+
 size_t SymbolFileBreakpad::ParseFunctions(CompileUnit &comp_unit) {
-  // TODO
-  return 0;
+  std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
+  return GetOrCreateFunction(comp_unit) ? 1 : 0;
 }
 
 bool SymbolFileBreakpad::ParseLineTable(CompileUnit &comp_unit) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   CompUnitData &data = m_cu_data->GetEntryRef(comp_unit.GetID()).data;
 
   if (!data.line_table_up)
     ParseLineTableAndSupportFiles(comp_unit, data);
bool SymbolFileBreakpad::ParseSupportFiles(CompileUnit &comp_unit,
   return true;
 }
 
 uint32_t
 SymbolFileBreakpad::ResolveSymbolContext(const Address &so_addr,
                                          SymbolContextItem resolve_scope,
                                          SymbolContext &sc) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
-  if (!(resolve_scope & (eSymbolContextCompUnit | eSymbolContextLineEntry)))
+  if (!(resolve_scope & (eSymbolContextCompUnit | eSymbolContextLineEntry |
+                         eSymbolContextFunction)))
     return 0;
 
   ParseCUData();
   uint32_t idx =
       m_cu_data->FindEntryIndexThatContains(so_addr.GetFileAddress());
   if (idx == UINT32_MAX)
     return 0;
 
   sc.comp_unit = GetCompileUnitAtIndex(idx).get();
   SymbolContextItem result = eSymbolContextCompUnit;
   if (resolve_scope & eSymbolContextLineEntry) {
     if (sc.comp_unit->GetLineTable()->FindLineEntryByAddress(so_addr,
                                                              sc.line_entry)) {
       result |= eSymbolContextLineEntry;
     }
   }
+  if (resolve_scope & eSymbolContextFunction) {
+    FunctionSP func_sp = GetOrCreateFunction(*sc.comp_unit);
+    if (func_sp) {
+      sc.function = func_sp.get();
+      result |= eSymbolContextFunction;
+    }
+  }
 
   return result;
 }
 
 uint32_t SymbolFileBreakpad::ResolveSymbolContext(
     const SourceLocationSpec &src_location_spec,
     lldb::SymbolContextItem resolve_scope, SymbolContextList &sc_list) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   if (!(resolve_scope & eSymbolContextCompUnit))
     return 0;
 
   uint32_t old_size = sc_list.GetSize();
   for (size_t i = 0, size = GetNumCompileUnits(); i < size; ++i) {
     CompileUnit &cu = *GetCompileUnitAtIndex(i);
     cu.ResolveSymbolContext(src_location_spec, resolve_scope, sc_list);
   }
   return sc_list.GetSize() - old_size;
 }
 
 void SymbolFileBreakpad::FindFunctions(
     ConstString name, const CompilerDeclContext &parent_decl_ctx,
     FunctionNameType name_type_mask, bool include_inlines,
     SymbolContextList &sc_list) {
-  // TODO
+  std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
+  // TODO: Implement this with supported FunctionNameType.
+
+  for (uint32_t i = 0; i < GetNumCompileUnits(); ++i) {
+    CompUnitSP cu_sp = GetCompileUnitAtIndex(i);
+    FunctionSP func_sp = GetOrCreateFunction(*cu_sp);
+    if (func_sp && name == func_sp->GetNameNoArguments()) {
+      SymbolContext sc;
+      sc.comp_unit = cu_sp.get();
+      sc.function = func_sp.get();
+      sc.module_sp = func_sp->CalculateSymbolContextModule();
+      sc_list.Append(sc);
+    }
+  }
 }
 
 void SymbolFileBreakpad::FindFunctions(const RegularExpression &regex,
                                        bool include_inlines,
                                        SymbolContextList &sc_list) {
   // TODO
 }
 
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();
   llvm::DenseSet<addr_t> found_symbol_addresses;
   std::vector<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?",
                name, address);
       return;
     }
     // Keep track of what addresses were already added so far and only add
     // the symbol with the first address.
     if (!found_symbol_addresses.insert(address).second)
       return;
     symbols.emplace_back(
         /*symID*/ 0, Mangled(name), 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);
   };
 
-  for (llvm::StringRef line : lines(Record::Func)) {
-    if (auto record = FuncRecord::parse(line))
-      add_symbol(record->Address, record->Size, record->Name);
-  }
-
   for (llvm::StringRef line : lines(Record::Public)) {
     if (auto record = PublicRecord::parse(line))
       add_symbol(record->Address, llvm::None, record->Name);
     else
       LLDB_LOG(log, "Failed to parse: {0}. Skipping record.", line);
   }
 
   for (Symbol &symbol : symbols)
     symtab.AddSymbol(std::move(symbol));

lldb/test/Shell/Minidump/Inputs/linux-x86_64.syms

 MODULE Linux x86_64 3B285CE327C387C262DB788BF5A4078B0 linux-x86_64
 INFO CODE_ID E35C283BC327C28762DB788BF5A4078BE2351448
-FUNC 3d0 18 0 crash()
-FUNC 3f0 10 0 _start
+PUBLIC 3d0 0 crash()
+PUBLIC 3f0 0 _start

lldb/test/Shell/Minidump/breakpad-symbols.test

 
 image list
 # CHECK-LABEL: image list
 # CHECK: E35C283B-C327-C287-62DB-788BF5A4078B-E2351448 0x0000000000400000 /tmp/test/linux-x86_64
 
 image dump symtab /tmp/test/linux-x86_64
 # CHECK-LABEL: image dump symtab /tmp/test/linux-x86_64
 # CHECK: Symtab, file = /tmp/test/linux-x86_64, num_symbols = 2:
-# CHECK: [    0]      0   X Code            0x00000000004003d0 0x00000000004003d0 0x0000000000000018 0x00000000 crash()
-# CHECK: [    1]      0   X Code            0x00000000004003f0 0x00000000004003f0 0x0000000000000010 0x00000000 _start
+# CHECK: [    0]      0   X Code            0x00000000004003d0 0x00000000004003d0 0x0000000000000020 0x00000000 crash()
+# CHECK: [    1]      0   X Code            0x00000000004003f0 0x00000000004003f0 0x0000000000000c10 0x00000000 _start
 
 image lookup -a 0x4003f3
 # CHECK-LABEL: image lookup -a 0x4003f3
 # CHECK: Summary: linux-x86_64`_start + 3
 
 image dump objfile /tmp/test/linux-x86_64
 # CHECK-LABEL: image dump objfile /tmp/test/linux-x86_64
 # CHECK: Placeholder object file for /tmp/test/linux-x86_64 loaded at [0x400000-0x401000)

lldb/test/Shell/SymbolFile/Breakpad/line-table.test

 # CHECK-LABEL: Line table for /tmp/d.c
 # CHECK-NEXT: 0x00000000004000e0: /tmp/d.c:2
 # CHECK-NEXT: 0x00000000004000e2: 
 # CHECK-EMPTY: 
 
 image lookup -a 0x4000b2 -v
 # CHECK-LABEL: image lookup -a 0x4000b2 -v
 # CHECK: Summary: line-table.out`func + 2
+# CHECK: Function: id = {0x00000000}, name = "func", range = [0x00000000004000b0-0x00000000004000c0)
+
+image dump symfile
+# CHECK-LABEL: Compile units:
+# CHECK-NEXT:  CompileUnit{0x00000000}, language = "<not loaded>", file = '/tmp/a.c'
+# CHECK-NEXT:   Function{0x00000000}, demangled = func, type_uid = 0x00000000
+# CHECK:  CompileUnit{0x00000001}, language = "<not loaded>", file = '/tmp/c.c'
+# CHECK-NEXT:  CompileUnit{0x00000002}, language = "<not loaded>", file = '/tmp/d.c'
+# CHECK-NEXT:  CompileUnit{0x00000003}, language = "<not loaded>", file = '/tmp/d.c'
 
 breakpoint set -f c.c -l 2
 # CHECK-LABEL: breakpoint set -f c.c -l 2
-# CHECK: Breakpoint 1: where = line-table.out`func + 2, address = 0x00000000004000b2
+# CHECK: Breakpoint 1: where = line-table.out`func + 2 at c.c:2, address = 0x00000000004000b2

lldb/test/Shell/SymbolFile/Breakpad/symtab.test

 # RUN: yaml2obj %S/Inputs/basic-elf.yaml -o %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 = 5:
+# CHECK: Symtab, file = {{.*}}symtab.out, num_symbols = 4:
 # CHECK: Index   UserID DSX Type            File Address/Value Load Address       Size               Flags      Name
 # CHECK: [    0]      0  SX Code            0x0000000000400000                    0x00000000000000b0 0x00000000 ___lldb_unnamed_symbol{{[0-9]*}}
-# CHECK: [    1]      0   X Code            0x00000000004000b0                    0x000000000000000c 0x00000000 f1_func
-# CHECK: [    2]      0   X Code            0x00000000004000a0                    0x000000000000000d 0x00000000 func_only
-# CHECK: [    3]      0   X Code            0x00000000004000c0                    0x0000000000000010 0x00000000 f2
-# CHECK: [    4]      0   X Code            0x00000000004000d0                    0x0000000000000022 0x00000000 _start
+# CHECK: [    1]      0   X Code            0x00000000004000b0                    0x0000000000000010 0x00000000 f1
+# CHECK: [    2]      0   X Code            0x00000000004000c0                    0x0000000000000010 0x00000000 f2
+# CHECK: [    3]      0   X Code            0x00000000004000d0                    0x0000000000000022 0x00000000 _start
 
 # 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-0x00000000004000bc), name="f1_func"
+# CHECK: Function: id = {0x00000001}, name = "f1_func", range = [0x00000000004000b0-0x00000000004000bc)
+# CHECK: Symbol: id = {0x00000000}, range = [0x00000000004000b0-0x00000000004000c0), name="f1"
 
 # 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