[PATCH] D86910: [lld-macho] Implement and test resolution of common symbols

[Jez Ng via Phabricator via llvm-commits]

int3 marked 3 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/SymbolTable.cpp:85
     if (auto *common = dyn_cast<CommonSymbol>(s)) {
       if (size < common->size)
         return s;
----------------
gkm wrote:
> s/`<`/`<=`/. I assume that `replaceSymbol` implies extra work, so why do that for equal-sized data?
it's to change the alignment. (See also discussion in D86909)


================
Comment at: lld/MachO/SymbolTable.cpp:90-91
     }
+    // Common symbols take priority over all non-Defined symbols, so in case of
+    // a name conflict, we fall through to the replaceSymbol() call below.
   }
----------------
gkm wrote:
> Tangent: I noticed that `DSOHandle` is not part of `SymbolUnion`. Should it be?
>   
oops yeah. I'll add it as part of D86909 since I need to add CommonSymbol there too


================
Comment at: lld/test/MachO/common-symbol-resolution.s:68-72
+#--- order
+## Order is important as we determine the size of a given symbol via the
+## address of the next symbol.
+_foo
+_foo_end
----------------
gkm wrote:
> I don't see a use for this `order` file.
oops...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86910