Currently the linker causes unnecessary errors when either the target or the config's platform is a simulator.
Details
- Reviewers
int3 - Group Reviewers
Restricted Project - Commits
- rG23233ad139f4: [lld-macho] Check simulator platforms to avoid issuing false positive errors.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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}, ...}; | |
158–159 | rm? |
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) |
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? |
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 |
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? |
lld/test/MachO/invalid/incompatible-arch.s | ||
---|---|---|
34 | Thanks!!! |
lld/MachO/InputFiles.cpp | ||
---|---|---|
161–162 | 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"? |
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 |
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 |
lld/MachO/InputFiles.cpp | ||
---|---|---|
161–162 | 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? :) |
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