Address review comments

Author: ahatanak

clang/include/clang/Basic/DiagnosticParseKinds.td

@@ -1710,6 +1710,9 @@ def warn_pragma_unroll_cuda_value_in_parens : Warning<
   "argument to '#pragma unroll' should not be in parentheses in CUDA C/C++">,
   InGroup<CudaCompat>;
 
+def err_ptrauth_qualifier_bad_arg_count : Error<
+  "'__ptrauth' qualifier must take between 1 and 3 arguments">;
+
 def warn_cuda_attr_lambda_position : Warning<
   "nvcc does not allow '__%0__' to appear after the parameter list in lambdas">,
   InGroup<CudaCompat>;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -989,8 +989,6 @@ def err_ptrauth_qualifier_nonpointer : Error<
   "'__ptrauth' qualifier only applies to pointer types; %0 is invalid">;
 def err_ptrauth_qualifier_redundant : Error<
   "type %0 is already %1-qualified">;
-def err_ptrauth_qualifier_bad_arg_count : Error<
-  "'__ptrauth' qualifier must take between 1 and 3 arguments">;
 def err_ptrauth_arg_not_ice : Error<
   "argument to '__ptrauth' must be an integer constant expression">;
 def err_ptrauth_address_discrimination_invalid : Error<

clang/include/clang/Basic/Features.def

@@ -107,7 +107,7 @@ FEATURE(thread_sanitizer, LangOpts.Sanitize.has(SanitizerKind::Thread))
 FEATURE(dataflow_sanitizer, LangOpts.Sanitize.has(SanitizerKind::DataFlow))
 FEATURE(scudo, LangOpts.Sanitize.hasOneOf(SanitizerKind::Scudo))
 FEATURE(ptrauth_intrinsics, LangOpts.PointerAuthIntrinsics)
-FEATURE(ptrauth_qualifier, LangOpts.PointerAuthIntrinsics)
+EXTENSION(ptrauth_qualifier, LangOpts.PointerAuthIntrinsics)
 FEATURE(ptrauth_calls, LangOpts.PointerAuthCalls)
 FEATURE(ptrauth_returns, LangOpts.PointerAuthReturns)
 FEATURE(ptrauth_vtable_pointer_address_discrimination, LangOpts.PointerAuthVTPtrAddressDiscrimination)

clang/lib/Parse/ParseDecl.cpp

@@ -3405,7 +3405,7 @@ void Parser::DistributeCLateParsedAttrs(Decl *Dcl,
 ///    ('__ptrauth') '(' constant-expression
 ///                   (',' constant-expression)[opt]
 ///                   (',' constant-expression)[opt] ')'
-void Parser::ParsePtrauthQualifier(ParsedAttributes &attrs) {
+void Parser::ParsePtrauthQualifier(ParsedAttributes &Attrs) {
   assert(Tok.is(tok::kw___ptrauth));
 
   IdentifierInfo *KwName = Tok.getIdentifierInfo();
@@ -3428,7 +3428,12 @@ void Parser::ParsePtrauthQualifier(ParsedAttributes &attrs) {
   T.consumeClose();
   SourceLocation EndLoc = T.getCloseLocation();
 
-  attrs.addNew(KwName, SourceRange(KwLoc, EndLoc),
+  if (ArgExprs.empty() || ArgExprs.size() > 3) {
+    Diag(KwLoc, diag::err_ptrauth_qualifier_bad_arg_count);
+    return;
+  }
+
+  Attrs.addNew(KwName, SourceRange(KwLoc, EndLoc),
                /*scope*/ nullptr, SourceLocation(), ArgExprs.data(),
                ArgExprs.size(),
                ParsedAttr::Form::Keyword(/*IsAlignAs=*/false,

clang/lib/Sema/SemaCast.cpp

@@ -72,6 +72,9 @@ namespace {
       //   Preceding an expression by a parenthesized type name converts the
       //   value of the expression to the unqualified, non-atomic version of
       //   the named type.
+      // Don't drop __ptrauth qualifiers. We want to treat casting to a
+      // __ptrauth-qualified type as an error instead of implicitly ignoring
+      // the qualifier.
       if (!S.Context.getLangOpts().ObjC && !DestType->isRecordType() &&
           !DestType->isArrayType() && !DestType.getPointerAuth()) {
         DestType = DestType.getAtomicUnqualifiedType();

clang/lib/Sema/SemaChecking.cpp

@@ -1577,18 +1577,12 @@ bool Sema::checkPointerAuthDiscriminatorArg(Expr *Arg,
   };
 
   if (*Result < 0 || *Result > Max) {
-    llvm::SmallString<32> Value;
-    {
-      llvm::raw_svector_ostream str(Value);
-      str << *Result;
-    }
-
     if (IsAddrDiscArg)
       Diag(Arg->getExprLoc(), diag::err_ptrauth_address_discrimination_invalid)
-          << Value;
+          << Result->getExtValue();
     else
       Diag(Arg->getExprLoc(), diag::err_ptrauth_extra_discriminator_invalid)
-          << Value << Max;
+          << Result->getExtValue() << Max;
 
     return false;
   };

clang/lib/Sema/SemaDecl.cpp

@@ -19379,10 +19379,10 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,
       } else if (FT.getQualifiers().getObjCLifetime() == Qualifiers::OCL_Weak) {
         Record->setArgPassingRestrictions(
             RecordArgPassingKind::CanNeverPassInRegs);
-      } else if (PointerAuthQualifier Q = FT.getPointerAuth()) {
-        if (Q.isAddressDiscriminated())
-          Record->setArgPassingRestrictions(
-              RecordArgPassingKind::CanNeverPassInRegs);
+      } else if (PointerAuthQualifier Q = FT.getPointerAuth();
+                 Q && Q.isAddressDiscriminated()) {
+        Record->setArgPassingRestrictions(
+            RecordArgPassingKind::CanNeverPassInRegs);
       }
     }
 

clang/lib/Sema/SemaDeclCXX.cpp

@@ -9363,7 +9363,7 @@ struct SpecialMemberDeletionInfo
 
   bool shouldDeleteForVariantObjCPtrMember(FieldDecl *FD, QualType FieldType);
 
-  bool shouldDeleteForVariantPtrAuthMember(FieldDecl *FD, QualType FieldType);
+  bool shouldDeleteForVariantPtrAuthMember(const FieldDecl *FD);
 
   bool visitBase(CXXBaseSpecifier *Base) { return shouldDeleteForBase(Base); }
   bool visitField(FieldDecl *Field) { return shouldDeleteForField(Field); }
@@ -9534,7 +9534,8 @@ bool SpecialMemberDeletionInfo::shouldDeleteForVariantObjCPtrMember(
 }
 
 bool SpecialMemberDeletionInfo::shouldDeleteForVariantPtrAuthMember(
-    FieldDecl *FD, QualType FieldType) {
+    const FieldDecl *FD) {
+  QualType FieldType = S.Context.getBaseElementType(FD->getType());
   // Copy/move constructors/assignment operators are deleted if the field has an
   // address-discriminated ptrauth qualifier.
   PointerAuthQualifier Q = FieldType.getPointerAuth();
@@ -9594,7 +9595,7 @@ bool SpecialMemberDeletionInfo::shouldDeleteForField(FieldDecl *FD) {
   if (inUnion() && shouldDeleteForVariantObjCPtrMember(FD, FieldType))
     return true;
 
-  if (inUnion() && shouldDeleteForVariantPtrAuthMember(FD, FieldType))
+  if (inUnion() && shouldDeleteForVariantPtrAuthMember(FD))
     return true;
 
   if (CSM == CXXSpecialMemberKind::DefaultConstructor) {
@@ -9660,7 +9661,7 @@ bool SpecialMemberDeletionInfo::shouldDeleteForField(FieldDecl *FD) {
         if (shouldDeleteForVariantObjCPtrMember(&*UI, UnionFieldType))
           return true;
 
-        if (shouldDeleteForVariantPtrAuthMember(&*UI, UnionFieldType))
+        if (shouldDeleteForVariantPtrAuthMember(&*UI))
           return true;
 
         if (!UnionFieldType.isConstQualified())

clang/lib/Sema/SemaType.cpp

@@ -8413,12 +8413,9 @@ static void HandleNeonVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
 /// Handle the __ptrauth qualifier.
 static void HandlePtrAuthQualifier(ASTContext &Ctx, QualType &T,
                                    const ParsedAttr &Attr, Sema &S) {
-  if (Attr.getNumArgs() < 1 || Attr.getNumArgs() > 3) {
-    S.Diag(Attr.getLoc(), diag::err_ptrauth_qualifier_bad_arg_count);
-    Attr.setInvalid();
-    return;
-  }
 
+  assert((Attr.getNumArgs() > 0 && Attr.getNumArgs() <= 3) &&
+         "__ptrauth qualifier takes between 1 and 3 arguments");
   Expr *KeyArg = Attr.getArgAsExpr(0);
   Expr *IsAddressDiscriminatedArg =
       Attr.getNumArgs() >= 2 ? Attr.getArgAsExpr(1) : nullptr;

clang/test/Parser/ptrauth-qualifier.c

@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios -fsyntax-only -verify -fptrauth-intrinsics %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -fsyntax-only -verify -fptrauth-intrinsics %s
+
+#if __aarch64__
+#define VALID_DATA_KEY 2
+#else
+#error Provide these constants if you port this test
+#endif
+
+int * __ptrauth(VALID_DATA_KEY) valid0;
+
+typedef int *intp;
+
+int nonConstantGlobal = 5;
+
+__ptrauth int invalid0; // expected-error{{expected '('}}
+__ptrauth() int invalid1; // expected-error{{expected expression}}
+int * __ptrauth(VALID_DATA_KEY, 1, 1000, 12) invalid12; // expected-error{{qualifier must take between 1 and 3 arguments}}