[lld-macho] Check simulator platforms to avoid issuing false positive errors.

[oontvoo]

Summary

Currently the linker causes unnecessary errors when either the target or the config's platform is a simulator.

[int3]

this needs a test :)

lld/MachO/InputFiles.cpp

144

nit 1: for uniformity, can we use a DenseMap instead of a std::map? I know perf isn't important here but it would be nice to avoid std::map where possible

nit 2: I'm pretty sure we can initialize this via initializer lists rather than a lambda, i.e. something like

platformMap = {{PlatformKind::iOS, PlatformKind::iOSSimulator}, {PlatformKind::iOS, PlatformKind::iOSSimulator}, ...};

160

rm?

[oontvoo]

added some tests

[oontvoo]

lld/test/MachO/invalid/incompatible-arch.s

34

Question: I've tried to make llvm-mc produce an obj file(*) with ios-simulator (or watchos-simulator) platform but it ended up producing files without any platform info.

(*) command used: llvm-mc -filetype=obj -triple=x86_64-apple-iossimulator10.15.0 %s -o %t/test_x86_ios_sim.o

Any idea what I got wrong?

(not super important for this patch , but would be nice to be able to test that an obj file with simulator-platform won't trigger an error against a "real" platform)

[oontvoo]

cleanup

[oontvoo]

lld/MachO/InputFiles.cpp

144

(actually not done)

Re: DenseMap, I couldn't use the DenseMap here because :

In file included from /Users/vyng/repo/llvm-project/llvm/include/llvm/ADT/DenseSet.h:16:
/Users/vyng/repo/llvm-project/llvm/include/llvm/ADT/DenseMap.h:456:22: error: no member named 'getEmptyKey' in 'llvm::DenseMapInfo<llvm::MachO::PlatformKind>'
    return KeyInfoT::getEmptyKey();

Didn't seem like it's worth changing the PlatformKind class ... thoughts?

[oontvoo]

comment

[smeenai]

lld/test/MachO/invalid/incompatible-arch.s

34

I believe the correct triple is something like x86_64-apple-ios9.0.0-simulator. There's probably some minimum version required to make it emit the platform version load command instead of the minimum version load command

[int3]

lld/MachO/InputFiles.cpp

144

Ah, is it because the type needs to be hashable? Yeah as-is is fine then

lld/test/MachO/invalid/incompatible-arch.s

34

huh, yeah, that's weird. Not sure why simulators aren't handled the same way.

We should still be able to test the simulator platform against "real" platform case using dylibs though, right?

[int3]

lld/test/MachO/invalid/incompatible-arch.s

34

I believe the correct triple is something like x86_64-apple-ios9.0.0-simulator

ooh yeah this works, thanks @smeenai!

[oontvoo]

add more tests

[oontvoo]

lld/test/MachO/invalid/incompatible-arch.s

34

Thanks!!!
added more test using this.

[int3]

lld/MachO/InputFiles.cpp

163–164

how about having a function that just canonicalizes the platform name, removing the simulator part? i.e. something like

removeSimulator(config->platform()) != removeSimulator(platformInfo->target.Platform)

pros: the map will only need half the entries, plus it's a bit more symmetric

either is fine though

lld/test/MachO/invalid/incompatible-arch.s

37–38

for clarity

45

why doesn't this say "iOS simulator"?

[oontvoo]

lld/test/MachO/invalid/incompatible-arch.s

45

It seems the "simulator" part was removed by llvm-mc. (Running otool on test_x86_ios_sim.oshows that it's only "IOS" and not IOS-simulator

[oontvoo]

-o /dev/null

[int3]

lld/test/MachO/invalid/incompatible-arch.s

45

Ah, that seems to happen when the specified iOS version is old enough to require LC_MIN_VERSION instead of LC_BUILD_VERSION. Using a modern iOS version should preserve the 'simulator' part:

> echo "" | llvm-mc -filetype=obj -triple=x86_64-apple-ios14.0-simulator -o test_x86_ios_sim.o
> llvm-objdump --macho --all-headers test_x86_ios_sim.o
...
Load command 1
       cmd LC_BUILD_VERSION
   cmdsize 24
  platform iossimulator
       sdk n/a
     minos 14.0
    ntools 0

[int3]

lld/MachO/InputFiles.cpp

163–164

another advantage of removeSimulator() is that we can have just one check here, i.e. no need for a separate config->platform() != platformInfo->target.Platform

With that in mind I think it's significantly nicer actually... could we do that? :)

[oontvoo]

increase version in test to preseve "simulator" + use removeSimulator() approach

lld/MachO/InputFiles.cpp

163–164

done

lld/test/MachO/invalid/incompatible-arch.s

45

Thanks!

[int3]

lgtm, thanks!

lld/MachO/InputFiles.cpp

if (const auto *cmd = findCommand<version_min_command>(
     }
     platformInfo.minimum = decodeVersion(cmd->version);
     return platformInfo;
   }
 
   return None;
 }
 
+static bool checkSimulatedPlatform(PlatformKind lhs, PlatformKind rhs) {
+  // Mapping of platform to simulator and vice-versa.
+  static const std::map<PlatformKind, PlatformKind> platformMap = {
+      {PlatformKind::iOS, PlatformKind::iOSSimulator},
+      {PlatformKind::iOSSimulator, PlatformKind::iOS},
+      {PlatformKind::tvOS, PlatformKind::tvOSSimulator},
+      {PlatformKind::tvOSSimulator, PlatformKind::tvOS},
+      {PlatformKind::watchOS, PlatformKind::watchOSSimulator},
+      {PlatformKind::watchOSSimulator, PlatformKind::watchOS}};
+
+  auto iter = platformMap.find(lhs);
+  return iter != platformMap.end() && iter->second == rhs;
+}
+
 static bool checkCompatibility(const InputFile *input) {
   Optional<PlatformInfo> platformInfo = getPlatformInfo(input);
   if (!platformInfo)
     return true;
   // TODO: Correctly detect simulator platforms or relax this check.
-  if (config->platform() != platformInfo->target.Platform) {
+  if (config->platform() != platformInfo->target.Platform &&
+      !checkSimulatedPlatform(config->platform(),
+                              platformInfo->target.Platform)) {
     error(toString(input) + " has platform " +
           getPlatformName(platformInfo->target.Platform) +
           Twine(", which is different from target platform ") +
           getPlatformName(config->platform()));
     return false;
   }
   if (platformInfo->minimum <= config->platformInfo.minimum)
     return true;
   error(toString(input) + " has version " +

lld/test/MachO/invalid/incompatible-arch.s

 # OBJ-PLAT: {{.*}}test_x86.o has platform macOS, which is different from target platform iOS
 
 # RUN: %lld %t/test_x86.o -lSystem -arch x86_64 -platform_version macOS 10.15.0 10.15.0 -o /dev/null
 
 # RUN: not %lld %t/test_x86.o -lSystem -arch x86_64 -platform_version macOS 10.14.0 10.15.0 \
 # RUN:  -o /dev/null 2>&1 | FileCheck %s --check-prefix=OBJ-VERSION
 # OBJ-VERSION: {{.*}}test_x86.o has version 10.15.0, which is newer than target minimum of 10.14.0
 
+## Test that simulators platforms are compat with their simulatees.   
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-ios10.15.0 %s -o %t/test_x86_ios.o
+
+# RUN: %lld -dylib  -platform_version ios-simulator 10.15.0 10.15.0 %t/test_x86_ios.o
+# RUN: not %lld -dylib  -platform_version watchos-simulator 10.15.0 10.15.0 %t/test_x86_ios.o \
+# RUN:  -o /dev/null 2>&1 | FileCheck %s --check-prefix=CROSS-SIM
+# CROSS-SIM: {{.*}}test_x86_ios.o has platform iOS, which is different from target platform watchOS Simulator
+  
+
 .globl _main
 _main:
   ret