This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dberris on Jun 9 2017, 5:15 AM.

Details

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.

Event Timeline

dberris created this revision.Jun 9 2017, 5:15 AM
dberris updated this revision to Diff 102018.Jun 9 2017, 5:18 AM
dberris edited the summary of this revision. (Show Details)

Adding related bug.

dberris added a subscriber: dblaikie.

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

dblaikie edited edge metadata.Jun 12 2017, 11:08 PM

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)

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 updated this revision to Diff 102491.Jun 13 2017, 11:54 PM
  • fixup: use bool arg in checkFunctionOrMethodParameterIndex

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

dblaikie accepted this revision.Jun 15 2017, 11:52 AM

Looks fine - thanks

This revision is now accepted and ready to land.Jun 15 2017, 11:52 AM
This revision was automatically updated to reflect the committed changes.