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

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:1129
+        auto file = make<DylibFile>(child, umbrella, /*isBundleLoader=*/false,
+                                    umbrella->hasSeparateMacAndCatalystVersion);
         file->parseReexports(child);
----------------
thakis wrote:
> int3 wrote:
> > should we have a test case that covers re-exports with separate versions?
> Oh hmm good point, I'll look into it (before landing).
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.


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

https://reviews.llvm.org/D124275