This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] improve handling of -platform_version
ClosedPublic

Authored by gkm on Jun 8 2020, 11:11 AM.

Details

Summary

This improves the handling of -platform_version by addressing the FIXME in the code to process the arguments.

Diff Detail

Event Timeline

compnerd created this revision.Jun 8 2020, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 11:11 AM
tschuett added a subscriber: tschuett.EditedJun 8 2020, 12:13 PM

Neither PlatformType nor PlatformKind cover all cases of -platform_version, but TextAPI uses PlatformKind.

The ld man page references <mach-o/loader.h> for the numeric values. PlatformType seems to be outdated. It is missing PLATFORM_DRIVERKIT = 10. For the same reason, PlatformKind might be missing driverkit support.

The OSX 10.14 loader.h is missing the driverkit entry. Only the OSX 10.15 loader.h contains the driverkit entry.

DriverKit was introduced in 10.15, so its not surprising that the LLVM enumeration for the object file format (PLATFORM_TYPE) does not contain the value yet. AIUI, PlatformType is the loader representation (and is used in the object file format). The values in the general purpose usage live outside of the loader (TAPI and dyld are separate code bases) but the PlatformKind enumeration mirrors the PlatformType enumeration.

compnerd added a reviewer: Restricted Project.Jun 9 2020, 9:13 AM
smeenai added a reviewer: gkm.Jun 9 2020, 5:52 PM

This is a dupe of a part of D80582.

gkm requested changes to this revision.Jun 10 2020, 10:42 PM

I removed the -parse_version portions of D80582, since it's better for my diff to be just foundational Options.td changes, and because @compnerd's diff is nicer.

lld/MachO/Driver.cpp
363–364

LLVM's option library does support multi-part options directly. See the first rev. of D80582.

467

Use error() for option-parsing diagnostics.

473

Use error() for option-parsing diagnostics.

This revision now requires changes to proceed.Jun 10 2020, 10:42 PM
gkm added a comment.Jul 9 2020, 9:10 AM

@compnerd, do you intend to complete & land this diff?

gkm commandeered this revision.Aug 8 2020, 9:06 AM
gkm edited reviewers, added: compnerd; removed: gkm.
gkm updated this revision to Diff 284134.Aug 8 2020, 9:14 AM

Finish, fix & expand tests

gkm retitled this revision from lld: improve handling of `-platform_version` to [lld-macho] improve handling of -platform_version.Aug 8 2020, 9:21 AM
gkm added a comment.Aug 8 2020, 6:25 PM

@tschuett, see D85594 for DriverKit platform addition

int3 added inline comments.Aug 9 2020, 8:01 AM
lld/test/MachO/platform-version.s
11

nit: it's more common to use hyphens instead of underscores for check prefixes, i.e. FAIL-MISSING

Also, I was thinking it might make sense to split this test into two parts, with all the FAIL- checks going under invalid/, and the passing ones remaining here. Once we support LC_BUILD_VERSION, we'll probably want to expand the test for passing platform strings to include checks that the LC_BUILD_VERSION command contains the expected output.

42

even though man ld indicates that ios-sim is a valid platform string, I couldn't get it to work on ld64 :/ it seems like ios-simulator is expected instead. (I tried that string out after finding it in rG25ce33a6e).

gkm added inline comments.Aug 9 2020, 9:49 PM
lld/test/MachO/platform-version.s
11

For now, I would rather keep all tests in one place.

42

Indeed, I got the *-sim names from the ld(1) manual page in Xcode 11. Obviously it is wrong.

I looked at ld64/src/ld/PlatformSupport.cpp the newest ld64 source I have (v512), and found that -platform_version matches are case insensitive, with words separated by either dash or space. Here are the official names:

"unknown"
"macOS"
"iOS"
"tvOS"
"watchOS"
"bridgeOS"
"Mac Catalyst"
"iOS Simulator"
"tvOS Simulator"
"watchOS Simulator"
"DriverKit"
"free standing"

I cross-checked against the binary for Xcode 11.3 (v530) via strings(1). I update to make the matches case insensitive and to tolerate either space or dash for word separators.

gkm updated this revision to Diff 284368.Aug 10 2020, 7:49 AM

respond to review feedback

int3 accepted this revision.Aug 10 2020, 9:58 AM

lgtm!

lld/test/MachO/platform-version.s
46

would be good to make one of these test strings contain a hyphen

This revision is now accepted and ready to land.Aug 10 2020, 9:58 AM
gkm updated this revision to Diff 284506.Aug 10 2020, 2:46 PM

Add dash to a couple of multi-word platform names

gkm marked an inline comment as done.Aug 10 2020, 2:47 PM
gkm added inline comments.
lld/test/MachO/platform-version.s
46

Done.

This revision was automatically updated to reflect the committed changes.
lld/test/MachO/platform-version.s