[ObjC] Disallow vector parameters and return values in Objective-C methods on older X86 targets

[arphaman]

Summary

This patch adds a new error that disallows methods that have parameters/return values with a vector type on certain older X86 targets. This is diagnostic is needed because objc_msgSend doesn't support SIMD vector registers/return values on X86 in iOS < 9 and OS X < 10.11.

[erik.pilkington]

Hi Alex, thanks for CCing me!

include/clang/Basic/DiagnosticSemaKinds.td

1157 ↗

(On Diff #84276)

s/return value/return type/? Also, It would be nice to mention what isn't supported (vector types) and the version that it becomes supported in.

lib/Sema/SemaDeclObjC.cpp

4337 ↗

(On Diff #84276)

I wonder if this is the right place to check this. If we're compiling using a new SDK, but deploying back before vector types were supported we will diagnose a method declared in a header even if it is never called. We should probably hook this into DiagnoseAvailabilityOfDecl (in SemaExpr.cpp), which is called whenever a method is actually used, and diagnose it then.

[arphaman]

Use better diagnostic message as suggested by Erik

lib/Sema/SemaDeclObjC.cpp

4337 ↗

(On Diff #84276)

I'm not sure I get your argument.. We don't diagnose on method declarations, just method definitions, so I don't see how the header stuff is relevant here. We definitely don't want to diagnose on method uses either.

[arphaman]

Ping.

[bruno]

Hi Alex,

lib/Sema/SemaDeclObjC.cpp

4312 ↗

(On Diff #84564)

Assuming objc/c++ can pass/return these, the current check wouldn't be enough to cover x86_64 ABI cases where small (< 8 bytes) trivial classes containing vector types are directly passed / returned as vector types. You might want to check if that applies here.

See test/CodeGenCXX/x86_64-arguments-avx.cpp for examples (and x86_64 abi doc, item 3.2.3)

test/SemaObjC/x86-method-vector-values.m

16 ↗

(On Diff #84564)

Can you also add to the testcase instances that use regular non-ext vector (vector_type(...))?

[arphaman]

Add a test for non-ext vector type

lib/Sema/SemaDeclObjC.cpp

4312 ↗

(On Diff #84564)

It seems that on X86 all the troubling aggregates are passed by pointer anyway, and we don't warn for X86_64 at all. The only aggregates that seem to use registers for Objective-C methods are the ones that don't use vector registers.

[arphaman]

Ping

[bruno]

lib/Sema/SemaDeclObjC.cpp

4309 ↗

(On Diff #90832)

Maybe add an assert for Triple.getArch() == llvm::Triple::x86) here?

[arphaman]

Add an assertion as requested by Bruno.

[arphaman]

Ping.

[bruno]

Thanks Alex. LGTM

cfe/trunk/include/clang/AST/DeclBase.h

public:
   /// deprecated, or is unavailable.
   ///
   /// \param EnclosingVersion The version to compare with. If empty, assume the
   /// deployment target version.
   AvailabilityResult
   getAvailability(std::string *Message = nullptr,
                   VersionTuple EnclosingVersion = VersionTuple()) const;
 
+  /// \brief Retrieve the version of the target platform in which this
+  /// declaration was introduced.
+  ///
+  /// \returns An empty version tuple if this declaration has no 'introduced'
+  /// availability attributes, or the version tuple that's specified in the
+  /// attribute otherwise.
+  VersionTuple getVersionIntroduced() const;
+
   /// \brief Determine whether this declaration is marked 'deprecated'.
   ///
   /// \param Message If non-NULL and the declaration is deprecated,
   /// this will be set to the message describing why the declaration
   /// was deprecated (which may be empty).
   bool isDeprecated(std::string *Message = nullptr) const {
     return getAvailability(Message) == AR_Deprecated;
   }

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def warn_multiple_selectors: Warning<
   "for the @selector expression">,
   InGroup<SelectorTypeMismatch>, DefaultIgnore;
 
 def err_objc_kindof_nonobject : Error<
   "'__kindof' specifier cannot be applied to non-object type %0">;
 def err_objc_kindof_wrong_position : Error<
   "'__kindof' type specifier must precede the declarator">;
 
+def err_objc_method_unsupported_param_ret_type : Error<
+  "%0 %select{parameter|return}1 type is unsupported; "
+  "support for vector types for this target is introduced in %2">;
+
 // C++ declarations
 def err_static_assert_expression_is_not_constant : Error<
   "static_assert expression is not an integral constant expression">;
 def err_static_assert_failed : Error<"static_assert failed%select{ %1|}0">;
 def ext_static_assert_no_message : ExtWarn<
   "static_assert with no message is a C++1z extension">, InGroup<CXX1z>;
 def warn_cxx14_compat_static_assert_no_message : Warning<
   "static_assert with no message is incompatible with C++ standards before C++1z">,

cfe/trunk/lib/AST/DeclBase.cpp

 const Attr *Decl::getDefiningAttr() const {
   if (AliasAttr *AA = getAttr<AliasAttr>())
     return AA;
   if (IFuncAttr *IFA = getAttr<IFuncAttr>())
     return IFA;
   return nullptr;
 }
 
+StringRef getRealizedPlatform(const AvailabilityAttr *A,
+                              const ASTContext &Context) {
+  // Check if this is an App Extension "platform", and if so chop off
+  // the suffix for matching with the actual platform.
+  StringRef RealizedPlatform = A->getPlatform()->getName();
+  if (!Context.getLangOpts().AppExt)
+    return RealizedPlatform;
+  size_t suffix = RealizedPlatform.rfind("_app_extension");
+  if (suffix != StringRef::npos)
+    return RealizedPlatform.slice(0, suffix);
+  return RealizedPlatform;
+}
+
 /// \brief Determine the availability of the given declaration based on
 /// the target platform.
 ///
 /// When it returns an availability result other than \c AR_Available,
 /// if the \p Message parameter is non-NULL, it will be set to a
 /// string describing why the entity is unavailable.
 ///
 /// FIXME: Make these strings localizable, since they end up in
 /// diagnostics.
 static AvailabilityResult CheckAvailability(ASTContext &Context,
                                             const AvailabilityAttr *A,
                                             std::string *Message,
                                             VersionTuple EnclosingVersion) {
   if (EnclosingVersion.empty())
     EnclosingVersion = Context.getTargetInfo().getPlatformMinVersion();
 
   if (EnclosingVersion.empty())
     return AR_Available;
 
-  // Check if this is an App Extension "platform", and if so chop off
-  // the suffix for matching with the actual platform.
   StringRef ActualPlatform = A->getPlatform()->getName();
-  StringRef RealizedPlatform = ActualPlatform;
-  if (Context.getLangOpts().AppExt) {
-    size_t suffix = RealizedPlatform.rfind("_app_extension");
-    if (suffix != StringRef::npos)
-      RealizedPlatform = RealizedPlatform.slice(0, suffix);
-  }
-
   StringRef TargetPlatform = Context.getTargetInfo().getPlatformName();
 
   // Match the platform name.
-  if (RealizedPlatform != TargetPlatform)
+  if (getRealizedPlatform(A, Context) != TargetPlatform)
     return AR_Available;
 
   StringRef PrettyPlatformName
     = AvailabilityAttr::getPrettyPlatformName(ActualPlatform);
 
   if (PrettyPlatformName.empty())
     PrettyPlatformName = ActualPlatform;
 
for (const auto *A : attrs()) {
     }
   }
 
   if (Message)
     Message->swap(ResultMessage);
   return Result;
 }
 
+VersionTuple Decl::getVersionIntroduced() const {
+  const ASTContext &Context = getASTContext();
+  StringRef TargetPlatform = Context.getTargetInfo().getPlatformName();
+  for (const auto *A : attrs()) {
+    if (const auto *Availability = dyn_cast<AvailabilityAttr>(A)) {
+      if (getRealizedPlatform(Availability, Context) != TargetPlatform)
+        continue;
+      if (!Availability->getIntroduced().empty())
+        return Availability->getIntroduced();
+    }
+  }
+  return VersionTuple();
+}
+
 bool Decl::canBeWeakImported(bool &IsDefinition) const {
   IsDefinition = false;
 
   // Variables, if they aren't definitions.
   if (const VarDecl *Var = dyn_cast<VarDecl>(this)) {
     if (Var->isThisDeclarationADefinition()) {
       IsDefinition = true;
       return false;

cfe/trunk/lib/Sema/SemaDeclObjC.cpp

QualType newParamType
           S, param->getLocation(), param->getType(),
           param->getObjCDeclQualifier() & Decl::OBJC_TQ_CSNullability,
           prevParam->getLocation(), prevParam->getType(),
           prevParam->getObjCDeclQualifier() & Decl::OBJC_TQ_CSNullability);
     param->setType(newParamType);
   }
 }
 
+/// Verify that the method parameters/return value have types that are supported
+/// by the x86 target.
+static void checkObjCMethodX86VectorTypes(Sema &SemaRef,
+                                          const ObjCMethodDecl *Method) {
+  assert(SemaRef.getASTContext().getTargetInfo().getTriple().getArch() ==
+             llvm::Triple::x86 &&
+         "x86-specific check invoked for a different target");
+  SourceLocation Loc;
+  QualType T;
+  for (const ParmVarDecl *P : Method->parameters()) {
+    if (P->getType()->isVectorType()) {
+      Loc = P->getLocStart();
+      T = P->getType();
+      break;
+    }
+  }
+  if (Loc.isInvalid()) {
+    if (Method->getReturnType()->isVectorType()) {
+      Loc = Method->getReturnTypeSourceRange().getBegin();
+      T = Method->getReturnType();
+    } else
+      return;
+  }
+
+  // Vector parameters/return values are not supported by objc_msgSend on x86 in
+  // iOS < 9 and macOS < 10.11.
+  const auto &Triple = SemaRef.getASTContext().getTargetInfo().getTriple();
+  VersionTuple AcceptedInVersion;
+  if (Triple.getOS() == llvm::Triple::IOS)
+    AcceptedInVersion = VersionTuple(/*Major=*/9);
+  else if (Triple.isMacOSX())
+    AcceptedInVersion = VersionTuple(/*Major=*/10, /*Minor=*/11);
+  else
+    return;
+  VersionTuple MethodVersion = Method->getVersionIntroduced();
+  if (SemaRef.getASTContext().getTargetInfo().getPlatformMinVersion() >=
+          AcceptedInVersion &&
+      (MethodVersion.empty() || MethodVersion >= AcceptedInVersion))
+    return;
+  SemaRef.Diag(Loc, diag::err_objc_method_unsupported_param_ret_type)
+      << T << (Method->getReturnType()->isVectorType() ? /*return value*/ 1
+                                                       : /*parameter*/ 0)
+      << (Triple.isMacOSX() ? "macOS 10.11" : "iOS 9");
+}
+
 Decl *Sema::ActOnMethodDeclaration(
     Scope *S,
     SourceLocation MethodLoc, SourceLocation EndLoc,
     tok::TokenKind MethodType, 
     ObjCDeclSpec &ReturnQT, ParsedType ReturnType,
     ArrayRef<SourceLocation> SelectorLocs,
     Selector Sel,
     // optional arguments. The number of types/arguments is obtained
case OMF_self:
       break;
     }
     
     if (InferRelatedResultType &&
         !ObjCMethod->getReturnType()->isObjCIndependentClassType())
       ObjCMethod->SetRelatedResultType();
   }
 
+  if (MethodDefinition &&
+      Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86)
+    checkObjCMethodX86VectorTypes(*this, ObjCMethod);
+
   ActOnDocumentableDecl(ObjCMethod);
 
   return ObjCMethod;
 }
 
 bool Sema::CheckObjCDeclScope(Decl *D) {
   // Following is also an error. But it is caused by a missing @end
   // and diagnostic is issued elsewhere.

cfe/trunk/test/SemaObjC/x86-method-vector-values.m

+// RUN: %clang_cc1 -verify -DMAC -triple=i686-apple-macosx10.10 -Wno-objc-root-class %s
+// RUN: %clang_cc1 -verify -DMAC -triple=i686-apple-macosx10.4 -Wno-objc-root-class %s
+// RUN: %clang_cc1 -verify -DMAC -triple=i686-apple-darwin14 -Wno-objc-root-class %s
+// RUN: %clang_cc1 -verify -triple=i686-apple-ios8 -Wno-objc-root-class %s
+
+// RUN: %clang_cc1 -verify -DALLOW -DMAC -triple=i686-apple-macosx10.11 -Wno-objc-root-class %s
+// RUN: %clang_cc1 -verify -DALLOW -DMAC -triple=i686-apple-darwin15 -Wno-objc-root-class %s
+// RUN: %clang_cc1 -verify -DALLOW -DIOS -triple=i686-apple-ios9 -Wno-objc-root-class %s
+// RUN: %clang_cc1 -verify -DALLOW -DOTHER -triple=i686-apple-watchos -Wno-objc-root-class %s
+// RUN: %clang_cc1 -verify -DALLOW -DOTHER -triple=i686-apple-tvos -Wno-objc-root-class %s
+
+// RUN: %clang_cc1 -verify -DALLOW -DOTHER -triple=x86_64-apple-macosx10.10 -Wno-objc-root-class %s
+
+// rdar://21662309
+
+typedef __attribute__((__ext_vector_type__(3))) float float3;
+
+typedef float __m128 __attribute__((__vector_size__(16)));
+
+struct Aggregate { __m128 v; };
+struct AggregateFloat { float v; };
+
+#define AVAILABLE_MACOS_10_10 __attribute__((availability(macos, introduced = 10.10)))
+#define AVAILABLE_MACOS_10_11 __attribute__((availability(macos, introduced = 10.11)))
+
+#define AVAILABLE_IOS_8 __attribute__((availability(ios, introduced = 8.0)))
+#define AVAILABLE_IOS_9 __attribute__((availability(ios, introduced = 9.0)))
+
+@interface VectorMethods
+
+-(void)takeVector:(float3)v; // there should be no diagnostic at declaration
+-(void)takeM128:(__m128)v;
+
+@end
+
+@implementation VectorMethods
+
+#ifndef ALLOW
+
+-(void)takeVector:(float3)v {
+#ifdef MAC
+  // expected-error@-2 {{'float3' (vector of 3 'float' values) parameter type is unsupported; support for vector types for this target is introduced in macOS 10.11}}
+#else
+  // expected-error@-4 {{'float3' (vector of 3 'float' values) parameter type is unsupported; support for vector types for this target is introduced in iOS 9}}
+#endif
+}
+
+-(float3)retVector { // expected-error {{'float3' (vector of 3 'float' values) return type is unsupported}}
+}
+
+-(void)takeVector2:(float3)v AVAILABLE_MACOS_10_10 { // expected-error {{'float3' (vector of 3 'float' values) parameter type is unsupported}}
+}
+
+-(void)takeVector3:(float3)v AVAILABLE_MACOS_10_11 { // expected-error {{'float3' (vector of 3 'float' values) parameter type is unsupported}}
+}
+
+-(void)takeVector4:(float3)v AVAILABLE_IOS_8 { // expected-error {{'float3' (vector of 3 'float' values) parameter type is unsupported}}
+}
+
+-(void)takeVector5:(float3)v AVAILABLE_IOS_9 { // expected-error {{'float3' (vector of 3 'float' values) parameter type is unsupported}}
+}
+
+- (__m128)retM128 { // expected-error {{'__m128' (vector of 4 'float' values) return type is unsupported}}
+}
+
+- (void)takeM128:(__m128)v { // expected-error {{'__m128' (vector of 4 'float' values) parameter type is unsupported}}
+}
+
+#else
+
+-(void)takeVector:(float3)v {
+}
+
+-(float3)retVector {
+  return 0;
+}
+
+- (__m128)retM128 {
+  __m128 value;
+  return value;
+}
+
+- (void)takeM128:(__m128)v {
+}
+
+-(void)takeVector2:(float3)v AVAILABLE_MACOS_10_10 {
+#ifdef MAC
+// expected-error@-2 {{'float3' (vector of 3 'float' values) parameter type is unsupported}}
+#endif
+}
+
+- (__m128)retM128_2 AVAILABLE_MACOS_10_10 {
+#ifdef MAC
+// expected-error@-2 {{'__m128' (vector of 4 'float' values) return type is unsupported}}
+#endif
+  __m128 value;
+  return value;
+}
+
+-(void)takeVector3:(float3)v AVAILABLE_MACOS_10_11 { // no error
+}
+
+-(void)takeVector4:(float3)v AVAILABLE_IOS_8 {
+#ifdef IOS
+  // expected-error@-2 {{'float3' (vector of 3 'float' values) parameter type is unsupported}}
+#endif
+}
+
+-(void)takeVector5:(float3)v AVAILABLE_IOS_9 { // no error
+}
+
+#ifdef OTHER
+// expected-no-diagnostics
+#endif
+
+#endif
+
+-(void)doStuff:(int)m { // no error
+}
+
+-(struct Aggregate)takesAndRetVectorInAggregate:(struct Aggregate)f { // no error
+  struct Aggregate result;
+  return result;
+}
+
+-(struct AggregateFloat)takesAndRetFloatInAggregate:(struct AggregateFloat)f { // no error
+  struct AggregateFloat result;
+  return result;
+}
+
+
+@end