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

[Pavel Labath via Phabricator via lldb-commits]

labath 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);
   }
 
----------------
zequanwu wrote:
> 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.
>> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113163