[Attr] Fix alloc_size's diags to report arg idx not value

[jdenny]

Summary

For consistency with other attributes, fix alloc_size's diagnostics to
report the attribute's argument index for a function parameter index
rather than the actual function parameter index specified in the
source.

[aaron.ballman]

Aside from a minor testcase nit, this LGTM. Why is this dependent on D43248?

test/Sema/alloc-size.c

3

This looks like a whitespace-only change?

[jdenny]

In D43749#1018818, @aaron.ballman wrote:

Aside from a minor testcase nit, this LGTM. Why is this dependent on D43248?

The dependency goes the other way. Did I get it wrong?

test/Sema/alloc-size.c

3

clang-format-diff.py suggested that. Should I drop that?

[aaron.ballman]

In D43749#1018825, @jdenny wrote:

In D43749#1018818, @aaron.ballman wrote:

Aside from a minor testcase nit, this LGTM. Why is this dependent on D43248?

The dependency goes the other way. Did I get it wrong?

Ugh, no, I misunderstood what Phab was trying to tell me. This makes considerably more sense. :-)

[aaron.ballman]

Committed (with whitespace fix for the test case) in r326058, thanks for the patch!

[jdenny]

In D43749#1018846, @aaron.ballman wrote:

Committed (with whitespace fix for the test case) in r326058, thanks for the patch!

Sure. Thanks for committing.

By the way the commit log you added isn't quite right. The issue isn't base 0 versus base 1. The issue is attribute argument index versus attribute argument value. See the test case for an example.

I'm not suggesting we somehow change the commit log. I just want to be sure you see what I actually did here.

[aaron.ballman]

In D43749#1019359, @jdenny wrote:

In D43749#1018846, @aaron.ballman wrote:

Committed (with whitespace fix for the test case) in r326058, thanks for the patch!

Sure. Thanks for committing.

By the way the commit log you added isn't quite right. The issue isn't base 0 versus base 1. The issue is attribute argument index versus attribute argument value. See the test case for an example.

I'm not suggesting we somehow change the commit log. I just want to be sure you see what I actually did here.

Oh, yeah, I did muck up that commit log message a bit. Sorry about that!

[jdenny]

In D43749#1019377, @aaron.ballman wrote:

Oh, yeah, I did muck up that commit log message a bit. Sorry about that!

No problem. :-)

I'll get to your comments in the other patches hopefully soon. Thanks again.

lib/Sema/SemaDeclAttr.cpp

static void handleAssertExclusiveLockAttr(Sema &S, Decl *D,
 
   unsigned Size = Args.size();
   Expr **StartArg = Size == 0 ? nullptr : &Args[0];
   D->addAttr(::new (S.Context) AssertExclusiveLockAttr(
       AL.getRange(), S.Context, StartArg, Size,
       AL.getAttributeSpellingListIndex()));
 }
 
-/// \brief Checks to be sure that the given parameter number is in bounds, and is
-/// an integral type. Will emit appropriate diagnostics if this returns
+/// \brief Checks to be sure that the given parameter number is in bounds, and
+/// is an integral type. Will emit appropriate diagnostics if this returns
 /// false.
 ///
-/// FuncParamNo is expected to be from the user, so is base-1. AttrArgNo is used
-/// to actually retrieve the argument, so it's base-0.
+/// AttrArgNo is used to actually retrieve the argument, so it's base-0.
 template <typename AttrInfo>
 static bool checkParamIsIntegerType(Sema &S, const FunctionDecl *FD,
-                                    const AttrInfo &AI, Expr *AttrArg,
-                                    unsigned FuncParamNo, unsigned AttrArgNo,
+                                    const AttrInfo &AI, unsigned AttrArgNo,
                                     bool AllowDependentType = false) {
+  assert(AI.isArgExpr(AttrArgNo) && "Expected expression argument");
+  Expr *AttrArg = AI.getArgAsExpr(AttrArgNo);
   uint64_t Idx;
-  if (!checkFunctionOrMethodParameterIndex(S, FD, AI, FuncParamNo, AttrArg,
+  if (!checkFunctionOrMethodParameterIndex(S, FD, AI, AttrArgNo + 1, AttrArg,
                                            Idx))
     return false;
 
   const ParmVarDecl *Param = FD->getParamDecl(Idx);
   if (AllowDependentType && Param->getType()->isDependentType())
     return true;
   if (!Param->getType()->isIntegerType() && !Param->getType()->isCharType()) {
     SourceLocation SrcLoc = AttrArg->getLocStart();
     S.Diag(SrcLoc, diag::err_attribute_integers_only)
         << getAttrName(AI) << Param->getSourceRange();
     return false;
   }
   return true;
 }
 
-/// \brief Checks to be sure that the given parameter number is in bounds, and is
-/// an integral type. Will emit appropriate diagnostics if this returns false.
-///
-/// FuncParamNo is expected to be from the user, so is base-1. AttrArgNo is used
-/// to actually retrieve the argument, so it's base-0.
-static bool checkParamIsIntegerType(Sema &S, const FunctionDecl *FD,
-                                    const AttributeList &AL,
-                                    unsigned FuncParamNo, unsigned AttrArgNo,
-                                    bool AllowDependentType = false) {
-  assert(AL.isArgExpr(AttrArgNo) && "Expected expression argument");
-  return checkParamIsIntegerType(S, FD, AL, AL.getArgAsExpr(AttrArgNo),
-                                 FuncParamNo, AttrArgNo, AllowDependentType);
-}
-
 static void handleAllocSizeAttr(Sema &S, Decl *D, const AttributeList &AL) {
   if (!checkAttributeAtLeastNumArgs(S, AL, 1) ||
       !checkAttributeAtMostNumArgs(S, AL, 2))
     return;
 
   const auto *FD = cast<FunctionDecl>(D);
   if (!FD->getReturnType()->isPointerType()) {
     S.Diag(AL.getLoc(), diag::warn_attribute_return_pointers_only)
         << AL.getName();
     return;
   }
 
   const Expr *SizeExpr = AL.getArgAsExpr(0);
   int SizeArgNo;
   // Parameter indices are 1-indexed, hence Index=1
   if (!checkPositiveIntArgument(S, AL, SizeExpr, SizeArgNo, /*Index=*/1))
     return;
 
-  if (!checkParamIsIntegerType(S, FD, AL, SizeArgNo, /*AttrArgNo=*/0))
+  if (!checkParamIsIntegerType(S, FD, AL, /*AttrArgNo=*/0))
     return;
 
   // Args are 1-indexed, so 0 implies that the arg was not present
   int NumberArgNo = 0;
   if (AL.getNumArgs() == 2) {
     const Expr *NumberExpr = AL.getArgAsExpr(1);
     // Parameter indices are 1-based, hence Index=2
     if (!checkPositiveIntArgument(S, AL, NumberExpr, NumberArgNo,
                                   /*Index=*/2))
       return;
 
-    if (!checkParamIsIntegerType(S, FD, AL, NumberArgNo, /*AttrArgNo=*/1))
+    if (!checkParamIsIntegerType(S, FD, AL, /*AttrArgNo=*/1))
       return;
   }
 
   D->addAttr(::new (S.Context)
                  AllocSizeAttr(AL.getRange(), S.Context, SizeArgNo, NumberArgNo,
                                AL.getAttributeSpellingListIndex()));
 }
 

test/Sema/alloc-size.c

 // RUN: %clang_cc1 %s -verify
 
-void *fail1(int a) __attribute__((alloc_size)); //expected-error{{'alloc_size' attribute takes at least 1 argument}}
+void *fail1(int a) __attribute__((alloc_size));   //expected-error{{'alloc_size' attribute takes at least 1 argument}}
 void *fail2(int a) __attribute__((alloc_size())); //expected-error{{'alloc_size' attribute takes at least 1 argument}}
 
-void *fail3(int a) __attribute__((alloc_size(0))); //expected-error{{'alloc_size' attribute parameter 0 is out of bounds}}
-void *fail4(int a) __attribute__((alloc_size(2))); //expected-error{{'alloc_size' attribute parameter 2 is out of bounds}}
+void *fail3(int a) __attribute__((alloc_size(0))); //expected-error{{'alloc_size' attribute parameter 1 is out of bounds}}
+void *fail4(int a) __attribute__((alloc_size(2))); //expected-error{{'alloc_size' attribute parameter 1 is out of bounds}}
 
-void *fail5(int a, int b) __attribute__((alloc_size(0, 1))); //expected-error{{'alloc_size' attribute parameter 0 is out of bounds}}
-void *fail6(int a, int b) __attribute__((alloc_size(3, 1))); //expected-error{{'alloc_size' attribute parameter 3 is out of bounds}}
+void *fail5(int a, int b) __attribute__((alloc_size(0, 1))); //expected-error{{'alloc_size' attribute parameter 1 is out of bounds}}
+void *fail6(int a, int b) __attribute__((alloc_size(3, 1))); //expected-error{{'alloc_size' attribute parameter 1 is out of bounds}}
 
-void *fail7(int a, int b) __attribute__((alloc_size(1, 0))); //expected-error{{'alloc_size' attribute parameter 0 is out of bounds}}
-void *fail8(int a, int b) __attribute__((alloc_size(1, 3))); //expected-error{{'alloc_size' attribute parameter 3 is out of bounds}}
+void *fail7(int a, int b) __attribute__((alloc_size(1, 0))); //expected-error{{'alloc_size' attribute parameter 2 is out of bounds}}
+void *fail8(int a, int b) __attribute__((alloc_size(1, 3))); //expected-error{{'alloc_size' attribute parameter 2 is out of bounds}}
 
 int fail9(int a) __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to return values that are pointers}}
 
 int fail10 __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to functions}}
 
 void *fail11(void *a) __attribute__((alloc_size(1))); //expected-error{{'alloc_size' attribute argument may only refer to a function parameter of integer type}}
 
 void *fail12(int a) __attribute__((alloc_size("abc"))); //expected-error{{'alloc_size' attribute requires parameter 1 to be an integer constant}}
 void *fail12(int a) __attribute__((alloc_size(1, "abc"))); //expected-error{{'alloc_size' attribute requires parameter 2 to be an integer constant}}
 void *fail13(int a) __attribute__((alloc_size(1U<<31))); //expected-error{{integer constant expression evaluates to value 2147483648 that cannot be represented in a 32-bit signed integer type}}
 
 int (*PR31453)(int) __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to functions}}