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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
