[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-15 Thread Dean Michael Berris via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL305544: [XRay][clang] Support capturing the implicit `this` 
argument to C++ class… (authored by dberris).

Changed prior to commit:
  https://reviews.llvm.org/D34052?vs=102491=102775#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D34052

Files:
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/test/Sema/xray-log-args-class.cpp


Index: cfe/trunk/test/Sema/xray-log-args-class.cpp
===
--- cfe/trunk/test/Sema/xray-log-args-class.cpp
+++ cfe/trunk/test/Sema/xray-log-args-class.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c++11 -x c++
+
+class Class {
+  [[clang::xray_always_instrument, clang::xray_log_args(1)]] void Method();
+  [[clang::xray_log_args(-1)]] void Invalid(); // expected-error 
{{'xray_log_args' attribute parameter 1 is out of bounds}}
+  [[clang::xray_log_args("invalid")]] void InvalidStringArg(); // 
expected-error {{'xray_log_args'}}
+};
Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -313,8 +313,8 @@
 /// \returns true if IdxExpr is a valid index.
 template 
 static bool checkFunctionOrMethodParameterIndex(
-Sema , const Decl *D, const AttrInfo& Attr,
-unsigned AttrArgNum, const Expr *IdxExpr, uint64_t ) {
+Sema , const Decl *D, const AttrInfo , unsigned AttrArgNum,
+const Expr *IdxExpr, uint64_t , bool AllowImplicitThis = false) {
   assert(isFunctionOrMethodOrBlock(D));
 
   // In C++ the implicit 'this' function parameter also counts.
@@ -341,7 +341,7 @@
 return false;
   }
   Idx--; // Convert to zero-based.
-  if (HasImplicitThisParam) {
+  if (HasImplicitThisParam && !AllowImplicitThis) {
 if (Idx == 0) {
   S.Diag(getAttrLoc(Attr),
  diag::err_attribute_invalid_implicit_this_argument)
@@ -4604,14 +4604,16 @@
 static void handleXRayLogArgsAttr(Sema , Decl *D,
   const AttributeList ) {
   uint64_t ArgCount;
+
   if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, Attr.getArgAsExpr(0),
-   ArgCount))
+   ArgCount,
+   true /* AllowImplicitThis*/))
 return;
 
   // ArgCount isn't a parameter index [0;n), it's a count [1;n] - hence + 1.
   D->addAttr(::new (S.Context)
- XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
- Attr.getAttributeSpellingListIndex()));
+ XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
+ Attr.getAttributeSpellingListIndex()));
 }
 
 
//===--===//


Index: cfe/trunk/test/Sema/xray-log-args-class.cpp
===
--- cfe/trunk/test/Sema/xray-log-args-class.cpp
+++ cfe/trunk/test/Sema/xray-log-args-class.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c++11 -x c++
+
+class Class {
+  [[clang::xray_always_instrument, clang::xray_log_args(1)]] void Method();
+  [[clang::xray_log_args(-1)]] void Invalid(); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+  [[clang::xray_log_args("invalid")]] void InvalidStringArg(); // expected-error {{'xray_log_args'}}
+};
Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -313,8 +313,8 @@
 /// \returns true if IdxExpr is a valid index.
 template 
 static bool checkFunctionOrMethodParameterIndex(
-Sema , const Decl *D, const AttrInfo& Attr,
-unsigned AttrArgNum, const Expr *IdxExpr, uint64_t ) {
+Sema , const Decl *D, const AttrInfo , unsigned AttrArgNum,
+const Expr *IdxExpr, uint64_t , bool AllowImplicitThis = false) {
   assert(isFunctionOrMethodOrBlock(D));
 
   // In C++ the implicit 'this' function parameter also counts.
@@ -341,7 +341,7 @@
 return false;
   }
   Idx--; // Convert to zero-based.
-  if (HasImplicitThisParam) {
+  if (HasImplicitThisParam && !AllowImplicitThis) {
 if (Idx == 0) {
   S.Diag(getAttrLoc(Attr),
  diag::err_attribute_invalid_implicit_this_argument)
@@ -4604,14 +4604,16 @@
 static void handleXRayLogArgsAttr(Sema , Decl *D,
   const AttributeList ) {
   uint64_t ArgCount;
+
   if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, Attr.getArgAsExpr(0),
-   ArgCount))
+   ArgCount,
+   true /* AllowImplicitThis*/))
 return;
 
   // ArgCount isn't a parameter index [0;n), it's a count [1;n] - 

Re: [PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-15 Thread David Blaikie via cfe-commits
On Mon, Jun 12, 2017 at 9:15 PM Dean Michael Berris via Phabricator <
revi...@reviews.llvm.org> wrote:

> dberris added a reviewer: dblaikie.
> dberris added a subscriber: dblaikie.
> dberris added a comment.
>
> @dblaikie -- do you have time to have a look?
>

Sure sure - no need to ping a patch more than about weekly. If it's
addressed to me I'll pretty reliably see it & get to it when I have time. (:

- Dave


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


[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-15 Thread David Blaikie via Phabricator via cfe-commits
dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks fine - thanks


https://reviews.llvm.org/D34052



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


[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-14 Thread Dean Michael Berris via Phabricator via cfe-commits
dberris added a comment.

@dblaikie it turns out that this is a much simpler change now. PTAL?


https://reviews.llvm.org/D34052



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


[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-14 Thread Dean Michael Berris via Phabricator via cfe-commits
dberris updated this revision to Diff 102491.
dberris added a comment.

- fixup: use bool arg in checkFunctionOrMethodParameterIndex


https://reviews.llvm.org/D34052

Files:
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/xray-log-args-class.cpp


Index: test/Sema/xray-log-args-class.cpp
===
--- /dev/null
+++ test/Sema/xray-log-args-class.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c++11 -x c++
+
+class Class {
+  [[clang::xray_always_instrument, clang::xray_log_args(1)]] void Method();
+  [[clang::xray_log_args(-1)]] void Invalid(); // expected-error 
{{'xray_log_args' attribute parameter 1 is out of bounds}}
+  [[clang::xray_log_args("invalid")]] void InvalidStringArg(); // 
expected-error {{'xray_log_args'}}
+};
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -313,8 +313,8 @@
 /// \returns true if IdxExpr is a valid index.
 template 
 static bool checkFunctionOrMethodParameterIndex(
-Sema , const Decl *D, const AttrInfo& Attr,
-unsigned AttrArgNum, const Expr *IdxExpr, uint64_t ) {
+Sema , const Decl *D, const AttrInfo , unsigned AttrArgNum,
+const Expr *IdxExpr, uint64_t , bool AllowImplicitThis = false) {
   assert(isFunctionOrMethodOrBlock(D));
 
   // In C++ the implicit 'this' function parameter also counts.
@@ -341,7 +341,7 @@
 return false;
   }
   Idx--; // Convert to zero-based.
-  if (HasImplicitThisParam) {
+  if (HasImplicitThisParam && !AllowImplicitThis) {
 if (Idx == 0) {
   S.Diag(getAttrLoc(Attr),
  diag::err_attribute_invalid_implicit_this_argument)
@@ -4604,14 +4604,16 @@
 static void handleXRayLogArgsAttr(Sema , Decl *D,
   const AttributeList ) {
   uint64_t ArgCount;
+
   if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, Attr.getArgAsExpr(0),
-   ArgCount))
+   ArgCount,
+   true /* AllowImplicitThis*/))
 return;
 
   // ArgCount isn't a parameter index [0;n), it's a count [1;n] - hence + 1.
   D->addAttr(::new (S.Context)
- XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
- Attr.getAttributeSpellingListIndex()));
+ XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
+ Attr.getAttributeSpellingListIndex()));
 }
 
 
//===--===//


Index: test/Sema/xray-log-args-class.cpp
===
--- /dev/null
+++ test/Sema/xray-log-args-class.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c++11 -x c++
+
+class Class {
+  [[clang::xray_always_instrument, clang::xray_log_args(1)]] void Method();
+  [[clang::xray_log_args(-1)]] void Invalid(); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+  [[clang::xray_log_args("invalid")]] void InvalidStringArg(); // expected-error {{'xray_log_args'}}
+};
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -313,8 +313,8 @@
 /// \returns true if IdxExpr is a valid index.
 template 
 static bool checkFunctionOrMethodParameterIndex(
-Sema , const Decl *D, const AttrInfo& Attr,
-unsigned AttrArgNum, const Expr *IdxExpr, uint64_t ) {
+Sema , const Decl *D, const AttrInfo , unsigned AttrArgNum,
+const Expr *IdxExpr, uint64_t , bool AllowImplicitThis = false) {
   assert(isFunctionOrMethodOrBlock(D));
 
   // In C++ the implicit 'this' function parameter also counts.
@@ -341,7 +341,7 @@
 return false;
   }
   Idx--; // Convert to zero-based.
-  if (HasImplicitThisParam) {
+  if (HasImplicitThisParam && !AllowImplicitThis) {
 if (Idx == 0) {
   S.Diag(getAttrLoc(Attr),
  diag::err_attribute_invalid_implicit_this_argument)
@@ -4604,14 +4604,16 @@
 static void handleXRayLogArgsAttr(Sema , Decl *D,
   const AttributeList ) {
   uint64_t ArgCount;
+
   if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, Attr.getArgAsExpr(0),
-   ArgCount))
+   ArgCount,
+   true /* AllowImplicitThis*/))
 return;
 
   // ArgCount isn't a parameter index [0;n), it's a count [1;n] - hence + 1.
   D->addAttr(::new (S.Context)
- XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
- Attr.getAttributeSpellingListIndex()));
+ XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
+ Attr.getAttributeSpellingListIndex()));
 }
 
 

Re: [PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-13 Thread David Blaikie via cfe-commits
hard to say if it's more readable without seeing it - if you could attach a
patch, if it's easy enough/you worked it up before, might be worth
comparing/contrasting


On Tue, Jun 13, 2017 at 12:28 AM Dean Michael Berris via Phabricator <
revi...@reviews.llvm.org> wrote:

> dberris added a comment.
>
> In https://reviews.llvm.org/D34052#778670, @dblaikie wrote:
>
> > I take it there's already handling for these attributes on non-member
> functions?
>
>
> Yes, we're just extending it to also apply to the case where it doesn't
> support this one case where we actually do need the implicit this argument
>
> > There's probably a reason this code can't be wherever that code is &
> subtract one from the index? (reducing code duplication by having all the
> error handling, etc, in one place/once)
>
> I tried doing it for the `checkFunctionOrMethodNumParams` function, but it
> ended up being much more complicated. :(
>
> The choice is between adding a bool argument (e.g. AllowImplicitThis) in
> `checkFunctionOrMethodParameterIndex(...)` that's default to always true
> (to preserve existing behaviour) but the checks and implementation of that
> template has a number of assumptions as to whether the index is valid for
> member functions.
>
> I can change this so that the logic is contained in
> `checkFunctionOrMethodParameterIndex(...)` if that's more readable?
>
>
> https://reviews.llvm.org/D34052
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-13 Thread Dean Michael Berris via Phabricator via cfe-commits
dberris added a comment.

In https://reviews.llvm.org/D34052#778670, @dblaikie wrote:

> I take it there's already handling for these attributes on non-member 
> functions?


Yes, we're just extending it to also apply to the case where it doesn't support 
this one case where we actually do need the implicit this argument

> There's probably a reason this code can't be wherever that code is & subtract 
> one from the index? (reducing code duplication by having all the error 
> handling, etc, in one place/once)

I tried doing it for the `checkFunctionOrMethodNumParams` function, but it 
ended up being much more complicated. :(

The choice is between adding a bool argument (e.g. AllowImplicitThis) in 
`checkFunctionOrMethodParameterIndex(...)` that's default to always true (to 
preserve existing behaviour) but the checks and implementation of that template 
has a number of assumptions as to whether the index is valid for member 
functions.

I can change this so that the logic is contained in 
`checkFunctionOrMethodParameterIndex(...)` if that's more readable?


https://reviews.llvm.org/D34052



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


[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-13 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a comment.

I take it there's already handling for these attributes on non-member functions?

There's probably a reason this code can't be wherever that code is & subtract 
one from the index? (reducing code duplication by having all the error 
handling, etc, in one place/once)


https://reviews.llvm.org/D34052



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


[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-09 Thread Dean Michael Berris via Phabricator via cfe-commits
dberris updated this revision to Diff 102018.
dberris edited the summary of this revision.
dberris added a comment.

Adding related bug.


https://reviews.llvm.org/D34052

Files:
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/xray-log-args-class.cpp


Index: test/Sema/xray-log-args-class.cpp
===
--- /dev/null
+++ test/Sema/xray-log-args-class.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c++11 -x c++
+
+class Class {
+  [[clang::xray_always_instrument, clang::xray_log_args(1)]] void Method();
+  [[clang::xray_log_args(-1)]] void Invalid(); // expected-error 
{{'xray_log_args' attribute parameter 1 is out of bounds}}
+  [[clang::xray_log_args("invalid")]] void InvalidStringArg(); // 
expected-error {{'xray_log_args'}}
+};
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4604,14 +4604,40 @@
 static void handleXRayLogArgsAttr(Sema , Decl *D,
   const AttributeList ) {
   uint64_t ArgCount;
+
+  // Treat member functions especially, since we do want to support saving the
+  // implicit this argument for member functions.
+  if (isInstanceMethod(D)) {
+auto *IdxExpr = Attr.getArgAsExpr(0);
+llvm::APSInt IdxInt;
+if (!IdxExpr->isIntegerConstantExpr(IdxInt, S.Context)) {
+  S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_n_type)
+  << getAttrName(Attr) << 1u << AANT_ArgumentIntegerConstant
+  << IdxExpr->getSourceRange();
+  return;
+}
+ArgCount = IdxInt.getLimitedValue();
+bool HP = hasFunctionProto(D);
+unsigned NumParams = HP ? getFunctionOrMethodNumParams(D) + 1 : 1;
+if (ArgCount > NumParams) {
+  S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_out_of_bounds)
+  << getAttrName(Attr) << 1 << IdxExpr->getSourceRange();
+  return;
+}
+D->addAttr(::new (S.Context)
+   XRayLogArgsAttr(Attr.getRange(), S.Context, ArgCount,
+   Attr.getAttributeSpellingListIndex()));
+return;
+  }
+
   if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, Attr.getArgAsExpr(0),
ArgCount))
 return;
 
   // ArgCount isn't a parameter index [0;n), it's a count [1;n] - hence + 1.
   D->addAttr(::new (S.Context)
- XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
- Attr.getAttributeSpellingListIndex()));
+ XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
+ Attr.getAttributeSpellingListIndex()));
 }
 
 
//===--===//


Index: test/Sema/xray-log-args-class.cpp
===
--- /dev/null
+++ test/Sema/xray-log-args-class.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c++11 -x c++
+
+class Class {
+  [[clang::xray_always_instrument, clang::xray_log_args(1)]] void Method();
+  [[clang::xray_log_args(-1)]] void Invalid(); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+  [[clang::xray_log_args("invalid")]] void InvalidStringArg(); // expected-error {{'xray_log_args'}}
+};
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4604,14 +4604,40 @@
 static void handleXRayLogArgsAttr(Sema , Decl *D,
   const AttributeList ) {
   uint64_t ArgCount;
+
+  // Treat member functions especially, since we do want to support saving the
+  // implicit this argument for member functions.
+  if (isInstanceMethod(D)) {
+auto *IdxExpr = Attr.getArgAsExpr(0);
+llvm::APSInt IdxInt;
+if (!IdxExpr->isIntegerConstantExpr(IdxInt, S.Context)) {
+  S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_n_type)
+  << getAttrName(Attr) << 1u << AANT_ArgumentIntegerConstant
+  << IdxExpr->getSourceRange();
+  return;
+}
+ArgCount = IdxInt.getLimitedValue();
+bool HP = hasFunctionProto(D);
+unsigned NumParams = HP ? getFunctionOrMethodNumParams(D) + 1 : 1;
+if (ArgCount > NumParams) {
+  S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_out_of_bounds)
+  << getAttrName(Attr) << 1 << IdxExpr->getSourceRange();
+  return;
+}
+D->addAttr(::new (S.Context)
+   XRayLogArgsAttr(Attr.getRange(), S.Context, ArgCount,
+   Attr.getAttributeSpellingListIndex()));
+return;
+  }
+
   if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, Attr.getArgAsExpr(0),
ArgCount))
 return;
 
   // ArgCount isn't a parameter index [0;n), it's a count [1;n] - hence + 1.
   D->addAttr(::new (S.Context)
- 

[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

2017-06-09 Thread Dean Michael Berris via Phabricator via cfe-commits
dberris created this revision.

Before this change, we couldn't capture the `this` pointer that's
implicitly the first argument of class member functions. There are some
interesting things we can do with capturing even just this single
argument for zero-argument member functions.


https://reviews.llvm.org/D34052

Files:
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/xray-log-args-class.cpp


Index: test/Sema/xray-log-args-class.cpp
===
--- /dev/null
+++ test/Sema/xray-log-args-class.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c++11 -x c++
+
+class Class {
+  [[clang::xray_always_instrument, clang::xray_log_args(1)]] void Method();
+  [[clang::xray_log_args(-1)]] void Invalid(); // expected-error 
{{'xray_log_args' attribute parameter 1 is out of bounds}}
+  [[clang::xray_log_args("invalid")]] void InvalidStringArg(); // 
expected-error {{'xray_log_args'}}
+};
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4604,14 +4604,40 @@
 static void handleXRayLogArgsAttr(Sema , Decl *D,
   const AttributeList ) {
   uint64_t ArgCount;
+
+  // Treat member functions especially, since we do want to support saving the
+  // implicit this argument for member functions.
+  if (isInstanceMethod(D)) {
+auto *IdxExpr = Attr.getArgAsExpr(0);
+llvm::APSInt IdxInt;
+if (!IdxExpr->isIntegerConstantExpr(IdxInt, S.Context)) {
+  S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_n_type)
+  << getAttrName(Attr) << 1u << AANT_ArgumentIntegerConstant
+  << IdxExpr->getSourceRange();
+  return;
+}
+ArgCount = IdxInt.getLimitedValue();
+bool HP = hasFunctionProto(D);
+unsigned NumParams = HP ? getFunctionOrMethodNumParams(D) + 1 : 1;
+if (ArgCount > NumParams) {
+  S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_out_of_bounds)
+  << getAttrName(Attr) << 1 << IdxExpr->getSourceRange();
+  return;
+}
+D->addAttr(::new (S.Context)
+   XRayLogArgsAttr(Attr.getRange(), S.Context, ArgCount,
+   Attr.getAttributeSpellingListIndex()));
+return;
+  }
+
   if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, Attr.getArgAsExpr(0),
ArgCount))
 return;
 
   // ArgCount isn't a parameter index [0;n), it's a count [1;n] - hence + 1.
   D->addAttr(::new (S.Context)
- XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
- Attr.getAttributeSpellingListIndex()));
+ XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
+ Attr.getAttributeSpellingListIndex()));
 }
 
 
//===--===//


Index: test/Sema/xray-log-args-class.cpp
===
--- /dev/null
+++ test/Sema/xray-log-args-class.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c++11 -x c++
+
+class Class {
+  [[clang::xray_always_instrument, clang::xray_log_args(1)]] void Method();
+  [[clang::xray_log_args(-1)]] void Invalid(); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+  [[clang::xray_log_args("invalid")]] void InvalidStringArg(); // expected-error {{'xray_log_args'}}
+};
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4604,14 +4604,40 @@
 static void handleXRayLogArgsAttr(Sema , Decl *D,
   const AttributeList ) {
   uint64_t ArgCount;
+
+  // Treat member functions especially, since we do want to support saving the
+  // implicit this argument for member functions.
+  if (isInstanceMethod(D)) {
+auto *IdxExpr = Attr.getArgAsExpr(0);
+llvm::APSInt IdxInt;
+if (!IdxExpr->isIntegerConstantExpr(IdxInt, S.Context)) {
+  S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_n_type)
+  << getAttrName(Attr) << 1u << AANT_ArgumentIntegerConstant
+  << IdxExpr->getSourceRange();
+  return;
+}
+ArgCount = IdxInt.getLimitedValue();
+bool HP = hasFunctionProto(D);
+unsigned NumParams = HP ? getFunctionOrMethodNumParams(D) + 1 : 1;
+if (ArgCount > NumParams) {
+  S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_out_of_bounds)
+  << getAttrName(Attr) << 1 << IdxExpr->getSourceRange();
+  return;
+}
+D->addAttr(::new (S.Context)
+   XRayLogArgsAttr(Attr.getRange(), S.Context, ArgCount,
+   Attr.getAttributeSpellingListIndex()));
+return;
+  }
+
   if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, Attr.getArgAsExpr(0),