[PATCH] D144589: [lld-macho] Don't emit symbols into the symtab multiple times

[Leonard Grey via Phabricator via llvm-commits]

lgrey created this revision.
lgrey added reviewers: int3, lld-macho.
lgrey added a project: lld-macho.
Herald added a reviewer: JDevlieghere.
Herald added a subscriber: pengfei.
Herald added a project: All.
lgrey requested review of this revision.
Herald added a project: LLVM.

https://reviews.llvm.org/D139069 made the entry in `symbols` for private aliases point directly to the aliasee. This caused a new symbol table entry to be created for the aliasee each time it appeared in `symbols` which

- Increases the number of symbols emitted (since the private aliases wouldn't have been emitted as their own symbol)
- Confuses `dsymutil`, which says stuff like:

  <snip>
  warning: (x86_64)  failed to insert symbol '_.str.422' in the debug map.
  warning: (x86_64)  failed to insert symbol '_.str.423' in the debug map.
  warning: (x86_64)  failed to insert symbol '_.str.424' in the debug map.
  warning: (x86_64)  failed to insert symbol '_.str.425' in the debug map.
  warning: (x86_64)  failed to insert symbol '_.str.426' in the debug map.
  <snip>

This change adds all symbols that have already been added to a set and checks against it to decide whether to add a symbol or not, which has a little bit of overhead (Chromium's `base_unittests` in ASAN configuration; as one might assume the difference is dwarfed in LTO builds). Not sure if this is significant enough to drop this approach and try something more complex.

  +------------------------------------------------------------+
  |              x                           +             +   |
  |x             *             x             +             +   |
  |    |_________A_________|                                   |
  |                         |________________A________________||
  +------------------------------------------------------------+
      N           Min           Max        Median           Avg        Stddev
  x   5          1.28           1.3          1.29          1.29  0.0070710678
  +   5          1.29          1.32          1.31          1.31   0.012247449
  Difference at 95.0% confidence
  	0.02 +/- 0.0145844
  	1.55039% +/- 1.13058%
  	(Student's t, pooled s = 0.01)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144589

Files:
  lld/MachO/SyntheticSections.cpp
  lld/test/MachO/private-label-alias.s


Index: lld/test/MachO/private-label-alias.s
===================================================================
--- lld/test/MachO/private-label-alias.s
+++ lld/test/MachO/private-label-alias.s
@@ -26,9 +26,11 @@
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/weak-then-private.s -o %t/weak-then-private.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/private-then-weak.s -o %t/private-then-weak.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/no-dead-strip.s -o %t/no-dead-strip.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/all-local.s -o %t/all-local.o
 # RUN: %lld -dylib %t/weak-then-private.o %t/private-then-weak.o -o %t/test1
 # RUN: %lld -dylib %t/private-then-weak.o %t/weak-then-private.o -o %t/test2
 # RUN: %lld -dead_strip %t/no-dead-strip.o -o %t/no-dead-strip
+# RUN: %lld -dylib %t/all-local.o -o %t/all-local
 
 # RUN: llvm-objdump --macho --syms --section="__DATA,__data" --weak-bind %t/test1 | FileCheck %s
 # RUN: llvm-objdump --macho --syms --section="__DATA,__data" --weak-bind %t/test2 | FileCheck %s
@@ -59,6 +61,13 @@
 # RUN:   --check-prefix=NO-DEAD-STRIP
 # NO-DEAD-STRIP: __data        00000010
 
+## Ensure that a private alias to a local label doesn't write two identical
+## entries to the symbol table.
+# RUN: llvm-objdump --macho --syms %t/all-local | FileCheck %s --check-prefix=NO-DUPES
+# NO-DUPES: SYMBOL TABLE:
+# NO-DUPES-NEXT: 00000000000002c8 l     F __TEXT,__text _label
+# NO-DUPES-NEXT: 00000000000002d0 l     F __TEXT,__text _ref
+
 #--- weak-then-private.s
 .globl _weak
 .weak_definition _weak
@@ -103,3 +112,15 @@
   .quad 0x123
 
 .subsections_via_symbols
+
+#--- all-local.s
+.text
+
+l_ignored:
+_label:
+  .quad 0x123
+
+_ref:
+  .quad l_ignored
+
+.subsections_via_symbols
Index: lld/MachO/SyntheticSections.cpp
===================================================================
--- lld/MachO/SyntheticSections.cpp
+++ lld/MachO/SyntheticSections.cpp
@@ -24,6 +24,7 @@
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/Parallel.h"
 #include "llvm/Support/Path.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/Support/xxhash.h"
 
 #if defined(__APPLE__)
@@ -1208,7 +1209,10 @@
 }
 
 void SymtabSection::finalizeContents() {
+  SmallPtrSet<const Symbol*, 8> seenSymbols;
   auto addSymbol = [&](std::vector<SymtabEntry> &symbols, Symbol *sym) {
+    if (seenSymbols.insert(sym).second != true)
+      return;
     uint32_t strx = stringTableSection.addString(sym->getName());
     symbols.push_back({sym, strx});
   };


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D144589.499631.patch
Type: text/x-patch
Size: 2547 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230222/f3d1e936/attachment.bin>