This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by oontvoo on May 4 2021, 1:25 PM.

Details

Summary

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

Diff Detail

Event Timeline

oontvoo created this revision.May 4 2021, 1:25 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
oontvoo requested review of this revision.May 4 2021, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2021, 1:25 PM
oontvoo edited the summary of this revision. (Show Details)May 4 2021, 1:25 PM
int3 added a comment.May 4 2021, 2:53 PM

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}, ...};
167

rm?

oontvoo updated this revision to Diff 342878.May 4 2021, 3:06 PM

added some tests

oontvoo marked 2 inline comments as done.May 4 2021, 3:10 PM
oontvoo added inline comments.
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 marked an inline comment as not done.May 4 2021, 3:20 PM
oontvoo added inline comments.
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 marked an inline comment as not done.May 4 2021, 3:20 PM
smeenai added a subscriber: smeenai.May 4 2021, 3:22 PM
smeenai added inline comments.
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 added inline comments.May 4 2021, 3:39 PM
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 added inline comments.May 4 2021, 3:40 PM
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 updated this revision to Diff 342920.May 4 2021, 5:42 PM

add more tests

oontvoo marked 3 inline comments as done.May 4 2021, 5:42 PM
oontvoo added inline comments.
lld/test/MachO/invalid/incompatible-arch.s
34

Thanks!!!
added more test using this.

int3 accepted this revision.May 5 2021, 11:18 AM
int3 added inline comments.
lld/MachO/InputFiles.cpp
170–171

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"?

This revision is now accepted and ready to land.May 5 2021, 11:18 AM
oontvoo marked 2 inline comments as done.May 5 2021, 2:40 PM
oontvoo added inline comments.
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 updated this revision to Diff 343199.May 5 2021, 2:42 PM

-o /dev/null

int3 added inline comments.May 5 2021, 2:49 PM
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 added inline comments.May 5 2021, 2:52 PM
lld/MachO/InputFiles.cpp
170–171

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 updated this revision to Diff 343204.May 5 2021, 2:59 PM
oontvoo marked 4 inline comments as done.

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

lld/MachO/InputFiles.cpp
170–171

done

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

Thanks!

int3 added a comment.May 5 2021, 3:03 PM

lgtm, thanks!

This revision was landed with ongoing or failed builds.May 5 2021, 3:08 PM
This revision was automatically updated to reflect the committed changes.