[Lldb-commits] [lldb] 5b354d2 - [lldb] Make symbol list output from `image dump symtab` not depend on internal ordering of DenseMap

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-09-03T10:27:19+02:00
New Revision: 5b354d204d0952a6dd39e41fb41b51414bff5f0b

URL: https://github.com/llvm/llvm-project/commit/5b354d204d0952a6dd39e41fb41b51414bff5f0b
DIFF: https://github.com/llvm/llvm-project/commit/5b354d204d0952a6dd39e41fb41b51414bff5f0b.diff

LOG: [lldb] Make symbol list output from `image dump symtab` not depend on internal ordering of DenseMap

`image dump symtab` seems to output the symbols in whatever order they appear in
the DenseMap that is used to filter out symbols with non-unique addresses. As
DenseMap is a hash map this order can change at any time so the output of this
command is pretty unstable. This also causes the `Breakpad/symtab.test` to fail
with enabled reverse iteration (which reverses the DenseMap order to find issues
like this).

This patch makes the DenseMap a std::vector and uses a separate DenseSet to do
the address filtering. The output order is now dependent on the order in which
the symbols are read (which should be deterministic). It might also avoid a bit
of work as all the work for creating the Symbol constructor parameters is only
done when we can actually emplace a new Symbol.

Reviewed By: labath

Differential Revision: https://reviews.llvm.org/D87036

Added: 
    

Modified: 
    lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
    lldb/test/Shell/SymbolFile/Breakpad/symtab.test

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
index eeec7296747e..07e5b284eab8 100644
--- a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
+++ b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
@@ -326,7 +326,8 @@ void SymbolFileBreakpad::AddSymbols(Symtab &symtab) {
   }
 
   const SectionList &list = *module.GetSectionList();
-  llvm::DenseMap<addr_t, Symbol> symbols;
+  llvm::DenseSet<addr_t> found_symbol_addresses;
+  std::vector<Symbol> symbols;
   auto add_symbol = [&](addr_t address, llvm::Optional<addr_t> size,
                         llvm::StringRef name) {
     address += base;
@@ -338,8 +339,12 @@ void SymbolFileBreakpad::AddSymbols(Symtab &symtab) {
                name, address);
       return;
     }
-    symbols.try_emplace(
-        address, /*symID*/ 0, Mangled(name), eSymbolTypeCode,
+    // Keep track of what addresses were already added so far and only add
+    // the symbol with the first address.
+    if (!found_symbol_addresses.insert(address).second)
+      return;
+    symbols.emplace_back(
+        /*symID*/ 0, Mangled(name), eSymbolTypeCode,
         /*is_global*/ true, /*is_debug*/ false,
         /*is_trampoline*/ false, /*is_artificial*/ false,
         AddressRange(section_sp, address - section_sp->GetFileAddress(),
@@ -359,8 +364,8 @@ void SymbolFileBreakpad::AddSymbols(Symtab &symtab) {
       LLDB_LOG(log, "Failed to parse: {0}. Skipping record.", line);
   }
 
-  for (auto &KV : symbols)
-    symtab.AddSymbol(std::move(KV.second));
+  for (Symbol &symbol : symbols)
+    symtab.AddSymbol(std::move(symbol));
   symtab.CalculateSymbolSizes();
 }
 

diff  --git a/lldb/test/Shell/SymbolFile/Breakpad/symtab.test b/lldb/test/Shell/SymbolFile/Breakpad/symtab.test
index a02d94c30aa3..1eb03fa43deb 100644
--- a/lldb/test/Shell/SymbolFile/Breakpad/symtab.test
+++ b/lldb/test/Shell/SymbolFile/Breakpad/symtab.test
@@ -6,10 +6,10 @@
 # CHECK: Symtab, file = {{.*}}symtab.out, num_symbols = 5:
 # CHECK: Index   UserID DSX Type            File Address/Value Load Address       Size               Flags      Name
 # CHECK: [    0]      0  SX Code            0x0000000000400000                    0x00000000000000b0 0x00000000 ___lldb_unnamed_symbol{{[0-9]*}}$$symtab.out
-# CHECK: [    1]      0   X Code            0x00000000004000c0                    0x0000000000000010 0x00000000 f2
-# CHECK: [    2]      0   X Code            0x00000000004000d0                    0x0000000000000022 0x00000000 _start
-# CHECK: [    3]      0   X Code            0x00000000004000a0                    0x000000000000000d 0x00000000 func_only
-# CHECK: [    4]      0   X Code            0x00000000004000b0                    0x000000000000000c 0x00000000 f1_func
+# CHECK: [    1]      0   X Code            0x00000000004000b0                    0x000000000000000c 0x00000000 f1_func
+# CHECK: [    2]      0   X Code            0x00000000004000a0                    0x000000000000000d 0x00000000 func_only
+# CHECK: [    3]      0   X Code            0x00000000004000c0                    0x0000000000000010 0x00000000 f2
+# CHECK: [    4]      0   X Code            0x00000000004000d0                    0x0000000000000022 0x00000000 _start
 
 # CHECK-LABEL: (lldb) image lookup -a 0x4000b0 -v
 # CHECK: Address: symtab.out[0x00000000004000b0] (symtab.out.PT_LOAD[0]..text2 + 0)