This is an archive of the discontinued LLVM Phabricator instance.

[MachO] Add skeletal support for DriverKit platform
ClosedPublic

Authored by gkm on Aug 8 2020, 6:22 PM.

Details

Summary

Define the platform ID = 10, and simple mappings between platform ID & name.

Diff Detail

Event Timeline

gkm created this revision.Aug 8 2020, 6:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2020, 6:22 PM
gkm requested review of this revision.Aug 8 2020, 6:22 PM
MaskRay added inline comments.Aug 8 2020, 10:34 PM
llvm/include/llvm/BinaryFormat/MachO.h
499

Add a trailing comma so that the next update can be easier.

llvm/include/llvm/TextAPI/MachO/Platform.h
33

Add a trailing comma so that the next update can be easier.

gkm added a comment.Aug 10 2020, 8:54 AM

@tschuett, I prefer not to do anything that involves semantics for DriverKit, since I don't have any domain knowledge. For that matter, perhaps I shouldn't even follow-through with this diff, which I only embarked upon at your suggestion because the original author of the MachO -platform_version diff (I commandeered it) had a TODO comment. I can code-monkey the TBDv4 testcase via simple substitution, but what about TBDv1 .. TBDv3? IDK, and don't want to spend time determining minimum version# for DriverKit. Do you know that?

No worries. I added @ributzka and @cishida as reviewers because they should know.

gkm updated this revision to Diff 284402.Aug 10 2020, 9:06 AM

Add trailing commas, as per review feeddback

MaskRay accepted this revision.Aug 10 2020, 9:09 AM

No worries. I added @ributzka and @cishida as reviewers because they should know.

A lot of string values in that list miss tests. People with domain knowledge can add the tests separately, I think.

This revision is now accepted and ready to land.Aug 10 2020, 9:09 AM
In D85594#2207080, @gkm wrote:

@tschuett, I prefer not to do anything that involves semantics for DriverKit, since I don't have any domain knowledge. For that matter, perhaps I shouldn't even follow-through with this diff, which I only embarked upon at your suggestion because the original author of the MachO -platform_version diff (I commandeered it) had a TODO comment. I can code-monkey the TBDv4 testcase via simple substitution, but what about TBDv1 .. TBDv3? IDK, and don't want to spend time determining minimum version# for DriverKit. Do you know that?

The current TextAPI library is simple reader/writer support and the referenced unit tests reflect any valid inputs to generate tbd and nothing else. IMO adding one TBDv4 test case with just the target info and install name would be good to add. I'd be happy to write the patch for it after this lands if that helps.

llvm/lib/TextAPI/MachO/Platform.cpp
52

DriverKit case should be included

gkm updated this revision to Diff 285539.Aug 13 2020, 6:25 PM

add unittest for new symbol PlatformKind::driverKit

gkm added inline comments.Aug 13 2020, 6:27 PM
llvm/unittests/TextAPI/TextStubV4Tests.cpp
761–784

@cishida, is this sufficient?

cishida accepted this revision.Aug 13 2020, 9:38 PM
cishida added inline comments.
llvm/unittests/TextAPI/TextStubV4Tests.cpp
761–784

Looks good, thanks!

gkm added inline comments.Aug 13 2020, 10:13 PM
llvm/lib/TextAPI/MachO/Platform.cpp
52

@cishida, a case here depends on first defining DriverKit for enum Triple::OSType in llvm/include/llvm/ADT/Triple.h, which does not yet exist. (Same for BridgeOS according to your comment above). I see that properly supporting these Triple::OSType enums requires knowing minimum OS versions in several contexts, and knowing more about the platforms than I do.

May I commit this diff as-is, without Triple::OSType::DriverKit, and leave that to you?

cishida added inline comments.Aug 13 2020, 11:02 PM
llvm/lib/TextAPI/MachO/Platform.cpp
52

SGTM! In the revision that lands, could you just include driverkit in the TODO comment?

gkm updated this revision to Diff 285723.Aug 14 2020, 12:05 PM

supplement TODO comment

cishida accepted this revision.Aug 14 2020, 12:34 PM
This revision was landed with ongoing or failed builds.Aug 14 2020, 12:36 PM
This revision was automatically updated to reflect the committed changes.