[PATCH] D134794: [lld-macho] Do not error out on dead stripped duplicate symbols

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/SymbolTable.cpp:49
+namespace {
+struct DuplicateSymbolReporter {
+  const Symbol *sym;
----------------
How about "DuplicateSymbolDiag" to parallel "UndefinedDiag"? IMO a "reporter" sounds like something that has nontrivial logic in it, rather than a POD type


================
Comment at: lld/MachO/SymbolTable.cpp:56
+};
+std::vector<DuplicateSymbolReporter> dupSymReporter;
+} // namespace
----------------
maybe make this a SmallVector? I think MaskRay has been converting a lot of `std::vector`s to SmallVector in order to save on binary size


================
Comment at: lld/MachO/SymbolTable.cpp:97-103
         std::string message =
             "duplicate symbol: " + toString(*defined) + "\n>>> defined in ";
         if (!src1.empty())
           message += src1 + "\n>>>            ";
         message += toString(defined->getFile()) + "\n>>> defined in ";
         if (!src2.empty())
           message += src2 + "\n>>>            ";
----------------
could consider moving this into reportPendingDuplicateSymbols -- that way builds that have `--dead-strip-duplicates` won't pay the cost of building these strings

that said the cost is probably trivial, so up to you


================
Comment at: lld/docs/MachO/ld64-vs-lld.rst:18
+Dead Stripping Duplicate Symbols
+****************************
+ld64 always dead strips before reporting duplicate symbols. By default, LLD does
----------------
sorry this is my OCD


================
Comment at: lld/docs/MachO/ld64-vs-lld.rst:19
+****************************
+ld64 always dead strips before reporting duplicate symbols. By default, LLD does
+the opposite. The behavior exhibited by ld64 is a violation of ODR, but LLD
----------------
or "strips dead code"


================
Comment at: lld/docs/MachO/ld64-vs-lld.rst:20-21
+ld64 always dead strips before reporting duplicate symbols. By default, LLD does
+the opposite. The behavior exhibited by ld64 is a violation of ODR, but LLD
+maintains compatibility with ld64 by adding the `--dead-strip-duplicates` flag.
+Usage of this flag is discouraged, and this behavior should be fixed in the source.
----------------
super nits:

1. ld64's behavior is not a violation of ODR; rather the build is violating ODR and ld64 is not flagging it
2. we're not maintaining compatibility so much as providing a way for the user to obtain compatible behavior
3. double backticks because rST (I hate how it's subtly different from markdown but oh well)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134794/new/

https://reviews.llvm.org/D134794