[Lldb-commits] [PATCH] D113163: [LLDB][Breakpad] Create a function for each compilation unit.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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



================
Comment at: lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:245
+    addr_t address = record->Address + base;
+    SectionSP section_sp = list->FindSectionContainingFileAddress(address);
+    AddressRange func_range(section_sp, address - section_sp->GetFileAddress(),
----------------
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.


================
Comment at: lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:337
+  std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
+  if (!(name_type_mask & eFunctionNameTypeMethod))
+    return;
----------------
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.)


================
Comment at: lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:381
   std::vector<Symbol> symbols;
   auto add_symbol = [&](addr_t address, llvm::Optional<addr_t> size,
                         llvm::StringRef name) {
----------------
It'd probably be better to inline this lambda now. You can do that as a follow-up commit. No need for review.


================
Comment at: lldb/test/Shell/Minidump/breakpad-symbols.test:17
 # 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: Symtab, file = /tmp/test/linux-x86_64, num_symbols = 0
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113163/new/

https://reviews.llvm.org/D113163