[PATCH] D83532: [lld-macho] Partial support for weak definitions

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Symbols.h:53
+  // This bool is only meaningful for Defineds and DylibSymbols.
+  const bool isWeakDef;
+
----------------
smeenai wrote:
> How would you feel about making the field private and adding an accessor which asserts it's only called on the right kind of symbol? (Ordinarily I would have suggested a virtual method with subclass overrides, but it looks like lld keeps `Symbol` free of virtual methods, and with an assert we wouldn't have any overhead in no-assert builds.)
I actually add virtual methods to Symbol in D83603, so I can do it here too


================
Comment at: lld/MachO/Symbols.h:77
+  Undefined(StringRefZ name)
+      : Symbol(UndefinedKind, name, /*isWeakDef*/ true) {}
 
----------------
smeenai wrote:
> Not that it holds any meaning for undefined symbols, but how come we're setting isWeakDef to true?
no reason really, Undefined symbols are just weak compared to everything else, so I figured why not :p but yeah not having redundant members will make more sense


================
Comment at: lld/test/MachO/weak-definition-direct-fetch.s:47
+# RUN: llvm-objdump --macho --lazy-bind --syms %t/weak-nonweak-dylibs | FileCheck %s --check-prefix=PREFER-NONWEAK-DYLIB
+# RUN: lld -flavor darwinnew -L%S/Inputs/MacOSX.sdk/usr/lib -lSystem -o %t/nonweak-weak-dylibs -Z -L%t -lweakfoo -lfoo %t/test.o
+# RUN: llvm-objdump --macho --lazy-bind --syms %t/nonweak-weak-dylibs | FileCheck %s --check-prefix=PREFER-NONWEAK-DYLIB
----------------
smeenai wrote:
> Was the order of `-lweakfoo` and `-lfoo` meant to be switched in this line?
oops yeah


================
Comment at: lld/test/MachO/weak-definition-direct-fetch.s:64
+# RUN: llvm-objdump --macho --lazy-bind --syms %t/weak-dylib-weak-ar | FileCheck %s --check-prefix=PREFER-WEAK-OBJECT
+# RUN: lld -flavor darwinnew -L%S/Inputs/MacOSX.sdk/usr/lib -lSystem -o %t/weak-ar-weak-dylib -Z -L%t %t/weakfoo.a -lweakfoo %t/test.o
+# RUN: llvm-objdump --macho --lazy-bind --syms %t/weak-ar-weak-dylib | FileCheck %s --check-prefix=PREFER-WEAK-OBJECT
----------------
smeenai wrote:
> I assume the intended behavior is that whichever comes first on the command line (the archive or the object) wins out. Is it worth testing that explicitly? (As in, by creating a different object file that's part of the archive, perhaps with the symbol in a different section.)
I'm confused. Was your comment intended for this line? We're only testing archive vs dylib here...

But in any case, for object vs archive, the object always wins out. Lines 82-85 demonstrate that.


================
Comment at: lld/test/MachO/weak-definition-indirect-fetch.s:13
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t/test.o
+# RUN: echo ".globl _foo, _bar; .section __TEXT,nonweak; _bar: callq _foo; _foo:" | llvm-mc -filetype=obj -triple=x86_64-apple-darwin -o %t/foo.o
+# RUN: echo ".globl _foo, _baz; .weak_definition _foo; .section __TEXT,weak; _baz: callq _foo; _foo:" | llvm-mc -filetype=obj -triple=x86_64-apple-darwin -o %t/weakfoo.o
----------------
smeenai wrote:
> Is it necessary to have the `callq _foo`?
nope, will remove


================
Comment at: lld/test/MachO/weak-definition-order.s:12
+
+# RUN: lld -flavor darwinnew -L%S/Inputs/MacOSX.sdk/usr/lib -lSystem -o %t/obj12 -Z -L%t %t/weak1.o %t/weak2.o %t/test.o
+# RUN: llvm-objdump --syms %t/obj12 | FileCheck %s --check-prefix=WEAK1
----------------
smeenai wrote:
> Do we need this test for archives as well?
from the other tests it's pretty clear that the weak flag has no impact on archive symbols, so I figured it wasn't really necessary to test them here...


================
Comment at: lld/test/MachO/weak-definition-over-dysym.s:26
+# RUN: lld -flavor darwinnew -L%S/Inputs/MacOSX.sdk/usr/lib -lSystem -o %t/nonweak-dylib-weak-ar -Z -L%t -lfoo %t/weakfoo.a %t/test.o
+# RUN: llvm-objdump --macho --lazy-bind --syms %t/nonweak-dylib-weak-ar | FileCheck %s --check-prefix=PREFER-WEAK-OBJECT
+# RUN: lld -flavor darwinnew -L%S/Inputs/MacOSX.sdk/usr/lib -lSystem -o %t/weak-ar-nonweak-dylib -Z -L%t %t/weakfoo.a -lfoo %t/test.o
----------------
smeenai wrote:
> Doesn't this contradict the test in weak-definition-direct-fetch that I commented "Huh, interesting" on?
> 
> Is weak-definition-direct-fetch not testing all these cases already?
The difference is that this isn't a "direct fetch". Pulling in `_bar` causes `_foo` to be pulled in too and take priority over the dylib symbol.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83532