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

[Jez Ng via Phabricator via llvm-commits]

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:
> int3 wrote:
> > 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)
> Let me be sure I understand:
>   - You coded it this way for sake of consistency with ld64.
>   - A later equal-sized common with smaller alignment will supersede the earlier with greater alignment.
>   - This seems wrong, but whatever. Call it bug-for-bug compatible.
>   - It must be a bug of little consequence, otherwise it would be fixed. Maybe we should fix it anyway and see if someone complains!
yup that's right, I was going for bug4bug compatibility. From what I understand, common symbols are being phased out in modern projects, so it's probably not too important anyway


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86910