zequanwu added inline comments.
================
Comment at:
lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:388-392
for (llvm::StringRef line : lines(Record::Func)) {
if (auto record = FuncRecord::parse(line))
add_symbol(record->Address, record->Size, record->Name);
}
----------------
labath wrote:
> zequanwu wrote:
> > zequanwu wrote:
> > > labath wrote:
> > > > zequanwu wrote:
> > > > > labath wrote:
> > > > > > 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.
> > > > > 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.
> > > > >
> > > > 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.
> > > These two commands `image lookup -n ...` and `image show-unwind -n ..`
> > > also failed to give any information if the name is from FUNC records.
> > Oh, maybe those commands use `FindFunctions` to lookup for function by name?
> 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.
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.
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
