[Lldb-commits] [PATCH] D16186: Unconditionally accept symbol sizes from elf

[Tamas Berghammer via lldb-commits]

tberghammer added inline comments.

================
Comment at: source/Symbol/Symtab.cpp:974-978
@@ -973,5 +973,7 @@
                         Symbol &symbol = m_symbols[entry.data];
-
-                        symbol.SetByteSize(end_section_file_addr - symbol_file_addr);
-                        symbol.SetSizeIsSynthesized(true);
+                        if (!symbol.GetByteSizeIsValid())
+                        {
+                            symbol.SetByteSize(end_section_file_addr - symbol_file_addr);
+                            symbol.SetSizeIsSynthesized(true);
+                        }
                     }
----------------
clayborg wrote:
> You can remove this if statement right? Symbol byte size will always be valid no?
I case of ELF all synbol size will be valid but I think this code is used for mach-o as well where they won't. The condition is kind of saying "if (mach-o)"

================
Comment at: source/Symbol/Symtab.cpp:1070-1071
@@ +1069,4 @@
+        Symbol* symbol = SymbolAtIndex(entry->data);
+        if (symbol->ContainsFileAddress(file_addr))
+            return symbol;
+    }
----------------
clayborg wrote:
> Why do we need to check this? Won't "m_file_addr_to_index.FindEntryThatContains(file_addr);" already check this for us?
The m_file_addr_to_index initialized with extending all 0 byte entry until the next entry so it can handle mach-o as well and because of this an entry can cover a larger address range then it's symbol.

An alternative implementation would be to sort the symbols based on address. Then calculate the size for all of them if they are not valid (mach-o) and finally generate the entries based on that. That way we can get rid of this condition but Symtab::InitAddressIndexes would become more complicated and most likely a bit less efficient.


http://reviews.llvm.org/D16186