Reland "[Attr] Fix parameter indexing for several attributes"

Relands r326602 (reverted in r326862) with new test and fix for
PR36620.

Differential Revision: https://reviews.llvm.org/D43248

llvm-svn: 327405

clang/include/clang/AST/Attr.h

@@ -195,6 +195,128 @@ class ParameterABIAttr : public InheritableParamAttr {
    }
 };
 
+/// A single parameter index whose accessors require each use to make explicit
+/// the parameter index encoding needed.
+class ParamIdx {
+  // Idx is exposed only via accessors that specify specific encodings.
+  unsigned Idx : 30;
+  unsigned HasThis : 1;
+  unsigned IsValid : 1;
+
+  void assertComparable(const ParamIdx &I) const {
+    assert(isValid() && I.isValid() &&
+           "ParamIdx must be valid to be compared");
+    // It's possible to compare indices from separate functions, but so far
+    // it's not proven useful.  Moreover, it might be confusing because a
+    // comparison on the results of getASTIndex might be inconsistent with a
+    // comparison on the ParamIdx objects themselves.
+    assert(HasThis == I.HasThis &&
+           "ParamIdx must be for the same function to be compared");
+  }
+
+public:
+  /// Construct an invalid parameter index (\c isValid returns false and
+  /// accessors fail an assert).
+  ParamIdx() : Idx(0), HasThis(false), IsValid(false) {}
+
+  /// \param Idx is the parameter index as it is normally specified in
+  /// attributes in the source: one-origin including any C++ implicit this
+  /// parameter.
+  ///
+  /// \param D is the declaration containing the parameters.  It is used to
+  /// determine if there is a C++ implicit this parameter.
+  ParamIdx(unsigned Idx, const Decl *D)
+      : Idx(Idx), HasThis(false), IsValid(true) {
+    assert(Idx >= 1 && "Idx must be one-origin");
+    if (const auto *FD = dyn_cast<FunctionDecl>(D))
+      HasThis = FD->isCXXInstanceMember();
+  }
+
+  /// A type into which \c ParamIdx can be serialized.
+  ///
+  /// A static assertion that it's of the correct size follows the \c ParamIdx
+  /// class definition.
+  typedef uint32_t SerialType;
+
+  /// Produce a representation that can later be passed to \c deserialize to
+  /// construct an equivalent \c ParamIdx.
+  SerialType serialize() const {
+    return *reinterpret_cast<const SerialType *>(this);
+  }
+
+  /// Construct from a result from \c serialize.
+  static ParamIdx deserialize(SerialType S) {
+    ParamIdx P(*reinterpret_cast<ParamIdx *>(&S));
+    assert((!P.IsValid || P.Idx >= 1) && "valid Idx must be one-origin");
+    return P;
+  }
+
+  /// Is this parameter index valid?
+  bool isValid() const { return IsValid; }
+
+  /// Get the parameter index as it would normally be encoded for attributes at
+  /// the source level of representation: one-origin including any C++ implicit
+  /// this parameter.
+  ///
+  /// This encoding thus makes sense for diagnostics, pretty printing, and
+  /// constructing new attributes from a source-like specification.
+  unsigned getSourceIndex() const {
+    assert(isValid() && "ParamIdx must be valid");
+    return Idx;
+  }
+
+  /// Get the parameter index as it would normally be encoded at the AST level
+  /// of representation: zero-origin not including any C++ implicit this
+  /// parameter.
+  ///
+  /// This is the encoding primarily used in Sema.  However, in diagnostics,
+  /// Sema uses \c getSourceIndex instead.
+  unsigned getASTIndex() const {
+    assert(isValid() && "ParamIdx must be valid");
+    assert(Idx >= 1 + HasThis &&
+           "stored index must be base-1 and not specify C++ implicit this");
+    return Idx - 1 - HasThis;
+  }
+
+  /// Get the parameter index as it would normally be encoded at the LLVM level
+  /// of representation: zero-origin including any C++ implicit this parameter.
+  ///
+  /// This is the encoding primarily used in CodeGen.
+  unsigned getLLVMIndex() const {
+    assert(isValid() && "ParamIdx must be valid");
+    assert(Idx >= 1 && "stored index must be base-1");
+    return Idx - 1;
+  }
+
+  bool operator==(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx == I.Idx;
+  }
+  bool operator!=(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx != I.Idx;
+  }
+  bool operator<(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx < I.Idx;
+  }
+  bool operator>(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx > I.Idx;
+  }
+  bool operator<=(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx <= I.Idx;
+  }
+  bool operator>=(const ParamIdx &I) const {
+    assertComparable(I);
+    return Idx >= I.Idx;
+  }
+};
+
+static_assert(sizeof(ParamIdx) == sizeof(ParamIdx::SerialType),
+              "ParamIdx does not fit its serialization type");
+
 #include "clang/AST/Attrs.inc"
 
 inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,

clang/include/clang/Basic/Attr.td

@@ -169,6 +169,12 @@ class VariadicUnsignedArgument<string name> : Argument<name, 1>;
 class VariadicExprArgument<string name> : Argument<name, 1>;
 class VariadicStringArgument<string name> : Argument<name, 1>;
 
+// Like VariadicUnsignedArgument except values are ParamIdx.
+class VariadicParamIdxArgument<string name> : Argument<name, 1>;
+
+// Like VariadicParamIdxArgument but for a single function parameter index.
+class ParamIdxArgument<string name, bit opt = 0> : Argument<name, opt>;
+
 // A version of the form major.minor[.subminor].
 class VersionArgument<string name, bit opt = 0> : Argument<name, opt>;
 
@@ -614,6 +620,12 @@ def XRayInstrument : InheritableAttr {
 def XRayLogArgs : InheritableAttr {
   let Spellings = [Clang<"xray_log_args">];
   let Subjects = SubjectList<[Function, ObjCMethod]>;
+  // This argument is a count not an index, so it has the same encoding (base
+  // 1 including C++ implicit this parameter) at the source and LLVM levels of
+  // representation, so ParamIdxArgument is inappropriate.  It is never used
+  // at the AST level of representation, so it never needs to be adjusted not
+  // to include any C++ implicit this parameter.  Thus, we just store it and
+  // use it as an unsigned that never needs adjustment.
   let Args = [UnsignedArgument<"ArgumentCount">];
   let Documentation = [XRayDocs];
 }
@@ -1021,7 +1033,8 @@ def EmptyBases : InheritableAttr, TargetSpecificAttr<TargetMicrosoftCXXABI> {
 def AllocSize : InheritableAttr {
   let Spellings = [GCC<"alloc_size">];
   let Subjects = SubjectList<[Function]>;
-  let Args = [IntArgument<"ElemSizeParam">, IntArgument<"NumElemsParam", 1>];
+  let Args = [ParamIdxArgument<"ElemSizeParam">,
+              ParamIdxArgument<"NumElemsParam", /*opt*/ 1>];
   let TemplateDependent = 1;
   let Documentation = [AllocSizeDocs];
 }
@@ -1108,7 +1121,7 @@ def Format : InheritableAttr {
 
 def FormatArg : InheritableAttr {
   let Spellings = [GCC<"format_arg">];
-  let Args = [IntArgument<"FormatIdx">];
+  let Args = [ParamIdxArgument<"FormatIdx">];
   let Subjects = SubjectList<[ObjCMethod, HasFunctionProto]>;
   let Documentation = [Undocumented];
 }
@@ -1388,16 +1401,16 @@ def NonNull : InheritableParamAttr {
   let Spellings = [GCC<"nonnull">];
   let Subjects = SubjectList<[ObjCMethod, HasFunctionProto, ParmVar], WarnDiag,
                              "functions, methods, and parameters">;
-  let Args = [VariadicUnsignedArgument<"Args">];
-  let AdditionalMembers =
-[{bool isNonNull(unsigned idx) const {
-    if (!args_size())
-      return true;
-    for (const auto &V : args())
-      if (V == idx)
+  let Args = [VariadicParamIdxArgument<"Args">];
+  let AdditionalMembers = [{
+    bool isNonNull(unsigned IdxAST) const {
+      if (!args_size())
         return true;
-    return false;
-  } }];
+      return args_end() != std::find_if(
+          args_begin(), args_end(),
+          [=](const ParamIdx &Idx) { return Idx.getASTIndex() == IdxAST; });
+    }
+  }];
   // FIXME: We should merge duplicates into a single nonnull attribute.
   let InheritEvenIfAlreadyPresent = 1;
   let Documentation = [NonNullDocs];
@@ -1455,7 +1468,7 @@ def AssumeAligned : InheritableAttr {
 def AllocAlign : InheritableAttr {
   let Spellings = [GCC<"alloc_align">];
   let Subjects = SubjectList<[HasFunctionProto]>;
-  let Args = [IntArgument<"ParamIndex">];
+  let Args = [ParamIdxArgument<"ParamIndex">];
   let Documentation = [AllocAlignDocs];
 }
 
@@ -1664,7 +1677,8 @@ def Ownership : InheritableAttr {
              Returns;
     }
   }];
-  let Args = [IdentifierArgument<"Module">, VariadicUnsignedArgument<"Args">];
+  let Args = [IdentifierArgument<"Module">,
+              VariadicParamIdxArgument<"Args">];
   let Subjects = SubjectList<[HasFunctionProto]>;
   let Documentation = [Undocumented];
 }
@@ -2490,8 +2504,8 @@ def ArgumentWithTypeTag : InheritableAttr {
                    Clang<"pointer_with_type_tag">];
   let Subjects = SubjectList<[HasFunctionProto], ErrorDiag>;
   let Args = [IdentifierArgument<"ArgumentKind">,
-              UnsignedArgument<"ArgumentIdx">,
-              UnsignedArgument<"TypeTagIdx">,
+              ParamIdxArgument<"ArgumentIdx">,
+              ParamIdxArgument<"TypeTagIdx">,
               BoolArgument<"IsPointer", /*opt*/0, /*fake*/1>];
   let Documentation = [ArgumentWithTypeTagDocs, PointerWithTypeTagDocs];
 }

clang/lib/AST/ExprConstant.cpp

@@ -5475,9 +5475,8 @@ static bool getBytesReturnedByAllocSizeCall(const ASTContext &Ctx,
                                             llvm::APInt &Result) {
   const AllocSizeAttr *AllocSize = getAllocSizeAttr(Call);
 
-  // alloc_size args are 1-indexed, 0 means not present.
-  assert(AllocSize && AllocSize->getElemSizeParam() != 0);
-  unsigned SizeArgNo = AllocSize->getElemSizeParam() - 1;
+  assert(AllocSize && AllocSize->getElemSizeParam().isValid());
+  unsigned SizeArgNo = AllocSize->getElemSizeParam().getASTIndex();
   unsigned BitsInSizeT = Ctx.getTypeSize(Ctx.getSizeType());
   if (Call->getNumArgs() <= SizeArgNo)
     return false;
@@ -5495,14 +5494,13 @@ static bool getBytesReturnedByAllocSizeCall(const ASTContext &Ctx,
   if (!EvaluateAsSizeT(Call->getArg(SizeArgNo), SizeOfElem))
     return false;
 
-  if (!AllocSize->getNumElemsParam()) {
+  if (!AllocSize->getNumElemsParam().isValid()) {
     Result = std::move(SizeOfElem);
     return true;
   }
 
   APSInt NumberOfElems;
-  // Argument numbers start at 1
-  unsigned NumArgNo = AllocSize->getNumElemsParam() - 1;
+  unsigned NumArgNo = AllocSize->getNumElemsParam().getASTIndex();
   if (!EvaluateAsSizeT(Call->getArg(NumArgNo), NumberOfElems))
     return false;
 

clang/lib/CodeGen/CGCall.cpp

@@ -1847,10 +1847,9 @@ void CodeGenModule::ConstructAttributeList(
     HasOptnone = TargetDecl->hasAttr<OptimizeNoneAttr>();
     if (auto *AllocSize = TargetDecl->getAttr<AllocSizeAttr>()) {
       Optional<unsigned> NumElemsParam;
-      // alloc_size args are base-1, 0 means not present.
-      if (unsigned N = AllocSize->getNumElemsParam())
-        NumElemsParam = N - 1;
-      FuncAttrs.addAllocSizeAttr(AllocSize->getElemSizeParam() - 1,
+      if (AllocSize->getNumElemsParam().isValid())
+        NumElemsParam = AllocSize->getNumElemsParam().getLLVMIndex();
+      FuncAttrs.addAllocSizeAttr(AllocSize->getElemSizeParam().getLLVMIndex(),
                                  NumElemsParam);
     }
   }
@@ -4399,7 +4398,7 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
                               OffsetValue);
     } else if (const auto *AA = TargetDecl->getAttr<AllocAlignAttr>()) {
       llvm::Value *ParamVal =
-          CallArgs[AA->getParamIndex() - 1].RV.getScalarVal();
+          CallArgs[AA->getParamIndex().getLLVMIndex()].RV.getScalarVal();
       EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
     }
   }

clang/lib/Sema/SemaChecking.cpp

@@ -2619,12 +2619,13 @@ static void CheckNonNullArguments(Sema &S,
         return;
       }
 
-      for (unsigned Val : NonNull->args()) {
-        if (Val >= Args.size())
+      for (const ParamIdx &Idx : NonNull->args()) {
+        unsigned IdxAST = Idx.getASTIndex();
+        if (IdxAST >= Args.size())
           continue;
         if (NonNullArgs.empty())
           NonNullArgs.resize(Args.size());
-        NonNullArgs.set(Val);
+        NonNullArgs.set(IdxAST);
       }
     }
   }
@@ -5002,12 +5003,7 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
     const CallExpr *CE = cast<CallExpr>(E);
     if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl())) {
       if (const FormatArgAttr *FA = ND->getAttr<FormatArgAttr>()) {
-        unsigned ArgIndex = FA->getFormatIdx();
-        if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(ND))
-          if (MD->isInstance())
-            --ArgIndex;
-        const Expr *Arg = CE->getArg(ArgIndex - 1);
-
+        const Expr *Arg = CE->getArg(FA->getFormatIdx().getASTIndex());
         return checkFormatStringExpr(S, Arg, Args,
                                      HasVAListArg, format_idx, firstDataArg,
                                      Type, CallType, InFunctionCall,
@@ -5032,8 +5028,7 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
     const auto *ME = cast<ObjCMessageExpr>(E);
     if (const auto *ND = ME->getMethodDecl()) {
       if (const auto *FA = ND->getAttr<FormatArgAttr>()) {
-        unsigned ArgIndex = FA->getFormatIdx();
-        const Expr *Arg = ME->getArg(ArgIndex - 1);
+        const Expr *Arg = ME->getArg(FA->getFormatIdx().getASTIndex());
         return checkFormatStringExpr(
             S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
             CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset);
@@ -10086,8 +10081,8 @@ void Sema::DiagnoseAlwaysNonNullPointer(Expr *E,
               return;
           }
 
-          for (unsigned ArgNo : NonNull->args()) {
-            if (ArgNo == ParamNo) {
+          for (const ParamIdx &ArgNo : NonNull->args()) {
+            if (ArgNo.getASTIndex() == ParamNo) {
               ComplainAboutNonnullParamOrCall(NonNull);
               return;
             }
@@ -12242,13 +12237,13 @@ void Sema::CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
   bool IsPointerAttr = Attr->getIsPointer();
 
   // Retrieve the argument representing the 'type_tag'.
-  if (Attr->getTypeTagIdx() >= ExprArgs.size()) {
-    // Add 1 to display the user's specified value.
+  unsigned TypeTagIdxAST = Attr->getTypeTagIdx().getASTIndex();
+  if (TypeTagIdxAST >= ExprArgs.size()) {
     Diag(CallSiteLoc, diag::err_tag_index_out_of_range)
-        << 0 << Attr->getTypeTagIdx() + 1;
+        << 0 << Attr->getTypeTagIdx().getSourceIndex();
     return;
   }
-  const Expr *TypeTagExpr = ExprArgs[Attr->getTypeTagIdx()];
+  const Expr *TypeTagExpr = ExprArgs[TypeTagIdxAST];
   bool FoundWrongKind;
   TypeTagData TypeInfo;
   if (!GetMatchingCType(ArgumentKind, TypeTagExpr, Context,
@@ -12262,13 +12257,13 @@ void Sema::CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
   }
 
   // Retrieve the argument representing the 'arg_idx'.
-  if (Attr->getArgumentIdx() >= ExprArgs.size()) {
-    // Add 1 to display the user's specified value.
+  unsigned ArgumentIdxAST = Attr->getArgumentIdx().getASTIndex();
+  if (ArgumentIdxAST >= ExprArgs.size()) {
     Diag(CallSiteLoc, diag::err_tag_index_out_of_range)
-        << 1 << Attr->getArgumentIdx() + 1;
+        << 1 << Attr->getArgumentIdx().getSourceIndex();
     return;
   }
-  const Expr *ArgumentExpr = ExprArgs[Attr->getArgumentIdx()];
+  const Expr *ArgumentExpr = ExprArgs[ArgumentIdxAST];
   if (IsPointerAttr) {
     // Skip implicit cast of pointer to `void *' (as a function argument).
     if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(ArgumentExpr))

clang/lib/Sema/SemaDecl.cpp

@@ -13185,7 +13185,7 @@ void Sema::AddKnownFunctionAttributes(FunctionDecl *FD) {
     // We already have a __builtin___CFStringMakeConstantString,
     // but builds that use -fno-constant-cfstrings don't go through that.
     if (!FD->hasAttr<FormatArgAttr>())
-      FD->addAttr(FormatArgAttr::CreateImplicit(Context, 1,
+      FD->addAttr(FormatArgAttr::CreateImplicit(Context, ParamIdx(1, FD),
                                                 FD->getLocation()));
   }
 }