[PATCH] D140225: [lld-macho] Provide an option to ignore framework-not-found errors coming from LC_LINKER_OPTIONS.

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

> Well, this patch can't make it any harder because it downgrades errors to warnings. In other words, such builds would be already broken - this would "unbreak" them, in exchange for some additional warnings

Sorry I wasn't clear -- I meant that it makes it harder relative to the option where they aren't warnings at all. The point I want to make is that we shouldn't just use "this doesn't break existing builds that use LLD" as a guideline, since we are still trying to grow our userbase. We should try to make it as easy as possible to adopt LLD, and if someone has an existing build that uses ld64 together with `-fatal_warnings` enabled, we (ideally) shouldn't require them to disable it in order to switch.



================
Comment at: lld/MachO/Driver.cpp:428
+
+  if (loadType == LoadType::LCLinkerOption)
+    warn("library not found for -l " + name + ", from LC_LINKER_OPTION in " +
----------------
would be nice to have an explanatory comment here about why we are only emitting a warning

could we also update the comment [[ https://github.com/llvm/llvm-project/blob/main/lld/MachO/Config.h#L172 | here ]]? It probably should now say that LLD will warn on missing libraries (instead of completely disallowing them), but that the `--ignore-auto-link-option` is useful for squelching those warnings entirely.


================
Comment at: lld/MachO/Driver.cpp:429-430
+  if (loadType == LoadType::LCLinkerOption)
+    warn("library not found for -l " + name + ", from LC_LINKER_OPTION in " +
+         refLoc);
+  else
----------------
Error messages should be of the form `toString(file) + ": $message"` if there is a file involved. Not all the messages in LLD-MachO are of this format yet, but I'd like to migrate them all. This seems to be the prevailing format used in LLD-ELF.

also the message should probably say "referenced from LC_LINKER_OPTION"


================
Comment at: lld/test/MachO/lc-linker-option.ll:140
+; RUN: %no-fatal-warnings-lld  -ObjC %t/load-framework-foo.o %t/main.o -o %t/main-no-foo.out  2>&1 | FileCheck %s --check-prefix=WARN-FW
+; RUN: %no-fatal-warnings-lld -dylib  -ObjC %t/load-library-foo.o -o %t/no-lib-foo.out  2>&1 | FileCheck %s --check-prefix=WARN-LIB
+
----------------



================
Comment at: lld/test/MachO/lc-linker-option.ll:144-145
+
+; WARN-FW: ld64.lld: warning: framework not found for -framework Foo, from LC_LINKER_OPTION in {{.+}}load-framework-foo.o
+; WARN-LIB: ld64.lld: warning: library not found for -l foo, from LC_LINKER_OPTION in {{.+}}load-library-foo.o
+
----------------
most tests don't include the `ld64.lld:` prefix

also the message should say `-lfoo`, not `-l foo` (to reflect the actual input, & for consistency with the message used for not-found libraries on the CLI)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140225