[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

(Our workaround is to call __builtin_available() once before engaging the 
sandbox, which isn't so bad. Just thought I'd let you know about it; this isn't 
a serious bug for us.)


Repository:
  rL LLVM

https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Looks like the email reply didn't make it to phab, so here it is again:

It's in this program, which is pretty stand-alone: 
https://cs.chromium.org/chromium/src/chrome/utility/safe_browsing/mac/crdmg.cc?q=crdmg=package:chromium=95
 EnableSandbox() is on line 134. clang, compiler-rt are trunk from 2 weeks ago, 
SDK is 10.12, os 10.12.5. I don't think the particular version numbers matter 
too much though. Here's a standalone demo:

thakis-macpro:src thakis$ cat foo.cc
#include 
int main() {

  const char sbox[] = "(version 1) (deny default)";
  char* err;
  ::sandbox_init(sbox, 0, );
  if (__builtin_available(macos 10.10, *))
return 32;
  else
return 14;

}
thakis-macpro:src thakis$ third_party/llvm-build/Release+Asserts/bin/clang -o 
foo foo.cc -isysroot $(xcrun -show-sdk-path) -mmacosx-version-min=10.9 -w && 
./foo
thakis-macpro:src thakis$ echo $?
14
thakis-macpro:src thakis$ sw_vers -productVersion
10.12.5

After running that, look for "sandbox" in console.app to find the "deny 
file-read-data".


Repository:
  rL LLVM

https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Nico Weber via cfe-commits]

On Thu, Aug 3, 2017 at 4:13 AM, Alex Lorenz via Phabricator via cfe-commits
 wrote:

> arphaman added a comment.
>
> In https://reviews.llvm.org/D27827#829661, @thakis wrote:
>
> > We just noticed that if you call __builtin_available() for the first
> time after activating your app's sandbox, the function will fail:
> >
> > SandboxViolation: crdmg(15489) deny file-read-data
> /System/Library/CoreServices/SystemVersion.plist
> >  Violation:   deny file-read-data /System/Library/CoreServices/S
> ystemVersion.plist
> >  Process: crdmg [15489]
> >  Path:/Volumes/Build/src/./out/release/crdmg
> >
> > Thread 0 (id: 421251):
> >  0   libsystem_kernel.dylib   0x7fffe94a1a86 __open_nocancel
> + 10
> >  1   crdmg0x00010444be98
> parseSystemVersionPList + 360
> >  20xec83485354415541
>
>
> Hmm, never saw this before. Please post your exact configuration -
> clang/compiler-rt versions, OS version, toolchain & SDK. Is it possible to
> get a reproducer?
>

It's in this program, which is pretty stand-alone: https://cs.
chromium.org/chromium/src/chrome/utility/safe_browsing/
mac/crdmg.cc?q=crdmg=package:chromium=95 EnableSandbox() is on line
134. clang, compiler-rt are trunk from 2 weeks ago, SDK is 10.12, os
10.12.5. I don't think the particular version numbers matter too much
though. Here's a standalone demo:

thakis-macpro:src thakis$ cat foo.cc
#include 
int main() {
  const char sbox[] = "(version 1) (deny default)";
  char* err;
  ::sandbox_init(sbox, 0, );
  if (__builtin_available(macos 10.10, *))
return 32;
  else
return 14;
}
thakis-macpro:src thakis$ third_party/llvm-build/Release+Asserts/bin/clang
-o foo foo.cc -isysroot $(xcrun -show-sdk-path) -mmacosx-version-min=10.9
-w && ./foo
thakis-macpro:src thakis$ echo $?
14
thakis-macpro:src thakis$ sw_vers -productVersion
10.12.5

After running that, look for "sandbox" in console.app to find the "deny
file-read-data".


>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D27827
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D27827#829661, @thakis wrote:

> We just noticed that if you call __builtin_available() for the first time 
> after activating your app's sandbox, the function will fail:
>
> SandboxViolation: crdmg(15489) deny file-read-data 
> /System/Library/CoreServices/SystemVersion.plist
>  Violation:   deny file-read-data 
> /System/Library/CoreServices/SystemVersion.plist 
>  Process: crdmg [15489]
>  Path:/Volumes/Build/src/./out/release/crdmg
>
> Thread 0 (id: 421251):
>  0   libsystem_kernel.dylib   0x7fffe94a1a86 __open_nocancel + 10
>  1   crdmg0x00010444be98 
> parseSystemVersionPList + 360
>  20xec83485354415541


Hmm, never saw this before. Please post your exact configuration - 
clang/compiler-rt versions, OS version, toolchain & SDK. Is it possible to get 
a reproducer?


Repository:
  rL LLVM

https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

We just noticed that if you call __builtin_available() for the first time after 
activating your app's sandbox, the function will fail:

SandboxViolation: crdmg(15489) deny file-read-data 
/System/Library/CoreServices/SystemVersion.plist
Violation:   deny file-read-data 
/System/Library/CoreServices/SystemVersion.plist 
Process: crdmg [15489]
Path:/Volumes/Build/src/./out/release/crdmg

Thread 0 (id: 421251):
0   libsystem_kernel.dylib  0x7fffe94a1a86 __open_nocancel + 10
1   crdmg   0x00010444be98 
parseSystemVersionPList + 360
2   0xec83485354415541


Repository:
  rL LLVM

https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL296015: [ObjC][CodeGen] CodeGen support for @available. 
(authored by epilk).

Changed prior to commit:
  https://reviews.llvm.org/D27827?vs=89305=89556#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27827

Files:
  cfe/trunk/lib/CodeGen/CGExprScalar.cpp
  cfe/trunk/lib/CodeGen/CGObjC.cpp
  cfe/trunk/lib/CodeGen/CodeGenFunction.h
  cfe/trunk/lib/CodeGen/CodeGenModule.h
  cfe/trunk/test/CodeGenObjC/availability-check.m

Index: cfe/trunk/test/CodeGenObjC/availability-check.m
===
--- cfe/trunk/test/CodeGenObjC/availability-check.m
+++ cfe/trunk/test/CodeGenObjC/availability-check.m
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.11 -emit-llvm -o - %s | FileCheck %s
+
+void use_at_available() {
+  // CHECK: call i32 @__isOSVersionAtLeast(i32 10, i32 12, i32 0)
+  // CHECK-NEXT: icmp ne
+  if (__builtin_available(macos 10.12, *))
+;
+
+  // CHECK: call i32 @__isOSVersionAtLeast(i32 10, i32 12, i32 0)
+  // CHECK-NEXT: icmp ne
+  if (@available(macos 10.12, *))
+;
+
+  // CHECK: call i32 @__isOSVersionAtLeast(i32 10, i32 12, i32 42)
+  // CHECK-NEXT: icmp ne
+  if (__builtin_available(ios 10, macos 10.12.42, *))
+;
+
+  // CHECK-NOT: call i32 @__isOSVersionAtLeast
+  // CHECK: br i1 true
+  if (__builtin_available(ios 10, *))
+;
+
+  // This check should be folded: our deployment target is 10.11.
+  // CHECK-NOT: call i32 @__isOSVersionAtLeast
+  // CHECK: br i1 true
+  if (__builtin_available(macos 10.11, *))
+;
+}
+
+// CHECK: declare i32 @__isOSVersionAtLeast(i32, i32, i32)
Index: cfe/trunk/lib/CodeGen/CGObjC.cpp
===
--- cfe/trunk/lib/CodeGen/CGObjC.cpp
+++ cfe/trunk/lib/CodeGen/CGObjC.cpp
@@ -3399,5 +3399,21 @@
   return Val;
 }
 
+llvm::Value *
+CodeGenFunction::EmitBuiltinAvailable(ArrayRef Args) {
+  assert(Args.size() == 3 && "Expected 3 argument here!");
+
+  if (!CGM.IsOSVersionAtLeastFn) {
+llvm::FunctionType *FTy =
+llvm::FunctionType::get(Int32Ty, {Int32Ty, Int32Ty, Int32Ty}, false);
+CGM.IsOSVersionAtLeastFn =
+CGM.CreateRuntimeFunction(FTy, "__isOSVersionAtLeast");
+  }
+
+  llvm::Value *CallRes =
+  EmitNounwindRuntimeCall(CGM.IsOSVersionAtLeastFn, Args);
+
+  return Builder.CreateICmpNE(CallRes, llvm::Constant::getNullValue(Int32Ty));
+}
 
 CGObjCRuntime::~CGObjCRuntime() {}
Index: cfe/trunk/lib/CodeGen/CodeGenFunction.h
===
--- cfe/trunk/lib/CodeGen/CodeGenFunction.h
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.h
@@ -3169,6 +3169,8 @@
 public:
   llvm::Value *EmitMSVCBuiltinExpr(MSVCIntrin BuiltinID, const CallExpr *E);
 
+  llvm::Value *EmitBuiltinAvailable(ArrayRef Args);
+
   llvm::Value *EmitObjCProtocolExpr(const ObjCProtocolExpr *E);
   llvm::Value *EmitObjCStringLiteral(const ObjCStringLiteral *E);
   llvm::Value *EmitObjCBoxedExpr(const ObjCBoxedExpr *E);
Index: cfe/trunk/lib/CodeGen/CodeGenModule.h
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.h
+++ cfe/trunk/lib/CodeGen/CodeGenModule.h
@@ -546,6 +546,10 @@
 return *ObjCData;
   }
 
+  // Version checking function, used to implement ObjC's @available:
+  // i32 @__isOSVersionAtLeast(i32, i32, i32)
+  llvm::Constant *IsOSVersionAtLeastFn = nullptr;
+
   InstrProfStats () { return PGOStats; }
   llvm::IndexedInstrProfReader *getPGOReader() const { return PGOReader.get(); }
 
Index: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
===
--- cfe/trunk/lib/CodeGen/CGExprScalar.cpp
+++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp
@@ -300,6 +300,24 @@
 return V;
   }
 
+  Value *VisitObjCAvailabilityCheckExpr(ObjCAvailabilityCheckExpr *E) {
+VersionTuple Version = E->getVersion();
+
+// If we're checking for a platform older than our minimum deployment
+// target, we can fold the check away.
+if (Version <= CGF.CGM.getTarget().getPlatformMinVersion())
+  return llvm::ConstantInt::get(Builder.getInt1Ty(), 1);
+
+Optional Min = Version.getMinor(), SMin = Version.getSubminor();
+llvm::Value *Args[] = {
+llvm::ConstantInt::get(CGF.CGM.Int32Ty, Version.getMajor()),
+llvm::ConstantInt::get(CGF.CGM.Int32Ty, Min ? *Min : 0),
+llvm::ConstantInt::get(CGF.CGM.Int32Ty, SMin ? *SMin : 0),
+};
+
+return CGF.EmitBuiltinAvailable(Args);
+  }
+
   Value *VisitArraySubscriptExpr(ArraySubscriptExpr *E);
   Value *VisitShuffleVectorExpr(ShuffleVectorExpr *E);
   Value *VisitConvertVectorExpr(ConvertVectorExpr *E);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Thanks for the explanation, now I get it!

LGTM, with one request for change in the tests




Comment at: test/CodeGenObjC/availability-check.m:22
+  // CHECK: br i1 true
+  if (__builtin_available(macos 10.11, *))
+;

Please add a line that verifies that `@available()` uses the builtin as well. 


https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.



Comment at: test/CodeGenObjC/availability-check.m:16
+  // CHECK: br i1 true
+  if (__builtin_available(ios 10, *))
+;

arphaman wrote:
> Shouldn't this be `br i1 false`, since we are building for macOS so we have 
> no iOS support at all?
No, this is intentional. If the platform we're targeting isn't mentioned, we 
take the `*` case, and emit -Wunguarded-availability diagnostics in the body of 
the `if` using the minimum deployment target. The idea is that if a new OS is 
released it will be forked from an existing one and use existing APIs, and it 
would be unfortunate for everyone to have to add the new platform to their 
existing `@available` calls. This is probably the most counterintuative part of 
this feature, and is the reason for the somewhat bizarre `*` syntax, to call 
out this control flow.


https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: test/CodeGenObjC/availability-check.m:16
+  // CHECK: br i1 true
+  if (__builtin_available(ios 10, *))
+;

Shouldn't this be `br i1 false`, since we are building for macOS so we have no 
iOS support at all?


https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 89305.
erik.pilkington added a comment.

This new patch addresses all of Alex's comments:

- Remove `ObjC` from function names
- Rename `_IsOSVersionAtLeast` -> `__isOSVersionAtLeast`
- Improve testcase


https://reviews.llvm.org/D27827

Files:
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CGObjC.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenModule.h
  test/CodeGenObjC/availability-check.m

Index: test/CodeGenObjC/availability-check.m
===
--- test/CodeGenObjC/availability-check.m
+++ test/CodeGenObjC/availability-check.m
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.11 -emit-llvm -o - %s | FileCheck %s
+
+void use_at_available() {
+  // CHECK: call i32 @__isOSVersionAtLeast(i32 10, i32 12, i32 0)
+  // CHECK-NEXT: icmp ne
+  if (__builtin_available(macos 10.12, *))
+;
+
+  // CHECK: call i32 @__isOSVersionAtLeast(i32 10, i32 12, i32 42)
+  // CHECK-NEXT: icmp ne
+  if (__builtin_available(ios 10, macos 10.12.42, *))
+;
+
+  // CHECK-NOT: call i32 @__isOSVersionAtLeast
+  // CHECK: br i1 true
+  if (__builtin_available(ios 10, *))
+;
+
+  // This check should be folded: our deployment target is 10.11.
+  // CHECK-NOT: call i32 @__isOSVersionAtLeast
+  // CHECK: br i1 true
+  if (__builtin_available(macos 10.11, *))
+;
+}
+
+// CHECK: declare i32 @__isOSVersionAtLeast(i32, i32, i32)
Index: lib/CodeGen/CodeGenModule.h
===
--- lib/CodeGen/CodeGenModule.h
+++ lib/CodeGen/CodeGenModule.h
@@ -546,6 +546,10 @@
 return *ObjCData;
   }
 
+  // Version checking function, used to implement ObjC's @available:
+  // i32 @__isOSVersionAtLeast(i32, i32, i32)
+  llvm::Constant *IsOSVersionAtLeastFn = nullptr;
+
   InstrProfStats () { return PGOStats; }
   llvm::IndexedInstrProfReader *getPGOReader() const { return PGOReader.get(); }
 
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -3168,6 +3168,8 @@
 public:
   llvm::Value *EmitMSVCBuiltinExpr(MSVCIntrin BuiltinID, const CallExpr *E);
 
+  llvm::Value *EmitBuiltinAvailable(ArrayRef Args);
+
   llvm::Value *EmitObjCProtocolExpr(const ObjCProtocolExpr *E);
   llvm::Value *EmitObjCStringLiteral(const ObjCStringLiteral *E);
   llvm::Value *EmitObjCBoxedExpr(const ObjCBoxedExpr *E);
Index: lib/CodeGen/CGObjC.cpp
===
--- lib/CodeGen/CGObjC.cpp
+++ lib/CodeGen/CGObjC.cpp
@@ -3399,5 +3399,21 @@
   return Val;
 }
 
+llvm::Value *
+CodeGenFunction::EmitBuiltinAvailable(ArrayRef Args) {
+  assert(Args.size() == 3 && "Expected 3 argument here!");
+
+  if (!CGM.IsOSVersionAtLeastFn) {
+llvm::FunctionType *FTy =
+llvm::FunctionType::get(Int32Ty, {Int32Ty, Int32Ty, Int32Ty}, false);
+CGM.IsOSVersionAtLeastFn =
+CGM.CreateRuntimeFunction(FTy, "__isOSVersionAtLeast");
+  }
+
+  llvm::Value *CallRes =
+  EmitNounwindRuntimeCall(CGM.IsOSVersionAtLeastFn, Args);
+
+  return Builder.CreateICmpNE(CallRes, llvm::Constant::getNullValue(Int32Ty));
+}
 
 CGObjCRuntime::~CGObjCRuntime() {}
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -300,6 +300,24 @@
 return V;
   }
 
+  Value *VisitObjCAvailabilityCheckExpr(ObjCAvailabilityCheckExpr *E) {
+VersionTuple Version = E->getVersion();
+
+// If we're checking for a platform older than our minimum deployment
+// target, we can fold the check away.
+if (Version <= CGF.CGM.getTarget().getPlatformMinVersion())
+  return llvm::ConstantInt::get(Builder.getInt1Ty(), 1);
+
+Optional Min = Version.getMinor(), SMin = Version.getSubminor();
+llvm::Value *Args[] = {
+llvm::ConstantInt::get(CGF.CGM.Int32Ty, Version.getMajor()),
+llvm::ConstantInt::get(CGF.CGM.Int32Ty, Min ? *Min : 0),
+llvm::ConstantInt::get(CGF.CGM.Int32Ty, SMin ? *SMin : 0),
+};
+
+return CGF.EmitBuiltinAvailable(Args);
+  }
+
   Value *VisitArraySubscriptExpr(ArraySubscriptExpr *E);
   Value *VisitShuffleVectorExpr(ShuffleVectorExpr *E);
   Value *VisitConvertVectorExpr(ConvertVectorExpr *E);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Duncan P. N. Exon Smith via cfe-commits]

> On 2017-Feb-20, at 13:11, Alex Lorenz via Phabricator 
>  wrote:
> 
> arphaman added inline comments.
> 
> 
> 
> Comment at: lib/CodeGen/CodeGenFunction.h:2479
> 
> +  llvm::Value *EmitObjCIsOSVersionAtLeast(ArrayRef Args);
> +
> 
> I think it's better to treat this as a builtin in its own right, without 
> including the ObjC part in the names. This function could be renamed to 
> something like `EmitBuiltinAvailable` and the variable that holds the 
> function pointer should be renamed appropriately as well.
> 
> 
> 
> Comment at: test/CodeGenObjC/availability-check.m:5
> +void use_at_available() {
> +  // CHECK: call i32 @_IsOSVersionAtLeast(i32 10, i32 12, i32 0)
> +  // CHECK-NEXT: icmp ne
> 
> I think the function name should have the lowercase `is`

Keep in mind that we need this to be a reserved identifier, so it either has to 
start with two underscores or one underscore with an uppercase letter.  In 
other words, don't forget to add an underscore if you lowercase it.

> 
> 
> 
> Comment at: test/CodeGenObjC/availability-check.m:7
> +  // CHECK-NEXT: icmp ne
> +  if (__builtin_available(macos 10.12, *))
> +;
> 
> Can you add a call to the builtin that has a non-zero sub-minor version as 
> well?
> 
> 
> https://reviews.llvm.org/D27827
> 
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: lib/CodeGen/CodeGenFunction.h:2479
 
+  llvm::Value *EmitObjCIsOSVersionAtLeast(ArrayRef Args);
+

I think it's better to treat this as a builtin in its own right, without 
including the ObjC part in the names. This function could be renamed to 
something like `EmitBuiltinAvailable` and the variable that holds the function 
pointer should be renamed appropriately as well.



Comment at: test/CodeGenObjC/availability-check.m:5
+void use_at_available() {
+  // CHECK: call i32 @_IsOSVersionAtLeast(i32 10, i32 12, i32 0)
+  // CHECK-NEXT: icmp ne

I think the function name should have the lowercase `is`



Comment at: test/CodeGenObjC/availability-check.m:7
+  // CHECK-NEXT: icmp ne
+  if (__builtin_available(macos 10.12, *))
+;

Can you add a call to the builtin that has a non-zero sub-minor version as well?


https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 89067.
erik.pilkington added a comment.

This new patch just generates a call to the function `_IsOSVersionAtLeast` and 
branches on the result. `_IsOSVersionAtLeast` is going through review here: 
https://reviews.llvm.org/D30136.

I decided to parse the SystemVersion.plist file, I think mapping from kernel 
versions to marketing versions works for macOS, but not for iOS, as iOS tends 
to cling on to darwin versions for longer. For example, darwin 14.0.0 was used 
from iOS 7.0 until iOS 8.4.1, (based on the table here: 
https://www.theiphonewiki.com/wiki/Kernel) which is not fine grain enough for 
use with `@available`.

Thanks for taking a look!
Erik


https://reviews.llvm.org/D27827

Files:
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CGObjC.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenModule.h
  test/CodeGenObjC/availability-check.m

Index: test/CodeGenObjC/availability-check.m
===
--- test/CodeGenObjC/availability-check.m
+++ test/CodeGenObjC/availability-check.m
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -xc -triple x86_64-apple-macosx10.11 -emit-llvm -o - %s | FileCheck %s
+
+void use_at_available() {
+  // CHECK: call i32 @_IsOSVersionAtLeast(i32 10, i32 12, i32 0)
+  // CHECK-NEXT: icmp ne
+  if (__builtin_available(macos 10.12, *))
+;
+  // This check should be folded: our deployment target is 10.11.
+  // CHECK-NOT: call i32 @_IsOSVersionAtLeast
+  if (__builtin_available(macos 10.11, *))
+;
+}
+
+// CHECK: declare i32 @_IsOSVersionAtLeast(i32, i32, i32)
Index: lib/CodeGen/CodeGenModule.h
===
--- lib/CodeGen/CodeGenModule.h
+++ lib/CodeGen/CodeGenModule.h
@@ -546,6 +546,10 @@
 return *ObjCData;
   }
 
+  // Version checking function, used to implement ObjC's @available:
+  // i32 @_IsOSVersionAtLeast(i32, i32, i32)
+  llvm::Constant *ObjCIsOSVersionAtLeastFn = nullptr;
+
   InstrProfStats () { return PGOStats; }
   llvm::IndexedInstrProfReader *getPGOReader() const { return PGOReader.get(); }
 
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2476,6 +2476,8 @@
   void EmitObjCAtSynchronizedStmt(const ObjCAtSynchronizedStmt );
   void EmitObjCAutoreleasePoolStmt(const ObjCAutoreleasePoolStmt );
 
+  llvm::Value *EmitObjCIsOSVersionAtLeast(ArrayRef Args);
+
   void EmitCoroutineBody(const CoroutineBodyStmt );
   RValue EmitCoroutineIntrinsic(const CallExpr *E, unsigned int IID);
 
Index: lib/CodeGen/CGObjC.cpp
===
--- lib/CodeGen/CGObjC.cpp
+++ lib/CodeGen/CGObjC.cpp
@@ -3399,5 +3399,21 @@
   return Val;
 }
 
+llvm::Value *
+CodeGenFunction::EmitObjCIsOSVersionAtLeast(ArrayRef Args) {
+  assert(Args.size() == 3 && "Expected 3 argument here!");
+
+  if (!CGM.ObjCIsOSVersionAtLeastFn) {
+llvm::FunctionType *FTy =
+llvm::FunctionType::get(Int32Ty, {Int32Ty, Int32Ty, Int32Ty}, false);
+CGM.ObjCIsOSVersionAtLeastFn =
+CGM.CreateRuntimeFunction(FTy, "_IsOSVersionAtLeast");
+  }
+
+  llvm::Value *CallRes =
+  EmitNounwindRuntimeCall(CGM.ObjCIsOSVersionAtLeastFn, Args);
+
+  return Builder.CreateICmpNE(CallRes, llvm::Constant::getNullValue(Int32Ty));
+}
 
 CGObjCRuntime::~CGObjCRuntime() {}
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -300,6 +300,24 @@
 return V;
   }
 
+  Value *VisitObjCAvailabilityCheckExpr(ObjCAvailabilityCheckExpr *E) {
+VersionTuple Version = E->getVersion();
+
+// If we're checking for a platform older than our minimum deployment
+// target, we can fold the check away.
+if (Version <= CGF.CGM.getTarget().getPlatformMinVersion())
+  return llvm::ConstantInt::get(Builder.getInt1Ty(), 1);
+
+Optional Min = Version.getMinor(), SMin = Version.getSubminor();
+llvm::Value *Args[] = {
+llvm::ConstantInt::get(CGF.CGM.Int32Ty, Version.getMajor()),
+llvm::ConstantInt::get(CGF.CGM.Int32Ty, Min ? *Min : 0),
+llvm::ConstantInt::get(CGF.CGM.Int32Ty, SMin ? *SMin : 0),
+};
+
+return CGF.EmitObjCIsOSVersionAtLeast(Args);
+  }
+
   Value *VisitArraySubscriptExpr(ArraySubscriptExpr *E);
   Value *VisitShuffleVectorExpr(ShuffleVectorExpr *E);
   Value *VisitConvertVectorExpr(ConvertVectorExpr *E);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D27827#625438, @erik.pilkington wrote:

> I seem to remember that mapping from kernel versions to marketing versions 
> was tossed around as a potential alternative to Gestalt. I think that the 
> problem was Apple sometimes introduces (or reserves the right to introduce) 
> new SDKs in a patch release (ie, Z in X.Y.Z), which wouldn't necessary imply 
> a new kernel version, and would still need to be queried by @available. This 
> makes it impossible to use kernel version in the general case (Or at least I 
> think, it would be nice if someone internal to Apple could confirm this?). 
> This rules out using `sysctl()` and the like.


This sounds like a legitimate concern, I'll try to get some information about 
this.

> AFAIK this just leaves `Gestalt()` and Objective-C's `NSProcessInfo`, the 
> latter would mean pulling in the Objective-C runtime, which would be 
> unfortunate for C users (using the `__builtin_available` spelling). I don't 
> think `Gestalt()` is in any danger of actually being removed, so we might as 
> well use it. The only alternative I know of to those would be manually 
> parsing the `SystemVersion.plist` XML file, but I think that might cause 
> problems with sandboxed processes (right?). Any thoughts here would be much 
> appreciated, `Gestalt()` is by no means a perfect solution.

Sandboxed applications are allowed to read that PLIST file, since 
https://developer.apple.com/library/content/documentation/Security/Conceptual/AppSandboxDesignGuide/AppSandboxInDepth/AppSandboxInDepth.html
 states the applications are permitted to read files that are world readable in 
`/System`. I confirmed that with a test sandboxed app as well. It would seem 
that reading that PLIST file is a good solution if we can get access to the 
PLIST APIs from compiler-rt (+ we can always fallback to `Gestalt`/`sysctl` if 
something goes wrong).

> Compiler-rt does seem like a good place it put this, should I move the 
> runtime code there instead?

Yes, please.

Btw, compiler-rt already has code that checks which version of macOS it's 
running on 
(https://github.com/llvm-project/compiler-rt/blob/master/lib/sanitizer_common/sanitizer_mac.cc#L410).
 It uses `sysctl` as well. I'm not sure if we can somehow refactor and reuse 
this code or not (since ours will be in builtins), but at least it's a good 
starting point.

> Thanks for taking a look!
> Erik


https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Nico Weber via cfe-commits]

https://developer.apple.com/library/content/releasenotes/General/CarbonCoreDeprecations/index.html#//apple_ref/doc/uid/TP40012224-CH1-SW16
explicitly suggests sysctl as replacement when targeting 10.8+, which
strongly suggests that it'll work.

On Fri, Dec 16, 2016 at 4:37 PM, Erik Pilkington via Phabricator <
revi...@reviews.llvm.org> wrote:

> erik.pilkington added a comment.
>
> I seem to remember that mapping from kernel versions to marketing versions
> was tossed around as a potential alternative to Gestalt. I think that the
> problem was Apple sometimes introduces (or reserves the right to introduce)
> new SDKs in a patch release (ie, Z in X.Y.Z), which wouldn't necessary
> imply a new kernel version, and would still need to be queried by
> @available. This makes it impossible to use kernel version in the general
> case (Or at least I think, it would be nice if someone internal to Apple
> could confirm this?). This rules out using `sysctl()` and the like.
>
> AFAIK this just leaves `Gestalt()` and Objective-C's `NSProcessInfo`, the
> latter would mean pulling in the Objective-C runtime, which would be
> unfortunate for C users (using the `__builtin_available` spelling). I don't
> think `Gestalt()` is in any danger of actually being removed, so we might
> as well use it. The only alternative I know of to those would be manually
> parsing the `SystemVersion.plist` XML file, but I think that might cause
> problems with sandboxed processes (right?). Any thoughts here would be much
> appreciated, `Gestalt()` is by no means a perfect solution.
>
> Compiler-rt does seem like a good place it put this, should I move the
> runtime code there instead?
>
> Thanks for taking a look!
> Erik
>
>
> https://reviews.llvm.org/D27827
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

I seem to remember that mapping from kernel versions to marketing versions was 
tossed around as a potential alternative to Gestalt. I think that the problem 
was Apple sometimes introduces (or reserves the right to introduce) new SDKs in 
a patch release (ie, Z in X.Y.Z), which wouldn't necessary imply a new kernel 
version, and would still need to be queried by @available. This makes it 
impossible to use kernel version in the general case (Or at least I think, it 
would be nice if someone internal to Apple could confirm this?). This rules out 
using `sysctl()` and the like.

AFAIK this just leaves `Gestalt()` and Objective-C's `NSProcessInfo`, the 
latter would mean pulling in the Objective-C runtime, which would be 
unfortunate for C users (using the `__builtin_available` spelling). I don't 
think `Gestalt()` is in any danger of actually being removed, so we might as 
well use it. The only alternative I know of to those would be manually parsing 
the `SystemVersion.plist` XML file, but I think that might cause problems with 
sandboxed processes (right?). Any thoughts here would be much appreciated, 
`Gestalt()` is by no means a perfect solution.

Compiler-rt does seem like a good place it put this, should I move the runtime 
code there instead?

Thanks for taking a look!
Erik


https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

I don't think that generating calls to `dispatch_once` and `Gestalt` is the 
right approach. Swift generates a call to `_stdlib_isOSVersionAtLeast`, so 
maybe we could add some function like that into compiler-rt?

`Gestalt` is also deprecated, we should use something better. Swift's 
`_stdlib_isOSVersionAtLeast` calls `_swift_stdlib_operatingSystemVersion` 
(https://github.com/apple/swift/blob/2fe4254cb712fa101a220f95b6ade8f99f43dc74/stdlib/public/core/Availability.swift)
 which in turn either calls `NSProcessInfo.operatingSystemVersion` or loads the 
version from a PLIST file 
(https://github.com/apple/swift/blob/3328592c20ac51a7c525a439af778a75521ab781/stdlib/public/stubs/Availability.mm).
 I don't think we can use Objective-C in compiler-rt though, so we probably 
have to use another solution. Maybe use `sysctlbyname("kern.osrelease", ...)` 
that returns the version of the kernel, which can be remapped into the OS 
version (although that seems hacky)?




Comment at: lib/CodeGen/CGExprScalar.cpp:305
+VersionTuple Version = E->getVersion();
+Optional Min = Version.getMinor(), SMin = Version.getSubminor();
+

You can move this line after the `if` below since these `Min` and `SMin` are 
used only after the if.


https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Huh, I'm surprised this has a runtime component, I missed that. I thought this 
would be a statically-checked thing. But having this does seem useful :-)

I'm also surprised that this compiles down to Gestalt – Gestalt is deprecated 
in newer SDKs, and in Chromium we invested some time to move off of it when 
recently bumping our min SDK target. So it seems strange the compiler would add 
a dependency to it. (We've also observed that Gestalt is pretty heavy-weight at 
least in macOS 10.6 in that it can spawn threads).

Things we've done instead, in various contexts:

- `uname()` 
(https://cs.chromium.org/chromium/src/base/mac/mac_util.mm?q=MacOSXMinorVersion=package:chromium=C=365)
- -[NSProcessInfo operatingSystemVersion] if it's available (it's documented 
available in 10.10+, but actually exists from 10.9.2+) (rhs on 
https://codereview.chromium.org/1842213002/diff/40001/base/sys_info_mac.mm)
- `sysctl()` 
https://github.com/google/google-toolbox-for-mac/blob/master/Foundation/GTMServiceManagement.c#L63
 That's what's recommended but might need 10.8


https://reviews.llvm.org/D27827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27827: [ObjC] CodeGen support for @available on macOS

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington created this revision.
erik.pilkington added reviewers: manmanren, dexonsmith, rjmccall, thakis.
erik.pilkington added a subscriber: cfe-commits.

This patch adds CodeGen support for `@available` on macos. This is done by 
compiling @available predicates into calls to `clang.is_os_version_at_least`. 
This function first populates 3 global variables which hold the major, minor 
and subminor with the operating system version, using core services' 
`Gestalt()` 
(https://developer.apple.com/reference/coreservices/1471624-gestalt?language=objc),
 and grand central dispatch's `dispatch_once_f` to make sure that this is only 
done once. After that, it compares the version that was passed to it with the 
host's OS version.

Unfortunately, `Gestalt()` is only available on macOS. In a follow-up patch, we 
can add support for other platforms. I think that the best way of doing that is 
to depend on Objective-C's `NSProcessInfo`. I'm not super familiar with this 
part of the compiler, and in particular I'm unsure of whether we should be 
directly calling into library functions like `Gestalt` and `dispatch_once_f`, 
so please tear this patch apart!

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

Thanks,
Erik


https://reviews.llvm.org/D27827

Files:
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CGObjC.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenModule.h
  test/CodeGenObjC/availability-check.m
  test/CodeGenObjC/availability-check2.m

Index: test/CodeGenObjC/availability-check2.m
===
--- test/CodeGenObjC/availability-check2.m
+++ test/CodeGenObjC/availability-check2.m
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.11 -emit-llvm -o - %s | FileCheck %s
+
+// Test that we behave correctly if the user already imported the libraries we
+// depend on to implement @available.
+
+// CHECK: @clang.os_version_major = linkonce_odr global i32 0
+// CHECK: @clang.os_version_minor = linkonce_odr global i32 0
+// CHECK: @clang.os_version_subminor = linkonce_odr global i32 0
+// CHECK: @clang.os_version_check_dispatch_once_counter = linkonce_odr global i64 0
+
+short Gestalt(int, int *);
+void dispatch_once_f(long long *, char *, void (*)(char *));
+
+void actually_emit() {
+  (void)Gestalt;
+  (void)dispatch_once_f;
+}
+
+// CHECK: declare signext i16 @Gestalt(i32, i32*)
+// CHECK: declare void @dispatch_once_f(i64*, i8*, void (i8*)*)
+
+void use_at_available() {
+  if (@available(macos 10.12, *))
+;
+  // CHECK: call i1 @clang.is_os_version_at_least(i32 10, i32 12, i32 0)
+}
+
+// CHECK-NOT: declare signext i16 @Gestalt(i32, i32*)
+// CHECK-NOT: declare void @dispatch_once_f(i64*, i8*, void (i8*)*)
+
+// CHECK: define linkonce_odr void @clang.populate_os_version(i8*)
+// CHECK: define linkonce_odr i1 @clang.is_os_version_at_least(i32, i32, i32)
Index: test/CodeGenObjC/availability-check.m
===
--- test/CodeGenObjC/availability-check.m
+++ test/CodeGenObjC/availability-check.m
@@ -0,0 +1,57 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -xc -triple x86_64-apple-macosx10.11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK:  @clang.os_version_major = linkonce_odr global i32 0
+// CHECK:  @clang.os_version_minor = linkonce_odr global i32 0
+// CHECK:  @clang.os_version_subminor = linkonce_odr global i32 0
+// CHECK:  @clang.os_version_check_dispatch_once_counter = linkonce_odr global i64 0
+
+void use_at_available() {
+  if (__builtin_available(macos 10.12, *))
+;
+  // CHECK: call i1 @clang.is_os_version_at_least(i32 10, i32 12, i32 0)
+
+  // Test that we constant fold the check if our OS version is older than our
+  // deployment target:
+  if (__builtin_available(macos 10.11, *))
+;
+  // CHECK-NOT: call i1 @clang.is_os_version_at_least
+  // CHECK: br i1 true
+}
+
+// CHECK:  define linkonce_odr void @clang.populate_os_version(i8*) {
+// CHECK-NEXT:   call i16 @Gestalt(i32 1937339185, i32* @clang.os_version_major)
+// CHECK-NEXT:   call i16 @Gestalt(i32 1937339186, i32* @clang.os_version_minor)
+// CHECK-NEXT:   call i16 @Gestalt(i32 1937339187, i32* @clang.os_version_subminor)
+// CHECK-NEXT:   ret void
+// CHECK-NEXT: }
+
+// CHECK: declare signext i16 @Gestalt(i32, i32*)
+// CHECK: declare void @dispatch_once_f(i64*, i8*, void (i8*)*)
+
+// CHECK:  define linkonce_odr i1 @clang.is_os_version_at_least(i32, i32, i32) {
+// CHECK:call void @dispatch_once_f(i64* @clang.os_version_check_dispatch_once_counter, i8* null, void (i8*)* @clang.populate_os_version)
+// entry:
+// CHECK:[[T0:%.*]] = load i32, i32* @clang.os_version_major, align 4
+// CHECK-NEXT:   [[T1:%.*]] = icmp slt i32 %0, [[T0]]
+// CHECK-NEXT:   br i1 [[T1]],
+// major_cmp:
+// CHECK: