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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:398
+      symbols.push_back(symtab->addDylib(saver.save(symbol->getName()),
+                                         umbrella, /*isWeakDef*/ false));
   // TODO(compnerd) properly represent the hierarchy of the documents as it is
----------------
Super super nit: convention is to format the comments as `/*isWeakDef=*/false`


================
Comment at: lld/MachO/Symbols.h:53
+  // This bool is only meaningful for Defineds and DylibSymbols.
+  const bool isWeakDef;
+
----------------
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.)


================
Comment at: lld/MachO/Symbols.h:77
+  Undefined(StringRefZ name)
+      : Symbol(UndefinedKind, name, /*isWeakDef*/ true) {}
 
----------------
Not that it holds any meaning for undefined symbols, but how come we're setting isWeakDef to true?


================
Comment at: lld/MachO/Symbols.h:97
   LazySymbol(ArchiveFile *file, const llvm::object::Archive::Symbol &sym)
-      : Symbol(LazyKind, sym.getName()), file(file), sym(sym) {}
+      : Symbol(LazyKind, sym.getName(), false), file(file), sym(sym) {}
 
----------------
Nit: missing the comment for the boolean argument.


================
Comment at: lld/test/MachO/weak-definition-direct-fetch.s:30
+# RUN: rm -f %t/weakfoo.a
+# RUN: llvm-ar rcs %t/weakfoo.a %t/weakfoo.o
+
----------------
LLD will read archives of all formats fine, but technically we should pass `--format=darwin` to get a Darwin archive.


================
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
----------------
Was the order of `-lweakfoo` and `-lfoo` meant to be switched in this line?


================
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
----------------
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.)


================
Comment at: lld/test/MachO/weak-definition-direct-fetch.s:69
+# RUN: llvm-objdump --macho --lazy-bind --syms %t/weak-ar-nonweak-dylib | FileCheck %s --check-prefix=PREFER-WEAK-OBJECT
+# 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-NONWEAK-DYLIB
----------------
Huh, interesting.


================
Comment at: lld/test/MachO/weak-definition-indirect-fetch.s:6
+## that are not referenced directly, but which are still loaded due to some
+## other symbol in the archive being referenced.
+##
----------------
"some other symbol in the archive **member** being referenced" would be more precise.


================
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
----------------
Is it necessary to have the `callq _foo`?


================
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
----------------
Do we need this test for archives as well?


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83532