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

[dberris]

Summary

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.

Fixes llvm.org/PR33356.

[dberris]

Adding related bug.

[dberris]

@dblaikie -- do you have time to have a look?

[dblaikie]

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)

[dberris]

In 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?

[dberris]

• fixup: use bool arg in checkFunctionOrMethodParameterIndex

[dberris]

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

[dblaikie]

Looks fine - thanks

lib/Sema/SemaDeclAttr.cpp

 }
 
 /// \brief Check if IdxExpr is a valid parameter index for a function or
 /// instance method D.  May output an error.
 ///
 /// \returns true if IdxExpr is a valid index.
 template <typename AttrInfo>
 static bool checkFunctionOrMethodParameterIndex(
-    Sema &S, const Decl *D, const AttrInfo& Attr,
-    unsigned AttrArgNum, const Expr *IdxExpr, uint64_t &Idx) {
+    Sema &S, const Decl *D, const AttrInfo &Attr, unsigned AttrArgNum,
+    const Expr *IdxExpr, uint64_t &Idx, bool AllowImplicitThis = false) {
   assert(isFunctionOrMethodOrBlock(D));
 
   // In C++ the implicit 'this' function parameter also counts.
   // Parameters are counted from one.
   bool HP = hasFunctionProto(D);
   bool HasImplicitThisParam = isInstanceMethod(D);
   bool IV = HP && isFunctionOrMethodVariadic(D);
   unsigned NumParams =
static bool checkFunctionOrMethodParameterIndex(
 
   Idx = IdxInt.getLimitedValue();
   if (Idx < 1 || (!IV && Idx > NumParams)) {
     S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_out_of_bounds)
       << getAttrName(Attr) << AttrArgNum << IdxExpr->getSourceRange();
     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)
         << getAttrName(Attr) << IdxExpr->getSourceRange();
       return false;
     }
     --Idx;
   }
D->addAttr(::new (S.Context)
                                     Attr.getLayoutCompatible(),
                                     Attr.getMustBeNull(),
                                     Attr.getAttributeSpellingListIndex()));
 }
 
 static void handleXRayLogArgsAttr(Sema &S, Decl *D,
                                   const AttributeList &Attr) {
   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()));
 }
 
 //===----------------------------------------------------------------------===//
 // Checker-specific attribute handlers.
 //===----------------------------------------------------------------------===//
 
 static bool isValidSubjectOfNSReturnsRetainedAttribute(QualType type) {
   return type->isDependentType() ||

test/Sema/xray-log-args-class.cpp

+// 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'}}
+};