[lld-macho] Do not warn on missing /usr/local/lib library search path

[thevinster]

Summary

...unless -Z is passed. Enabling -fatal_warnings turns these into errors
even though they are harmless. This mirrors ld64's behavior as shown here:
https://github.com/apple-oss-distributions/ld64/blob/main/src/ld/Options.cpp#L4361-L4363

This broke one of our internal builds when trying to enable -fatal_warnings and
switching ld64 to LLD.

[int3]

Looks like /Library/Frameworks is treated similarly: https://github.com/apple-oss-distributions/ld64/blob/main/src/ld/Options.cpp#L4415

I was thinking that instead of hardcoding those two values in warnIfNotDirectory, we could instead convert the last parameter of

getSearchPaths(unsigned optionCode, InputArgList &args,
               const std::vector<StringRef> &roots,
               const SmallVector<StringRef, 2> &systemPaths)

into

getSearchPaths(unsigned optionCode, InputArgList &args,
               const std::vector<StringRef> &roots,
               StringRef systemPath, StringRef optionalSystemPath)

& then skip warning if optionalSystemPath doesn't exist.

Also, could we have a test?

[thevinster]

We do handle systemPaths as optional right now in L186-L194. The issue is that the link arguments contain an explicit -L/usr/local/lib that doesn't exist.

Looks like /Library/Frameworks is treated similarly.

I can add that as an extra condition as well.

Also, could we have a test?

I'm not sure of a good way to test this because I'm trying to test the scenario that passing in -L/usr/local/lib that doesn't exist on the host doesn't result in a warning (and when passing -L/usr/local/lib with -Z, it does result in a warning). AFAICT, I can't test the universally test the latter behavior because that relies on having a non-existent /usr/local/lib directory (which most developer machines will almost be guaranteed to have). I can create a test for the former. It'll be no-op for the most developers since developers will have the directory, but if not, the behavior should be the same.

[int3]

We do handle systemPaths as optional right now in L186-L194. The issue is that the link arguments contain an explicit -L/usr/local/lib that doesn't exist.

I understand. I was thinking that the system path is optional even when explicitly specified, hence optionalSystemPath. Maybe strictlyOptionalSystemPath? But I can see that it might be confusing.

Not a fan of hardcoded paths, but as long as there's an explanatory comment I suppose it's fine.

AFAICT, I can't test the universally test the latter behavior because that relies on having a non-existent /usr/local/lib directory

Ah I was under the impression that it would be looking up $syslibroot/usr/local/lib, but I see now that it is looking up the non-rerooted path. Fair enough then, this does seem untestable.

[oontvoo]

In D142248#4074581, @thevinster wrote:

Also, could we have a test?

I'm not sure of a good way to test this because I'm trying to test the scenario that passing in -L/usr/local/lib that doesn't exist on the host doesn't result in a warning (and when passing -L/usr/local/lib with -Z, it does result in a warning). AFAICT, I can't test the universally test the latter behavior because that relies on having a non-existent /usr/local/lib directory (which most developer machines will almost be guaranteed to have). I can create a test for the former. It'll be no-op for the most developers since developers will have the directory, but if not, the behavior should be the same.

Would having this test run only on windows be helpful?

[thevinster]

After some internal discussions, I'm going to abandon this in favor of a local internal patch since the current behavior is easier to reason. If other folks run into this issue and also want this behavior, I'm happy to bring this back.

Would having this test run only on windows be helpful?

Could be an option if there's demand to upstream it.

lld/MachO/Driver.cpp

for (StringRef dir : config->frameworkSearchPaths) {
     }
 
     if (std::optional<StringRef> path = resolveDylibPath(symlink.str()))
       return resolvedFrameworks[key] = *path;
   }
   return {};
 }
 
-static bool warnIfNotDirectory(StringRef option, StringRef path) {
+static bool warnIfNotDirectory(StringRef option, StringRef path,
+                               InputArgList &args) {
   if (!fs::exists(path)) {
+    if (args.hasArg(OPT_Z) || path != "/usr/local/lib") {
       warn("directory not found for option -" + option + path);
       return false;
+    }
   } else if (!fs::is_directory(path)) {
     warn("option -" + option + path + " references a non-directory path");
     return false;
   }
   return true;
 }
 
 static std::vector<StringRef>
if (path::is_absolute(path, path::Style::posix)) {
         path::append(buffer, path);
         // Do not warn about paths that are computed via the syslib roots
         if (fs::is_directory(buffer)) {
           paths.push_back(saver().save(buffer.str()));
           found = true;
         }
       }
     }
-    if (!found && warnIfNotDirectory(optionLetter, path))
+    if (!found && warnIfNotDirectory(optionLetter, path, args))
       paths.push_back(path);
   }
 
   // `-Z` suppresses the standard "system" search paths.
   if (args.hasArg(OPT_Z))
     return paths;
 
   for (const StringRef &path : systemPaths) {