[lld/mac] For catalyst outputs, tolerate linking against mac-only tbd files

[thakis]

Summary

Before this,

clang empty.cc -target x86_64-apple-ios13.1-macabi \
    -framework CoreServices -fuse-ld=lld

would error out with

ld64.lld: error: path/to/MacOSX.sdk/System/Library/Frameworks/
     CoreServices.framework/Versions/A/Frameworks/CarbonCore.framework/
     Versions/A/CarbonCore.tbd(
         /System/Library/Frameworks/
         CoreServices.framework/Versions/A/Frameworks/CarbonCore.framework/
         Versions/A/CarbonCore) is incompatible with x86_64 (macCatalyst)

Now it works, like with ld64.

[thakis]

zap trailing newlines

[thakis]

s/Both/Separate/

[int3]

lgtm

lld/MachO/InputFiles.cpp

1129

should we have a test case that covers re-exports with separate versions?

[thakis]

clang-format, and try to get test passing on windows

[thakis]

thanks!

lld/MachO/InputFiles.cpp

1129

Oh hmm good point, I'll look into it (before landing).

[thakis]

lld/MachO/InputFiles.cpp

1129

While writing the test for this, I noticed that ld64 doesn't actually accept the MacOnly.framework case in the existing test here.

This patch is incorrect: I misread the

// Normally linked dylibs need to support all the platforms we are building for,
// with the exception of zippered binaries linking to macOS only libraries in the OS.
// To handle that case we exit early when the commandline platform is iOSMac, if
// the platforms also contain macOS.
if (cmdLinePlatform == ld::Platform::iOSMac && contains(ld::Platform::macOS) && !isUnzipperedTwin)
    return;

in ld64's PlatformSupport.cpp. This doesn't check the dylib architecture at all, it only checks the output platforms. We don't support writing zippered outputs yet, so we don't need this code at all yet.

What makes the thing mentioned in the patch description work in ld64 is instead the next few lines I think:

// <rdar://problem/48416765> spurious warnings when iOSMac binary is built links with zippered dylib that links with macOS dylib
// <rdar://problem/50517845> iOSMac app links simulator frameworks without warning/error
if  ( indirectDylib && dylibPlatforms.contains(ld::Platform::macOS) && contains(ld::Platform::iOSMac) )
    return;

We don't need all that hasSeparateMacAndCatalystVersion machinery for that.

I'll make a new patch for that, I think.

[thakis]

New patch is at D124336.

The machinery in this patch here will be useful if we ever add support for creating zippered libraries, but for now we don't need it.

lld/MachO/DriverUtils.cpp

Optional<StringRef> macho::resolveDylibPath(StringRef dylibPath) {
 
   bool dylibExists = fs::exists(dylibPath);
   searchedDylib(dylibPath, dylibExists);
   if (dylibExists)
     return saver().save(dylibPath);
   return {};
 }
 
+static Optional<std::string> replace(StringRef in,
+                                     StringRef what,
+                                     StringRef with) {
+  size_t p = in.find(what);
+  if (p == StringRef::npos)
+    return None;
+  return (in.substr(0, p) + with + in.substr(p + what.size())).str();
+}
+
+static Optional<std::string> insert_before(StringRef in,
+                                           StringRef before,
+                                           StringRef what) {
+  size_t p = in.find(before);
+  if (p == StringRef::npos)
+    return None;
+  return (in.substr(0, p) + what + in.substr(p)).str();
+}
+
+// Catalyst uses three kinds of libraries:
+// 1. Normal mac libraries. These are often, but not always, marked
+//    as libraries that work on both mac and catalyst. This is the
+//    common case.
+// 2. Catalyst-only libraries, which are available for catalyst apps,
+//    but not for non-catalyst apps (e.g. UIKit.framework)
+// 3. Libraries that are available to both mac and catalyst apps,
+//    but that are slightly different for mac and catalyst
+//    (e.g. PDFKit.framework).
+//
+// Libraries for case 1 are the normal libraries in MacOSX.sdk.
+// Libraries for cases 2 and 3 are below /System/iOSSupport in the SDK.
+// This function returns true for libraries in case 3.
+//
+// Case 3 is called "unzippered twins" in ld64.
+static bool hasBothMacAndCatalystVersion(StringRef p) {
+  // First case: Library's path is inside /System/iOSSupport.
+  // This is certainly a catalyst library. Check if the library also
+  // exists outside /System/iOSSupport to differentiate cases 2 and 3.
+  if (auto inside = replace(p, "/System/iOSSupport/", "/"))
+    return fs::exists(*inside);
+
+  // Second case: Library's path is outside /System/iOSSupport.
+  // Check if it exists inside /System/iOSSupport too to differentiate cases 1
+  // and 3.
+  if (auto outside = insert_before(p, "/System/Library/", "/System/iOSSupport"))
+    return fs::exists(*outside);
+  if (auto outside = insert_before(p, "/usr/lib/", "/System/iOSSupport"))
+    return fs::exists(*outside);
+
+  return false;
+}
+
 // It's not uncommon to have multiple attempts to load a single dylib,
 // especially if it's a commonly re-exported core library.
 static DenseMap<CachedHashStringRef, DylibFile *> loadedDylibs;
 
 DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
                             bool isBundleLoader) {
   CachedHashStringRef path(mbref.getBufferIdentifier());
   DylibFile *&file = loadedDylibs[path];
   if (file)
     return file;
 
   DylibFile *newFile;
   file_magic magic = identify_magic(mbref.getBuffer());
   if (magic == file_magic::tapi_file) {
     Expected<std::unique_ptr<InterfaceFile>> result = TextAPIReader::get(mbref);
     if (!result) {
       error("could not load TAPI file at " + mbref.getBufferIdentifier() +
             ": " + toString(result.takeError()));
       return nullptr;
     }
-    file = make<DylibFile>(**result, umbrella, isBundleLoader);
+    file = make<DylibFile>(**result, umbrella, isBundleLoader,
+                           hasBothMacAndCatalystVersion(path.val()));
 
     // parseReexports() can recursively call loadDylib(). That's fine since
     // we wrote the DylibFile we just loaded to the loadDylib cache via the
     // `file` reference. But the recursive load can grow loadDylibs, so the
     // `file` reference might become invalid after parseReexports() -- so copy
     // the pointer it refers to before continuing.
     newFile = file;
     if (newFile->exportingFile)

lld/MachO/InputFiles.h

public:
   // library's namespace. Those sub-libraries can also have their own
   // re-exports. When loading a re-exported dylib, `umbrella` should be set to
   // the root dylib to ensure symbols in the child library are correctly bound
   // to the root. On the other hand, if a dylib is being directly loaded
   // (through an -lfoo flag), then `umbrella` should be a nullptr.
   explicit DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
                      bool isBundleLoader = false);
   explicit DylibFile(const llvm::MachO::InterfaceFile &interface,
-                     DylibFile *umbrella = nullptr,
-                     bool isBundleLoader = false);
+                     DylibFile *umbrella,
+                     bool isBundleLoader,
+                     bool hasBothMacAndCatalystVersion);
 
   void parseLoadCommands(MemoryBufferRef mb);
   void parseReexports(const llvm::MachO::InterfaceFile &interface);
   bool isReferenced() const { return numReferencedSymbols > 0; }
 
   static bool classof(const InputFile *f) { return f->kind() == DylibKind; }
 
   StringRef installName;
public:
   bool forceWeakImport = false;
   bool deadStrippable = false;
   bool explicitlyLinked = false;
   // An executable can be used as a bundle loader that will load the output
   // file being linked, and that contains symbols referenced, but not
   // implemented in the bundle. When used like this, it is very similar
   // to a dylib, so we've used the same class to represent it.
   bool isBundleLoader;
+  bool hasBothMacAndCatalystVersion = false;
 
 private:
   bool handleLDSymbol(StringRef originalName);
   void handleLDPreviousSymbol(StringRef name, StringRef originalName);
   void handleLDInstallNameSymbol(StringRef name, StringRef originalName);
   void handleLDHideSymbol(StringRef name, StringRef originalName);
   void checkAppExtensionSafety(bool dylibIsAppExtensionSafe) const;
 

lld/MachO/InputFiles.cpp

   }
 
   // FIXME: Should this be further up?
   if (currentTopLevelTapi) {
     for (InterfaceFile &child :
          make_pointee_range(currentTopLevelTapi->documents())) {
       assert(child.documents().empty());
       if (path == child.getInstallName()) {
-        auto file = make<DylibFile>(child, umbrella);
+        auto file = make<DylibFile>(child, umbrella, /*isBundleLoader=*/false,
+                                    umbrella->hasBothMacAndCatalystVersion);
         file->parseReexports(child);
         return file;
       }
     }
   }
 
   if (Optional<StringRef> dylibPath = resolveDylibPath(path))
     return loadDylib(*dylibPath, umbrella);

       DylibFile *dylib = findDylib(dylibPath, umbrella, nullptr);
       if (!dylib)
         error(Twine("unable to locate library '") + dylibPath +
               "' loaded from '" + toString(this) + "' for -flat_namespace");
     }
   }
 }
 
-// Some versions of XCode ship with .tbd files that don't have the right
+// Some versions of Xcode ship with .tbd files that don't have the right
 // platform settings.
 constexpr std::array<StringRef, 4> skipPlatformChecks{
     "/usr/lib/system/libsystem_kernel.dylib",
     "/usr/lib/system/libsystem_platform.dylib",
     "/usr/lib/system/libsystem_pthread.dylib",
     "/usr/lib/system/libcompiler_rt.dylib"};
 
+static bool skipPlatformCheckForCatalyst(const InterfaceFile &interface,
+                                         bool hasBothMacAndCatalystVersion) {
+  // Catalyst outputs can link against macOS libraries, as long those libraries
+  // do not have a dedicated catalyst version.  See also the comment above
+  // hasBothMacAndCatalystVersion() in DriverUtils.cpp.
+  if (config->platform() != PLATFORM_MACCATALYST ||
+      hasBothMacAndCatalystVersion)
+    return false;
+  return is_contained(interface.targets(),
+                      MachO::Target(config->arch(), PLATFORM_MACOS));
+}
+
 DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
-                     bool isBundleLoader)
+                     bool isBundleLoader, bool hasBothMacAndCatalystVersion)
     : InputFile(DylibKind, interface), refState(RefState::Unreferenced),
-      isBundleLoader(isBundleLoader) {
+      isBundleLoader(isBundleLoader),
+      hasBothMacAndCatalystVersion(hasBothMacAndCatalystVersion) {
   // FIXME: Add test for the missing TBD code path.
 
   if (umbrella == nullptr)
     umbrella = this;
   this->umbrella = umbrella;
 
   installName = saver().save(interface.getInstallName());
   compatibilityVersion = interface.getCompatibilityVersion().rawValue();
   currentVersion = interface.getCurrentVersion().rawValue();
 
   if (config->printEachFile)
     message(toString(this));
   inputFiles.insert(this);
 
   if (!is_contained(skipPlatformChecks, installName) &&
-      !is_contained(interface.targets(), config->platformInfo.target)) {
+      !is_contained(interface.targets(), config->platformInfo.target) &&
+      !skipPlatformCheckForCatalyst(interface, hasBothMacAndCatalystVersion)) {
     error(toString(this) + " is incompatible with " +
           std::string(config->platformInfo.target));
     return;
   }
 
   checkAppExtensionSafety(interface.isApplicationExtensionSafe());
 
   exportingFile = isImplicitlyLinked(installName) ? this : umbrella;

lld/test/MachO/Inputs/MacOSX.sdk/System/Library/Frameworks/MacAndCatalyst.framework/MacAndCatalyst.tbd

+--- !tapi-tbd
+tbd-version:      4
+targets:          [ x86_64-macos ]
+uuids:
+  - target: x86_64-macos
+    value:  00000000-0000-0000-0000-000000000000
+install-name:     'MacAndCatalyst.dylib'
+current-version:  0001.001.1
+exports:
+  - targets:      [ x86_64-macos ]
+    symbols:      [ _bar ]
+...

lld/test/MachO/Inputs/MacOSX.sdk/System/Library/Frameworks/MacOnly.framework/MacOnly.tbd

+--- !tapi-tbd
+tbd-version:      4
+targets:          [ x86_64-macos ]
+uuids:
+  - target: x86_64-macos
+    value:  00000000-0000-0000-0000-000000000000
+install-name:     'MacOnly.dylib'
+current-version:  0001.001.1
+exports:
+  - targets:      [ x86_64-macos ]
+    symbols:      [ _bar ]
+...

lld/test/MachO/Inputs/MacOSX.sdk/System/iOSSupport/System/Library/Frameworks/MacAndCatalyst.framework/MacAndCatalyst.tbd

+--- !tapi-tbd
+tbd-version:      4
+targets:          [ x86_64-maccatalyst ]
+uuids:
+  - target: x86_64-maccatalyst
+    value:  00000000-0000-0000-0000-000000000000
+install-name:     'MacAndCatalyst.dylib'
+current-version:  0001.001.1
+exports:
+  - targets:      [ x86_64-maccatalyst ]
+    symbols:      [ _bar ]
+...

lld/test/MachO/zippered.yaml

 # REQUIRES: x86
 # RUN: rm -rf %t; mkdir %t
 # RUN: yaml2obj %s > %t/test.dylib
 # RUN: echo "" | llvm-mc -filetype=obj -triple=x86_64-apple-macos10.15 -o %t/test_macos.o
 # RUN: echo "" | llvm-mc -filetype=obj -triple=x86_64-apple-ios13.15.0-macabi -o %t/test_maccatalyst.o
 # RUN: echo "" | llvm-mc -filetype=obj -triple=x86_64-apple-ios13.15.0 -o %t/test_ios.o
 
 # RUN: %lld -lSystem -dylib %t/test.dylib %t/test_macos.o -o /dev/null
 # RUN: %no-arg-lld -syslibroot %S/Inputs/MacOSX.sdk -lSystem -dylib -arch x86_64 -platform_version mac-catalyst 13.15.0 14.0 %t/test.dylib %t/test_maccatalyst.o -o /dev/null
 
+# RUN: %no-arg-lld -syslibroot %S/Inputs/MacOSX.sdk -lSystem -dylib -arch x86_64 -platform_version mac-catalyst 13.15.0 14.0 %t/test.dylib %t/test_maccatalyst.o -o /dev/null -framework MacOnly
+
+# RUN: not %no-arg-lld -syslibroot %S/Inputs/MacOSX.sdk -lSystem -dylib -arch x86_64 -platform_version mac-catalyst 13.15.0 14.0 %t/test.dylib %t/test_maccatalyst.o -o /dev/null -framework MacAndCatalyst 2>&1 | FileCheck --check-prefix=UNZIPPERED_TWIN %s
+# UNZIPPERED_TWIN: System/Library/Frameworks/MacAndCatalyst.framework/MacAndCatalyst.tbd(MacAndCatalyst.dylib) is incompatible with x86_64 (macCatalyst)
+
 # RUN: not %no-arg-lld -syslibroot %S/Inputs/MacOSX.sdk -lSystem -dylib -arch x86_64 -platform_version ios 13.15.0 14.0 %t/test.dylib %t/test_ios.o -o /dev/null 2>&1 | FileCheck %s
 # CHECK: test.dylib has platform macOS/macCatalyst, which is different from target platform iOS
 
 --- !mach-o
 FileHeader:
   magic:           0xFEEDFACF
   cputype:         0x1000007
   cpusubtype:      0x3