[lldb] Do not use LC_FUNCTION_STARTS data to determine symbol size as symbols are created

[bulbazord]

Summary:
This improves the performance of ObjectFileMacho::ParseSymtab by removing eager and expensive work in favor of doing it later in a less-expensive fashion.

Experiment:
My goal was to understand LLDB's startup time.
First, I produced a Debug build of LLDB (no dSYM) and a Release+NoAsserts build of LLDB. The Release build debugged the Debug build as it debugged a small C++ program. I found that ObjectFileMachO::ParseSymtab accounted for somewhere between 1.2 and 1.3 seconds consistently. After applying this change, I consistently measured a reduction of approximately 100ms, putting the time closer to 1.1s and 1.2s on average.

Background:
ObjectFileMachO::ParseSymtab will incrementally create symbols by parsing nlist entries from the symtab section of a MachO binary. As it does this, it eagerly tries to determine the size of symbols (e.g. how long a function is) using LC_FUNCTION_STARTS data (or eh_frame if LC_FUNCTION_STARTS is unavailable). Concretely, this is done by performing a binary search on the function starts array and calculating the distance to the next function or the end of the section (whichever is smaller).

However, this work is unnecessary for 2 reasons:

1. If you have debug symbol entries (i.e. STABs), the size of a function is usually stored right after the function's entry. Performing this work right before parsing the next entry is unnecessary work.
2. Calculating symbol sizes for symbols of size 0 is already performed in Symtab::InitAddressIndexes after all the symbols are added to the Symtab. It also does this more efficiently by walking over a list of symbols sorted by address, so the work to calculate the size per symbol is constant instead of O(log n).

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Alex Langford (bulbazord)

Changes

Summary:
This improves the performance of ObjectFileMacho::ParseSymtab by removing eager and expensive work in favor of doing it later in a less-expensive fashion.

Experiment:
My goal was to understand LLDB's startup time.
First, I produced a Debug build of LLDB (no dSYM) and a Release+NoAsserts build of LLDB. The Release build debugged the Debug build as it debugged a small C++ program. I found that ObjectFileMachO::ParseSymtab accounted for somewhere between 1.2 and 1.3 seconds consistently. After applying this change, I consistently measured a reduction of approximately 100ms, putting the time closer to 1.1s and 1.2s on average.

Background:
ObjectFileMachO::ParseSymtab will incrementally create symbols by parsing nlist entries from the symtab section of a MachO binary. As it does this, it eagerly tries to determine the size of symbols (e.g. how long a function is) using LC_FUNCTION_STARTS data (or eh_frame if LC_FUNCTION_STARTS is unavailable). Concretely, this is done by performing a binary search on the function starts array and calculating the distance to the next function or the end of the section (whichever is smaller).

However, this work is unnecessary for 2 reasons:

1. If you have debug symbol entries (i.e. STABs), the size of a function is usually stored right after the function's entry. Performing this work right before parsing the next entry is unnecessary work.
2. Calculating symbol sizes for symbols of size 0 is already performed in Symtab::InitAddressIndexes after all the symbols are added to the Symtab. It also does this more efficiently by walking over a list of symbols sorted by address, so the work to calculate the size per symbol is constant instead of O(log n).

---

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

1 Files Affected:

• (modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (-45)

diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 2004622e547be9..f53108ff6340aa 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -3768,7 +3768,6 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
 
       SymbolType type = eSymbolTypeInvalid;
       SectionSP symbol_section;
-      lldb::addr_t symbol_byte_size = 0;
       bool add_nlist = true;
       bool is_gsym = false;
       bool demangled_is_synthesized = false;
@@ -4354,47 +4353,6 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
 
       if (symbol_section) {
         const addr_t section_file_addr = symbol_section->GetFileAddress();
-        if (symbol_byte_size == 0 && function_starts_count > 0) {
-          addr_t symbol_lookup_file_addr = nlist.n_value;
-          // Do an exact address match for non-ARM addresses, else get the
-          // closest since the symbol might be a thumb symbol which has an
-          // address with bit zero set.
-          FunctionStarts::Entry *func_start_entry =
-              function_starts.FindEntry(symbol_lookup_file_addr, !is_arm);
-          if (is_arm && func_start_entry) {
-            // Verify that the function start address is the symbol address
-            // (ARM) or the symbol address + 1 (thumb).
-            if (func_start_entry->addr != symbol_lookup_file_addr &&
-                func_start_entry->addr != (symbol_lookup_file_addr + 1)) {
-              // Not the right entry, NULL it out...
-              func_start_entry = nullptr;
-            }
-          }
-          if (func_start_entry) {
-            func_start_entry->data = true;
-
-            addr_t symbol_file_addr = func_start_entry->addr;
-            if (is_arm)
-              symbol_file_addr &= THUMB_ADDRESS_BIT_MASK;
-
-            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;
-              // Be sure the clear the Thumb address bit when we calculate the
-              // size from the current and next address
-              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;
-            }
-          }
-        }
         symbol_value -= section_file_addr;
       }
 
@@ -4501,9 +4459,6 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
       if (nlist.n_desc & N_WEAK_REF)
         sym[sym_idx].SetIsWeak(true);
 
-      if (symbol_byte_size > 0)
-        sym[sym_idx].SetByteSize(symbol_byte_size);
-
       if (demangled_is_synthesized)
         sym[sym_idx].SetDemangledNameIsSynthesized(true);
 

[jasonmolenda]

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?

[clayborg]

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]

We already have bits that know if the size is synthesize in lldb_private::Symbol::m_size_is_synthesized and we have a bit that tracks is the size is valid with lldb_private::Symbol::m_size_is_valid. We could accomplish this size on demand feature by letting clients subclass a special class that does this and is created and registered with the Symtab when it is created. Then any accesses to the Symtab class can be taught to check if a symbol's size is valid or not, and if it isn't check if the size can be calculated, and if so, call through to the new size helper class to complete the size of any symbols. ELF symbol tables would just not register a size completion class, but Mach-O would.

[bulbazord]

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.

[jasonmolenda]

LGTM.

[JDevlieghere]

It looks like this breaks Unwind.trap_frame_sym_ctx on x86_64: https://ci.swift.org/view/all/job/llvm.org/view/LLDB/job/lldb-cmake/5745/testReport/junit/lldb-shell/Unwind/trap_frame_sym_ctx_test/

I'm going to revert this to get the bots green over the weekend.

[royitaqi]

Do we have a sense of whether it's just a broken test, or if the PR broke some functionality? We tested the change on some internal scenarios and it showed good improvement, so we'd like to cherry-pick this patch (or a new patch).

[bulbazord]

Do we have a sense of whether it's just a broken test, or if the PR broke some functionality? We tested the change on some internal scenarios and it showed good improvement, so we'd like to cherry-pick this patch (or a new patch).

I'm not sure yet, I haven't had time to investigate what broke exactly. The test suite passes on arm64 on all the machines I tried it on, so it may be specific to x86_64. I'll try to find some time to investigate this week or the next.