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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:144
+  // Mapping of platform to simulator and vice-versa.
+  static std::map<PlatformKind, PlatformKind> *platformMap = []() {
+    auto *ret = new std::map<PlatformKind, PlatformKind>();
----------------
oontvoo wrote:
> int3 wrote:
> > 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}, ...};
> > ```
> (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? 
Ah, is it because the type needs to be hashable? Yeah as-is is fine then


================
Comment at: lld/test/MachO/invalid/incompatible-arch.s:34
+## 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
+
----------------
smeenai wrote:
> oontvoo wrote:
> > **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)
> 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
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101855