[Lldb-commits] [lldb] [lldb] Do not use LC_FUNCTION_STARTS data to determine symbol size as symbols are created (PR #106791)

[Alex Langford via lldb-commits]

bulbazord wrote:

> OK, I see in `Symtab::InitAddressIndexes` we go through the symbol table and calculate the sizes of any entries that don't have a size based on the next symbol, or the end of the section.
> 
> A little further in ObjectFileMachO::ParseSymtab when we add any LC_FUNCTION_STARTS entries to the symbol table, we calculate the size of the symbols based on the symbols we have parsed so far,
> 
> ```
>             uint32_t symbol_byte_size = 0;
>             if (symbol_section) {
>               const addr_t section_file_addr = symbol_section->GetFileAddress();
>               const FunctionStarts::Entry *next_func_start_entry =
>                   function_starts.FindNextEntry(func_start_entry);
>               const addr_t section_end_file_addr =
>                   section_file_addr + symbol_section->GetByteSize();
>               if (next_func_start_entry) {
>                 addr_t next_symbol_file_addr = next_func_start_entry->addr;
>                 if (is_arm)   
>                   next_symbol_file_addr &= THUMB_ADDRESS_BIT_MASK;
>                 symbol_byte_size = std::min<lldb::addr_t>(
>                     next_symbol_file_addr - symbol_file_addr,
>                     section_end_file_addr - symbol_file_addr);
>               } else {            
>                 symbol_byte_size = section_end_file_addr - symbol_file_addr;
>               }             
> ```
> 
> and this should probably set the size to 0 as well, leaving it to `Symtab::InitAddressIndexes` to do the same calculation. Does this sound right to you?

@jasonmolenda In this instance it shouldn't matter tremendously where we do the work, but doing it in `Symtab::InitAddressIndexes` simplifies the code a lot. Great catch! :)

> All symbol sizes are zero for mach-o symbols unless they come from the debug map as there is no size field in nlist entries. This PR seems to just not do the work of setting the symbol size at all. Does something try to set the symbol sizes later? Does the code still try to create symbols from the LC_FUNCTION_STARTS later in this function?
> 
> I worry about not setting the function sizes for symbols as we use this information for back tracing when we lookup a symbol by address. Now if we don't have debug symbols, we won't get any sizes for symbols and I am not sure how much that affects the unwinder. I believe it is pretty important. @jasonmolenda let me know if you agree?
> 
> Can we possibly find a way to calculate a symbol size on demand when we access the address range from a `lldb_private::Symbol`? The idea would be we could mark certain symbols as `can_synthesize_size = true` and then any accessor to a symbol's address range would need to see if this is true and call some function that could calculate this on the fly on demand.

@clayborg Yes, the symtab reaches the same state with or without this patch. This is purely for performance. The code I've deleted is an O(log n) operation for each symbol processed when the same work is being done later in O(1) time. Accessing the symbol's size on-demand would be a much more involved change and I'm not convinced it would be faster in the end.

https://github.com/llvm/llvm-project/pull/106791