This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][builtins][WIP] Add _IsOSVersionAtLeast, to be used by ObjC's @available
ClosedPublic

Authored by erik.pilkington on Feb 18 2017, 10:40 AM.

Details

Summary

This patch adds the function _IsOSVersionAtLeast, which uses of Objective-C's @available will compile into. This function uses the Core Foundation API to parse the SystemVersion.plist file, which contains the marketing OS version, which is used by @available, as recommended here: https://reviews.llvm.org/D27827.

The API that I used for this requires at least macOS 10.6/iOS 4.0. Is this far enough back? We could provide to a conservative mapping from darwin versions to marketing versions (as suggested by Alex Lorenz) if this is too new.

Inb4 this patch doesn't have a testcase! I can't figure out how to run the builtins tests, looks like they're not attached to the cmake check-all target, as mentioned in test/CMakeLists.txt:8, and the shell script at test/builtins/Unit/test errors out on me (it can't find the darwin_fat directory), and looks like it hasn't been touched since 2010. How are these tests run? Is there any place where this is documented?

I did test this on my own machine, and works as intended on iOS simulators and macOS.

This patch is part of a feature I proposed last summer:
http://lists.llvm.org/pipermail/cfe-dev/2016-July/049851.html

Thanks for taking a look!
Erik

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman edited edge metadata.Feb 20 2017, 9:18 AM

Thanks for taking your time to work on this!

We've discussed the best approach internally and came to the conclusion that Gestalt should be used to determine the version of the OS even though it's officially deprecated. I'm sorry if my previous comments in which I dismissed Gestalt made you do extra work, I wasn't aware of some nuances back then. Can you please rewrite this function to use Gestalt?

@arphaman: AFAIK Gestalt() is only present on macOS (https://developer.apple.com/reference/coreservices/1471624-gestalt?language=objc), so for iOS and the like we'll need to fall back to parsing the SystemVersion.plist file. Is there any reason to prefer special casing macOS to use Gestalt?

You're right, sorry about the confusion :)
I guess the plist approach should be used on macOS as well then.

We need to add a test for this function as well, can you try to create one in test/builtins/Unit?

lib/builtins/os_version_check.c
31 ↗(On Diff #89027)

NIT: The function name can be lowercase

61 ↗(On Diff #89027)

Please add a check that verifies that malloc succeeded.

81 ↗(On Diff #89027)

Please add a check to see if this call fails.

94 ↗(On Diff #89027)

The function should be renamed to match the new name emitted by clang.

erik.pilkington marked 4 inline comments as done.

This new patch addresses Alex's comments:

  • Add more failure checks
  • Rename _IsOSVersionAtLeast -> __isOSVersionAtLeast

Additionally:

  • fall back to the function CFPropertyListCreateFromXMLData if CFPropertyListCreateWithData is unavailable.

@arphaman: As far as adding a test to test/builtins/Unit, I would like to but I don't know how these tests are run. I went through my trouble with it in the description above. Do you know how these tests are run?

Thanks for taking a look!

Just realized I did a if (!&CFLongFunctionName) when I meant to do a if (&CFLongFunctionName). Oops!

@arphaman: As far as adding a test to test/builtins/Unit, I would like to but I don't know how these tests are run. I went through my trouble with it in the description above. Do you know how these tests are run?

It looks like the builtins didn't have cmake test support, but I've worked out a way to add it. I've attached a diff with a sample test that you can use:

I tested it on macOS only, so I'm not sure if the code is 100% correct.

I uploaded an empty diff by mistake, here's the test diff that I had in mind:

This new patch incorporates Alex Lorenz's testing infrastructure, and uses it to write a testcase.

arphaman accepted this revision.Mar 7 2017, 11:11 AM

LGTM. Can you please ensure that the modifications to the testing infrastructure won't cause failures on non-Darwin platforms (e.g. Linux) before committing?

This revision is now accepted and ready to land.Mar 7 2017, 11:11 AM

@arphaman: Sure, works as intended on debian8 x86_64. Thanks for all your help here!

This revision was automatically updated to reflect the committed changes.

Looks like it's failing to build compiler-rt for iOS: http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_build/31958/consoleFull . I'm investigating the issue.