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

[oontvoo]

Summary

This makes it a bit easier for our current users to move to LLD and avoid these framework-not-found errors.

I know there is an existing option to list the framework(s) to be ignored but in our user-case it is not practical to do that.

[oontvoo]

@keith Since you initially implemented these LC_LINKER_OPTION support, perhaps you could help reviewing this?
Thx

[keith]

I wonder if at this point we should just implement LD64's behavior with an off-by-default flag to be more strict? Specifically they ignore these failures and collect the missing ones and only print about them in the case the link fails

lld/MachO/Options.td

99 ↗

(On Diff #483570)

nit: accidental whitespace

104 ↗

(On Diff #483570)

for these options we've been using dashes instead of underscores

[oontvoo]

In D140225#4001991, @keith wrote:

I wonder if at this point we should just implement LD64's behavior with an off-by-default flag to be more strict? Specifically they ignore these failures and collect the missing ones and only print about them in the case the link fails

Honestly, I find LD64's behaviour a bit sketchy ... How would people feel about making these warnings (instead of errors)? (and of course, get rid of this new flag)
But no strong preference - either would work :)

[oontvoo]

From offline discussions, we have a few options: (in no particular order)

• make the new ignore_auto_link_errors behave like Apple's LD64, which is that it ignore the errors IFF the link succeeded
• make framework-not-found errors from LC_LINKER_OPTION warnings instead of errors
• make existing --ignore_linker_option understand a new special value * to mean "ignore not found errors IFF the link succeeded"

@int3 @thakis /et al : any thought/preference?

[keith]

Here's the ld64 mirroring option: https://reviews.llvm.org/D140244

[thakis]

Making them warnings seems like a good compromise to me, since people can then use -fatal_warnings if they want to turn this into errors and we don't need a new flag. And if we do want a flag, we could add one to disable this particular warning -- warning-disabling flags have a ton of precedent, so that seems like a nice and cohesive approach to me.

[keith]

If we make them warnings, you would actually be able to disable the warning today by passing --ignore-auto-link-option=Foo as well, without any new flags

[thakis]

I mean we could remove the somewhat special-purpose ignore-auto-link-option= flag then -- or rather, replace it with a no-warn-autolink flag. That wouldn't allow suppressing this on a per lib level, but that's probably fine?

[keith]

The benefit of per library opt outs is you can make sure new violations don't sneak in. But either way we can deal with that part separately from this if we want to add a new flag like that

[int3]

This comment was removed by int3.

[int3]

Making them warnings seems like a good compromise to me, since people can then use -fatal_warnings if they want to turn this into errors and we don't need a new flag.

My understanding is that some third-party/system libraries have these unsatisfied dependencies baked in, so it's not something that's always fixable in the build. We shouldn't require people to turn off -fatal_warnings in their builds to support third-party unfixable stuff.

I'm leaning toward Vy's option #3 (extend existing flag, no new flags), though I think #1 is good too (would make it easier for people to drop in LLD into their builds)

[oontvoo]

Hmm, unfortunately doesn't look like we have a majority here ... I guess how about we go with the option that requires the least API change which is #2 (ie., no new flag):

• not-found-errors from LC_LINKER_OPTION are warnings by default
• you can use the existing -ignore_linker_option to silence the warnings if desired.

P.S: This should be the least disruptive, too, because missing libraries/frameworks are already *errors* today - so we're not going to pollute the build logs any more than they're already broken. @int3 You're good with this? :) (You've pointed out that we shouldn't require people to turn off warnings, but this proposed change would not introduce more warnings/errors than whatever already exists)

[thakis]

Eh, in doubt I'd do what int3 recommends 😅

[oontvoo]

In D140225#4006170, @thakis wrote:

Eh, in doubt I'd do what int3 recommends 😅

I know I'd proposed this, but my reservation with #3 is that it's debatable how the the special * value interacts with others, eg., -ignore_linker_option=* -ignore_linker_option=foo -ignore_linker_option=bar could mean one of the two:

• * overrides all other instances of ignore_linker_option
• * covers all frameworks/libraries that are NOT foo or bar, specifically:
  • don't report errors for missing foo and bar even if the link failed
  • BUT don't errors for other missing framework only if the link succeeded (if the link failed, then report those errors)

Furthermore, this means that once a build sets -ignore_linker_option=*, new missing frameworks/libraries from bad LC_LINKER_OPTIONs will be ignored. This is bad because invalid LC_LINKER_OPTIONs aren't exactly "free" .

Ideally, I'd like to have a solution that:

• tolerates existing violations well enough that users can switch linkers
• AND at the same time prevents new violations. (if not "prevents" then at least makes users aware of new problems)

With that in mind, option 2 seems a bit better.

[int3]

This comment was removed by int3.

[int3]

Okay, you've convinced me. But just to clarify some of what I said earlier:

You've pointed out that we shouldn't require people to turn off warnings, but this proposed change would not introduce more warnings/errors than whatever already exists

Yes, but it still makes turning on -fatal_warnings harder. And I would like to be able to turn it on for most of our builds. (We are already looking at downgrading certain warnings internally to achieve this, but I would like to minimize the number of local patches.)

Furthermore, this means that once a build sets -ignore_linker_option=*, new missing frameworks/libraries from bad LC_LINKER_OPTIONs will be ignored.

I was thinking that we would still emit a non-warning message when we encounter missing frameworks that aren't explicitly ignored. But you're right, this is a deviation from how the existing -ignore_linker_option flag works for explicit ignores.

Anyway I realize that making them warnings isn't so bad because folks can use -ignore_linker_option to make the build warning-free. Turning on -fatal_warnings usually requires some build cleanup work anyway, so requiring people to figure out which libraries to ignore seems par for the course.

Eh, in doubt I'd do what int3 recommends 😅

Please push back if things don't make sense 😅 I'm not always thinking straight...

[oontvoo]

Updated diff to implement option #2, whic is to make missing frameworks/libraries warnings instead of errors.

Thanks, all, for the inputs!

[oontvoo]

In D140225#4006999, @int3 wrote:

Okay, you've convinced me. But just to clarify some of what I said earlier:

You've pointed out that we shouldn't require people to turn off warnings, but this proposed change would not introduce more warnings/errors than whatever already exists

Yes, but it still makes turning on -fatal_warnings harder. And I would like to be able to turn it on for most of our builds. (We are already looking at downgrading certain warnings internally to achieve this, but I would like to minimize the number of local patches.)

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

[oontvoo]

updated warning msg in test

[oontvoo]

added more tests for missing libaries (-l) too. Also included the files where the bad LC_LINKER_OPTION live to help debugging

[oontvoo]

reformat

[int3]

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.

lld/MachO/Driver.cpp

428

would be nice to have an explanatory comment here about why we are only emitting a warning

could we also update the comment 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.

429–430

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"

lld/test/MachO/lc-linker-option.ll

140

144–145

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)

[oontvoo]

addressed review comments:

• put fileloc's at the beginning of the warn msg
• remove ld64.lld from expected warning msgs in tests
• add comments

[oontvoo]

Thanks!

[oontvoo]

removed redundant whitespaces

[oontvoo]

typo

[keith]

After reading all this discussion, and seeing this well timed issue https://github.com/llvm/llvm-project/issues/59627 which is caused by this as well, I feel like we should take the opportunity to mirror ld64's behavior entirely, and potentially add a flag to opt-in to the more strict behavior that exists today. My opinion changed here since we talked about this last year because at least 5 people who have tried this have asked me about this specific issue. This issue is far more wide-spread than I originally thought in the community, and I don't think there's huge value in fixing these issues if your link succeeds anyways. Overall from this conversation it doesn't seem like anyone feels strongly enough about being strict on this issue that it justifies differing from ld64 and making adoption even just slightly harder.

I submitted https://reviews.llvm.org/D140491 with that approach if folks agree

[int3]

I feel bad for all the back-and-forth with @oontvoo's work, but in general I am in favor of things that improve ease of adoption. @keith's solution seems clean enough too. +1

[oontvoo]

In D140225#4011616, @int3 wrote:

I feel bad for all the back-and-forth with @oontvoo's work, but in general I am in favor of things that improve ease of adoption. @keith's solution seems clean enough too. +1

It's really not a big deal 😃 (I'd just like to see a solution submitted the sooner the better)

Keith's proposal LGTM , as long as we have that option to opt-in to the more strict because I still don't think bad limker-options are completely harmless. For one, they takes up space. For two, if our toolchains generate bad linker-options then we'd really like to know to fix the bug.

[int3]

Superseded by D140491: [lld-macho] Use ld64's LC_LINKER_OPTIONS behavior by default

[oontvoo]

rescinding this in favour of D140491.
Thanks!

lld/MachO/Config.h

struct Configuration {
   llvm::CachePruningPolicy thinLTOCachePolicy;
   llvm::StringRef thinLTOCacheDir;
   bool deadStripDylibs = false;
   bool demangle = false;
   bool deadStrip = false;
   bool errorForArchMismatch = false;
   bool ignoreAutoLink = false;
   // ld64 allows invalid auto link options as long as the link succeeds. LLD
-  // does not, but there are cases in the wild where the invalid linker options
+  // does not. Instead, it emits warnings on missing frameworks/libraries.
+  // But there are cases in the wild where the invalid linker options
   // exist. This allows users to ignore the specific invalid options in the case
   // they can't easily fix them.
   llvm::StringSet<> ignoreAutoLinkOptions;
   PlatformInfo platformInfo;
   std::optional<PlatformInfo> secondaryPlatformInfo;
   NamespaceKind namespaceKind = NamespaceKind::twolevel;
   UndefinedSymbolTreatment undefinedSymbolTreatment =
       UndefinedSymbolTreatment::error;

lld/MachO/Driver.cpp

if (config->printEachFile && magic != file_magic::archive && !isLazy)
       message(toString(newFile));
     inputFiles.insert(newFile);
   }
   return newFile;
 }
 
 static void addLibrary(StringRef name, bool isNeeded, bool isWeak,
                        bool isReexport, bool isHidden, bool isExplicit,
-                       LoadType loadType) {
+                       LoadType loadType, InputFile *refLoc = nullptr) {
   if (std::optional<StringRef> path = findLibrary(name)) {
     if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
             addFile(*path, loadType, /*isLazy=*/false, isExplicit,
                     /*isBundleLoader=*/false, isHidden))) {
       if (isNeeded)
         dylibFile->forceNeeded = true;
       if (isWeak)
         dylibFile->forceWeakImport = true;
       if (isReexport) {
         config->hasReexports = true;
         dylibFile->reexport = true;
       }
     }
     return;
   }
+
+  // We only emit a warning rather than an errors to make it easier
+  // for new users with existing bad LC_LINKER_OPTIONs to migrate to LLD.
+  // This is deliberately different from LD64's approach, which is that it'd
+  // swallow the errors if the link succeeds, because we'd like to be able
+  // to identify these bad LC_LINKER_OPTIONs and fix them.
+  if (loadType == LoadType::LCLinkerOption)
+    warn((refLoc == nullptr ? "" : toString(refLoc) + ": ") +
+         "library not found for -l" + name + ", from LC_LINKER_OPTION");
+  else
     error("library not found for -l" + name);
 }
 
 static DenseSet<StringRef> loadedObjectFrameworks;
 static void addFramework(StringRef name, bool isNeeded, bool isWeak,
-                         bool isReexport, bool isExplicit, LoadType loadType) {
+                         bool isReexport, bool isExplicit, LoadType loadType,
+                         InputFile *refLoc = nullptr) {
   if (std::optional<StringRef> path = findFramework(name)) {
     if (loadedObjectFrameworks.contains(*path))
       return;
 
     InputFile *file =
         addFile(*path, loadType, /*isLazy=*/false, isExplicit, false);
     if (auto *dylibFile = dyn_cast_or_null<DylibFile>(file)) {
       if (isNeeded)
if (auto *dylibFile = dyn_cast_or_null<DylibFile>(file)) {
       // symbols. Frameworks containing static archives are cached separately
       // in addFile() to share caching with libraries, and frameworks
       // containing dylibs should allow overwriting of attributes such as
       // forceNeeded by subsequent loads
       loadedObjectFrameworks.insert(*path);
     }
     return;
   }
+  if (loadType == LoadType::LCLinkerOption)
+    warn((refLoc == nullptr ? "" : toString(refLoc) + ": ") +
+         "framework not found for -framework " + name +
+         ", from LC_LINKER_OPTION ");
+  else
     error("framework not found for -framework " + name);
 }
 
 // Parses LC_LINKER_OPTION contents, which can add additional command line
 // flags. This directly parses the flags instead of using the standard argument
 // parser to improve performance.
 void macho::parseLCLinkerOption(InputFile *f, unsigned argc, StringRef data) {
   if (config->ignoreAutoLink)
     return;
void macho::parseLCLinkerOption(InputFile *f, unsigned argc, StringRef data) {
 
   unsigned i = 0;
   StringRef arg = argv[i];
   if (arg.consume_front("-l")) {
     if (config->ignoreAutoLinkOptions.contains(arg))
       return;
     addLibrary(arg, /*isNeeded=*/false, /*isWeak=*/false,
                /*isReexport=*/false, /*isHidden=*/false, /*isExplicit=*/false,
-               LoadType::LCLinkerOption);
+               LoadType::LCLinkerOption, f);
   } else if (arg == "-framework") {
     StringRef name = argv[++i];
     if (config->ignoreAutoLinkOptions.contains(name))
       return;
     addFramework(name, /*isNeeded=*/false, /*isWeak=*/false,
                  /*isReexport=*/false, /*isExplicit=*/false,
-                 LoadType::LCLinkerOption);
+                 LoadType::LCLinkerOption, f);
   } else {
     error(arg + " is not allowed in LC_LINKER_OPTION");
   }
 }
 
 static void addFileList(StringRef path, bool isLazy) {
   std::optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)

lld/test/MachO/lc-linker-option.ll

 ; RUN: %lld -F %t -framework Foo -framework Foo %t/main -o /dev/null
 
 ; RUN: llvm-as %t/foo.ll -o %t/Foo.framework/Foo
 ; RUN: llvm-as %t/load-framework-twice.ll -o %t/main
 ;; Order of the object with the LC_LINKER_OPTION vs -framework arg is important.
 ; RUN: %lld %t/main -F %t -framework Foo -framework Foo -o /dev/null
 ; RUN: %lld -F %t -framework Foo -framework Foo %t/main -o /dev/null
 
+;; Checks that "framework/library not found" from LC_LINKER_OPTIONS are reported
+;; as warnings and not errors.
+; 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
+
+; RUN: llvm-objdump --macho --syms %t/main-no-foo.out | FileCheck %s --check-prefix=SYMS_NO_FOO
+
+; WARN-FW: warning: {{.+}}load-framework-foo.o: framework not found for -framework Foo, from LC_LINKER_OPTION
+; WARN-LIB:warning: {{.+}}load-library-foo.o: library not found for -lfoo, from LC_LINKER_OPTION
+
+;; Verify that nothing from the framework is included.
+; SYMS_NO_FOO:       SYMBOL TABLE:
+; SYMS_NO_FOO-NEXT:  g     F __TEXT,__text _main
+; SYMS_NO_FOO-NOT:   g     O __DATA,__objc_data _OBJC_CLASS_$_TestClass
+
 ;--- framework.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-framework", !"CoreFoundation"}
 !llvm.linker.options = !{!0}
 
 declare void @_CFBigNumGetInt128(...)