Use the section sizes to determine symbols sizes in the Symtab instead of just using the following symbol's address

[jasonmolenda]

Summary

The Symtab has an array of symbol file addresses and sizes for address-to-symbol lookups, created in Symtab::InitAddressIndexes. The Symtab has a vector of symbol (m_symbols) and a parallel vector of Range entries (m_file_addr_to_index - the index being an index into m_symbols). Once the m_symbols have been filled in, it makes a pass over them and creates the m_file_addr_to_index entries. Then it does additional passes to fill in the sizes of the Range entries based on the other symbol start addresses.

I'm working on a problem in an environment where the sections of a file are noncontiguous in memory - so we can have a gap of hundreds of megabytes between the sections, and the last symbol in the first section gets a 400MB size. If the unwinder (or one of the fallback unwind methods) happens to come up with an address in that 400MB gap, lldb will try to disassemble that entire section, thinking it is a function.

Before this patch, InitAddressIndexes would create the m_file_addr_to_index array, then set sizes based on the next symbol, then use the last section's end address to set the last symbol(s) in the array.

In this patch, I'm using the smaller of the containing section end OR the next symbol's address to determine the symbol's size. Looking up the section for each symbol would be expensive, so I make a local RangeVector with all of the section address/sizes before I loop over the entries. RangeVector::Sort sorts by start address and then by size so we'll get the smallest containing section for a given lookup.

[clayborg]

Only add sections that don't have children in AddSectionsToRangeMap() and this will be good to go.

source/Symbol/Symtab.cpp

952–975

You want to only add sections that don't have children. On MacOSX we have:

__TEXT [0x1000-0x2000)

__text [0x1000-0x1100)
__textcoal_nt [0x1100-0x1200)

If you have all of these in the mix you will have a non deterministic sort since TEXT and text will compare to the same thing. Object files are also tricky because they have only one LC_SEGMENT with a bunch of sections and the ObjectFileMachO will actually make segments from the segname and sectname and they will have overlapping address ranges. Checking out a .o file from a recent build:

% dwarfdump -R main.o
Sections
Section Name     Segment Name     addr             size             offset   align    reloff   nreloc   flags    reserv1  reserv2  reserv3  size     size %
---------------- ---------------- ---------------- ---------------- -------- -------- -------- -------- -------- -------- -------- -------- ======== ======
__text           __TEXT           0000000000000000 0000000000000165 00000590 00000004 00000b60 00000015 80000400 00000000 00000000 00000000  357       nan%
__debug_info     __DWARF          0000000000000165 00000000000000da 000006f5 00000000 00000c08 00000004 02000000 00000000 00000000 00000000  218       nan%
__debug_abbrev   __DWARF          000000000000023f 000000000000007d 000007cf 00000000 00000000 00000000 02000000 00000000 00000000 00000000  125       nan%
__debug_line     __DWARF          00000000000002bc 0000000000000075 0000084c 00000000 00000c28 00000001 02000000 00000000 00000000 00000000  117       nan%
__debug_str      __DWARF          0000000000000331 000000000000008f 000008c1 00000000 00000000 00000000 02000000 00000000 00000000 00000000  143       nan%
__debug_loc      __DWARF          00000000000003c0 0000000000000000 00000950 00000000 00000000 00000000 02000000 00000000 00000000 00000000    0       nan%
__debug_ranges   __DWARF          00000000000003c0 0000000000000000 00000950 00000000 00000000 00000000 02000000 00000000 00000000 00000000    0       nan%
__cstring        __TEXT           00000000000003c0 000000000000009c 00000950 00000000 00000000 00000000 00000002 00000000 00000000 00000000  156       nan%
__apple_names    __DWARF          000000000000045c 000000000000003c 000009ec 00000000 00000000 00000000 02000000 00000000 00000000 00000000   60       nan%
__apple_objc     __DWARF          0000000000000498 0000000000000024 00000a28 00000000 00000000 00000000 02000000 00000000 00000000 00000000   36       nan%
__apple_namespac __DWARF          00000000000004bc 0000000000000024 00000a4c 00000000 00000000 00000000 02000000 00000000 00000000 00000000   36       nan%
__apple_types    __DWARF          00000000000004e0 0000000000000066 00000a70 00000000 00000000 00000000 02000000 00000000 00000000 00000000  102       nan%
__apple_exttypes __DWARF          0000000000000546 0000000000000024 00000ad6 00000000 00000000 00000000 02000000 00000000 00000000 00000000   36       nan%
__compact_unwind __LD             0000000000000570 0000000000000020 00000b00 00000003 00000c30 00000001 02000000 00000000 00000000 00000000   32       nan%
__eh_frame       __TEXT           0000000000000590 0000000000000040 00000b20 00000003 00000000 00000000 6800000b 00000000 00000000 00000000   64       nan%

Now if we look at what ObjectFileMachO does:

% xcrun lldb main.o
(lldb) target create "main.o"
Current executable set to 'main.o' (x86_64).
(lldb) image dump sections
Dumping sections for 1 modules.
Sections for '/Volumes/work/gclayton/Documents/src/args/main.o' (x86_64):
  SectID     Type             File Address                             File Off.  File Size  Flags      Section Name
  ---------- ---------------- ---------------------------------------  ---------- ---------- ---------- ----------------------------
  0x00000100 container        [0x0000000000000000-0x00000000000005d0)  0x00000590 0x000005d0 0x00000000 main.o.__TEXT
  0x00000001 code             [0x0000000000000000-0x0000000000000165)  0x00000590 0x00000165 0x80000400 main.o.__TEXT.__text
  0x00000008 data-cstr        [0x00000000000003c0-0x000000000000045c)  0x00000950 0x0000009c 0x00000002 main.o.__TEXT.__cstring
  0x0000000f eh-frame         [0x0000000000000590-0x00000000000005d0)  0x00000b20 0x00000040 0x6800000b main.o.__TEXT.__eh_frame
  0x00000200 container        [0x0000000000000165-0x000000000000056a)  0x000006f5 0x00000405 0x00000000 main.o.__DWARF
  0x00000002 dwarf-info       [0x0000000000000165-0x000000000000023f)  0x000006f5 0x000000da 0x02000000 main.o.__DWARF.__debug_info
  0x00000003 dwarf-abbrev     [0x000000000000023f-0x00000000000002bc)  0x000007cf 0x0000007d 0x02000000 main.o.__DWARF.__debug_abbrev
  0x00000004 dwarf-line       [0x00000000000002bc-0x0000000000000331)  0x0000084c 0x00000075 0x02000000 main.o.__DWARF.__debug_line
  0x00000005 dwarf-str        [0x0000000000000331-0x00000000000003c0)  0x000008c1 0x0000008f 0x02000000 main.o.__DWARF.__debug_str
  0x00000006 dwarf-loc                                                 0x00000950 0x00000000 0x02000000 main.o.__DWARF.__debug_loc
  0x00000007 dwarf-ranges                                              0x00000950 0x00000000 0x02000000 main.o.__DWARF.__debug_ranges
  0x00000009 apple-names      [0x000000000000045c-0x0000000000000498)  0x000009ec 0x0000003c 0x02000000 main.o.__DWARF.__apple_names
  0x0000000a apple-objc       [0x0000000000000498-0x00000000000004bc)  0x00000a28 0x00000024 0x02000000 main.o.__DWARF.__apple_objc
  0x0000000b apple-namespaces [0x00000000000004bc-0x00000000000004e0)  0x00000a4c 0x00000024 0x02000000 main.o.__DWARF.__apple_namespac
  0x0000000c apple-types      [0x00000000000004e0-0x0000000000000546)  0x00000a70 0x00000066 0x02000000 main.o.__DWARF.__apple_types
  0x0000000d apple-external-types [0x0000000000000546-0x000000000000056a)  0x00000ad6 0x00000024 0x02000000 main.o.__DWARF.__apple_exttypes
  0x00000300 container        [0x0000000000000570-0x0000000000000590)  0x00000b00 0x00000020 0x00000000 main.o.__LD
  0x0000000e regular          [0x0000000000000570-0x0000000000000590)  0x00000b00 0x00000020 0x02000000 main.o.__LD.__compact_unwind

Note that TEXT and DWARF overlap. That will probably cause problems. So the fix is to only add sections that don't have children.

[jasonmolenda]

Updated the patch to address Greg's feedback.

[clayborg]

Looks good.

[jasonmolenda]

Committed in r266165.

include/lldb/Core/RangeMap.h

         {
             return ((i < m_entries.size()) ? &m_entries[i] : nullptr);
         }
-        
+        Entry *
+        GetMutableEntryAtIndex (size_t i)
+        {
+            return ((i < m_entries.size()) ? &m_entries[i] : nullptr);
+        }
+
         // Clients must ensure that "i" is a valid index prior to calling this function
         const Entry &
         GetEntryRef (size_t i) const

source/Symbol/Symtab.cpp

     addr_t match_offset;
 } SymbolSearchInfo;
 
+// Add all the section file start address & size to the RangeVector,
+// recusively adding any children sections.  
+static void
+AddSectionsToRangeMap (SectionList *sectlist, RangeVector<addr_t, addr_t> &section_ranges)
+{
+    const int num_sections = sectlist->GetNumSections (0);
+    for (int i = 0; i < num_sections; i++)
+    {
+        SectionSP sect_sp = sectlist->GetSectionAtIndex (i);
+        if (sect_sp)
+        {
+            SectionList &child_sectlist = sect_sp->GetChildren();
+
+            // If this section has children, add the children to the RangeVector.
+            // Else add this section to the RangeVector.
+            if (child_sectlist.GetNumSections (0) > 0)
+            {
+                AddSectionsToRangeMap (&child_sectlist, section_ranges);
+            }
+            else
+            {
+                addr_t base_addr = sect_sp->GetFileAddress();
+                size_t size = sect_sp->GetByteSize();
+                RangeVector<addr_t, addr_t>::Entry entry;
+                entry.SetRangeBase (base_addr);
+                entry.SetByteSize (size);
+                section_ranges.Append (entry);
+            }
+        }
+    }
+}
+
 void
 Symtab::InitAddressIndexes()
 {

                 m_file_addr_to_index.Append(entry);
             }
         }
-
         const size_t num_entries = m_file_addr_to_index.GetSize();
         if (num_entries > 0)
         {
             m_file_addr_to_index.Sort();
 
-            // Now our last address range might not have had sizes because there
-            // was no subsequent symbol to calculate the size from. If this is
-            // the case, then calculate the size by capping it at the end of the
-            // section in which the symbol resides
-            lldb::addr_t total_size = 0;
-            const FileRangeToIndexMap::Entry* entry = m_file_addr_to_index.Back();
-            if (entry->GetByteSize() == 0)
+            // Create a RangeVector with the start & size of all the sections for
+            // this objfile.  We'll need to check this for any FileRangeToIndexMap
+            // entries with an uninitialized size, which could potentially be a
+            // large number so reconstituting the weak pointer is busywork when it
+            // is invariant information.
+            SectionList *sectlist = m_objfile->GetSectionList();
+            RangeVector<addr_t, addr_t> section_ranges;
+            if (sectlist)
             {
-                const Address& address = m_symbols[entry->data].GetAddressRef();
-                if (SectionSP section_sp = address.GetSection())
-                    total_size = entry->base + section_sp->GetByteSize() - address.GetOffset();
+                AddSectionsToRangeMap (sectlist, section_ranges);
+                section_ranges.Sort();
             }
 
-            m_file_addr_to_index.CalculateSizesOfZeroByteSizeRanges(total_size);
+            // Iterate through the FileRangeToIndexMap and fill in the size for any
+            // entries that didn't already have a size from the Symbol (e.g. if we
+            // have a plain linker symbol with an address only, instead of debug info
+            // where we get an address and a size and a type, etc.)
+            for (int i = 0; i < num_entries; i++)
+            {
+                FileRangeToIndexMap::Entry *entry = m_file_addr_to_index.GetMutableEntryAtIndex (i);
+                if (entry->GetByteSize() == 0)
+                {
+                    addr_t curr_base_addr = entry->GetRangeBase();
+                    const RangeVector<addr_t, addr_t>::Entry *containing_section =
+                                                              section_ranges.FindEntryThatContains (curr_base_addr);
+
+                    // Use the end of the section as the default max size of the symbol
+                    addr_t sym_size = 0;
+                    if (containing_section)
+                    {
+                        sym_size = containing_section->GetByteSize() - 
+                                        (entry->GetRangeBase() - containing_section->GetRangeBase());
+                    }
+                    
+                    for (int j = i; j < num_entries; j++)
+                    {
+                        FileRangeToIndexMap::Entry *next_entry = m_file_addr_to_index.GetMutableEntryAtIndex (j);
+                        addr_t next_base_addr = next_entry->GetRangeBase();
+                        if (next_base_addr > curr_base_addr)
+                        {
+                            addr_t size_to_next_symbol = next_base_addr - curr_base_addr;
+
+                            // Take the difference between this symbol and the next one as its size,
+                            // if it is less than the size of the section.
+                            if (sym_size == 0 || size_to_next_symbol < sym_size)
+                            {
+                                sym_size = size_to_next_symbol;
+                            }
+                            break;
+                        }
+                    }
+
+                    if (sym_size > 0)
+                    {
+                        entry->SetByteSize (sym_size);
+                        Symbol &symbol = m_symbols[entry->data];
+                        symbol.SetByteSize (sym_size);
+                        symbol.SetSizeIsSynthesized (true);
+                    }
+                }
+            }
+
+            // Sort again in case the range size changes the ordering
+            m_file_addr_to_index.Sort();
         }
     }
 }