Page MenuHomePhabricator

Add XRay flags to Clang
ClosedPublic

Authored by dberris on May 17 2016, 10:34 PM.

Details

Summary

This depends on D19904 to land in LLVM so that the attributes for XRay
instrumentation in the IR are supported and generate the appropriate nop sleds
in function entry/exits. We implement two flags to control the XRay behaviour:

  • -fxray-instrument: enables XRay annotation of IR
  • -fxray-instruction-threshold: configures the threshold for function size (looking at IR instructions), and allow LLVM to decide whether to add the nop sleds later on in the process.

Also Depends on D21612 for the XRay runtime dependency.

Diff Detail

Event Timeline

dberris updated this revision to Diff 57561.May 17 2016, 10:34 PM
dberris retitled this revision from to Add XRay flags to Clang.
dberris added reviewers: echristo, sanjoy, rnk, kcc.
dberris updated this object.
dberris updated this object.May 17 2016, 10:39 PM

Couple of inline comments. Also adding Aaron since he normally reviews the attributes :)

-eric

include/clang/Driver/Options.td
788

fno-xray-instrument

include/clang/Frontend/CodeGenOptions.def
75

Would it make sense to turn -finstrument-functions to -finstrument-functions=<type> to avoid tons of instrumentation type arguments? If we want to keep -fxray-instrument still it can then just be an alias.

aaron.ballman added inline comments.Jun 22 2016, 5:47 AM
include/clang/Basic/Attr.td
434

Would this also make sense for ObjC methods?

435

This gives a diagnostic suggesting ObjC methods, so this should either be removed (it will diagnose properly without it) or the Subjects list should be altered to include ObjC methods.

436

No new undocumented attributes. Also, TODO usually don't contain user names when we add them.

439

Same comments here as above.

I kind of think these are a good candidates to combine into a single attribute with multiple spellings.

def XRayInstrument : InheritableAttr {
  let Spellings = [GNU<"xray_always_instrument">, ...,
                           GNU<"xray_never_instrument">, ...];
}

I think this because I am guessing they will always appertain to the same subjects and take the same (lack of) arguments. You can tell which attribute you have at runtime by looking at the semantic spelling of the attribute. You could add an accessor to get that in a more friendly fashion if desired.

If my guess is wrong, feel free to ignore this suggestion. :-)

test/Sema/xray-always-instrument-attr.c
5

Should also be a test showing that it doesn't accept args. Also, CodeGen tests.

dberris updated this revision to Diff 61567.Jun 22 2016, 10:17 AM
dberris edited edge metadata.
dberris updated this revision to Diff 61932.Jun 26 2016, 11:27 PM
dberris marked 2 inline comments as done.
  • Address review comments; see details.
dberris marked an inline comment as done.Jun 26 2016, 11:32 PM
dberris added inline comments.
include/clang/Basic/Attr.td
434

Yes it does, done.

435

Done the latter to include ObjC methods.

439

This actually makes a lot of sense. I've done some of the consolidation to make it a lot easier to check in the code and to expand later on too. I suppose other attributes that are XRay-specific with additional values/arguments would make a better candidate for additional attributes. Will cross that bridge when we get to that part of the implementation. :)

include/clang/Frontend/CodeGenOptions.def
75

I'm open to this idea, except this might break compatibility with GCC (as this seems to be a gcc-inspired flag).

I'm happy to explore consolidating the different instrumentation flags under -finstrument= which then has namespace values internally (profile-..., xray-...) for different modes, etc. For now the simplest thing to do might be to have XRay do this independently first, then have a cleanup later to consolidate flags on the driver and deprecate the flags.

aaron.ballman edited edge metadata.Jun 27 2016, 7:37 AM

It looks like you ran clang-format over the entire file in some instances, causing a lot of unrelated changes to creep in. Can you back those changes out and only run clang-format over just your patch instead (http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting)?

include/clang/Basic/Attr.td
436

You can drop the WarnDiag and string literal; the defaults already do the right thing.

include/clang/Basic/AttrDocs.td
2457

Let's make sure that this patch and D19908 do not conflict, because they seem to be implementing competing semantics.

lib/CodeGen/CodeGenFunction.cpp
381

Can't you rangify this loop?

402

Any particular reason this isn't a const function?

696

This is not the right way to do this. ;-)

You should check the *semantic* spelling instead. e.g.,

if (const auto *A = D->getAttr<XRayInstrumentAttr>()) {
  switch (A->getSemanticSpelling()) {
  case XRayInstrumentAttr::GNU_blahblah:
  case XRayInstrumentAttr::CXX11_clang_blahblah:
  }
}

Alternatively (and I think this may be cleaner), you could add an additional member in Attr.td that exposes this as two Boolean getters bool alwaysInstrument() const and bool neverInstrument() const that do the semantic spelling switch.

dberris updated this revision to Diff 62056.Jun 27 2016, 9:26 PM
dberris marked 2 inline comments as done.
dberris edited edge metadata.
  • Undo formatting changes
  • Re-apply changes post-review
  • Address more comments
include/clang/Basic/Attr.td
436

I tried, but I'm getting this error:

.../Basic/Attr.td:436:18: error: Could not deduce diagnostic argument for Attr subjects

let Subjects = SubjectList<[CXXMethod, ObjCMethod, Function]>;
include/clang/Basic/AttrDocs.td
2457

Good catch. Let me comment on D19908 to see if it competes with how we define XRay instrumentation. As far as I can tell from the LLVM side, those should not interfere with each other.

lib/CodeGen/CodeGenFunction.cpp
381

Probably in a different change. :) For now I've reverted the formatting changes.

402

No good reason. :) Good catch, fixed.

696

I thought this was a little weird, thanks for pointing this out!

I've used accessors instead, maybe it looks better now?

dberris updated this revision to Diff 62720.Jul 5 2016, 12:31 AM

Rebase harder.

aaron.ballman added inline comments.Jul 5 2016, 7:30 AM
include/clang/Basic/Attr.td
436

Then ClangAttrEmitter.cpp needs to be updated as well (this is a reasonable case to automatically support, I think).

include/clang/Basic/AttrDocs.td
2457

I get the impression they do conflict because they have overlapping functionality (both provide prologue space for runtime patching, for instance). It would be best to have only one attribute if we can do it.

lib/CodeGen/CodeGenFunction.cpp
696

Yes, this is the right way to solve it. Thanks!

However, you should elide the braces when there's only a single statement (per our coding standards).

dberris marked 2 inline comments as done.Jul 5 2016, 10:36 PM
dberris added inline comments.
include/clang/Basic/Attr.td
436

That makes sense, I can do it in another change that's more focused on just that if you don't mind?

include/clang/Basic/AttrDocs.td
2457

I've explored that option (using patchable-function instead) and while I haven't excluded that outright, there needs to be a discussion about how we're going to implement complimentary instrumentation functionality that do nop-sled inserting. I suspect that has to happen in D19904 where we can still make those changes to instead use patchable-function.

The response to my comment in D19908 seems to be that there should be no conflicts in the platforms XRay currently cares about -- but that if we do ever want to get XRay implemented/supported in x86 32-bit, then we're going to have issues. This suggests that a harder look at patchable-function and exclusive modes (or special-casing the supported configurations) might need to happen sooner than later.

dberris updated this revision to Diff 62824.Jul 5 2016, 10:36 PM
dberris marked an inline comment as done.
  • Formatting changes
aaron.ballman added inline comments.Jul 6 2016, 6:41 AM
include/clang/Basic/Attr.td
436

That's fine by me (it's also fine to do as part of this patch since this patch will be what exercises that functionality anyway). Your choice. :-)

include/clang/Basic/AttrDocs.td
2457

I think that makes sense. I mostly want to avoid having multiple user-facing attributes that do very similar operations under different names. That makes it confusing for users, and makes for possibly more-complicated code (are these mutually exclusive attributes, etc). I am less worried about whether the backend has multiple attributes, though I suspect the same concerns may apply there as well.

dberris updated this revision to Diff 63038.Jul 7 2016, 1:49 AM
  • Check first whether D is actually not nullptr
dberris updated this revision to Diff 63039.Jul 7 2016, 1:52 AM

Undo previous change -- updated the wrong patch. :(

dberris updated this revision to Diff 63041.Jul 7 2016, 1:59 AM
  • Check D is valid before using it
rnk accepted this revision.Jul 7 2016, 11:32 AM
rnk edited edge metadata.

Looks good from a driver perspective. Aaron, you're happy with the attribute spelling stuff, right?

It looks like you can actually land this before landing the two dependent changes you mention and the tests will pass. I'd go ahead and land it to avoid the need to rebase, but it's up to you.

include/clang/Basic/AttrDocs.td
2459

These paragraphs can be wrapped to 80 cols like the others.

This revision is now accepted and ready to land.Jul 7 2016, 11:32 AM
aaron.ballman requested changes to this revision.Jul 7 2016, 11:35 AM
aaron.ballman edited edge metadata.
In D20352#477038, @rnk wrote:

Looks good from a driver perspective. Aaron, you're happy with the attribute spelling stuff, right?

No, I'm not. I am worried about how this conflicts with another in-flight patch for supporting MS hotpatchable functions since it seems these two attributes do roughly the same thing. I'd like to understand how these two user-facing attributes will not be confusing to users, what should happen if both attributes wind up on a function, etc. I am hoping that we can wind up with only one attribute that covers both cases, if possible.

This revision now requires changes to proceed.Jul 7 2016, 11:35 AM
rnk added a comment.Jul 7 2016, 12:37 PM

No, I'm not. I am worried about how this conflicts with another in-flight patch for supporting MS hotpatchable functions since it seems these two attributes do roughly the same thing. I'd like to understand how these two user-facing attributes will not be confusing to users, what should happen if both attributes wind up on a function, etc. I am hoping that we can wind up with only one attribute that covers both cases, if possible.

From a user perspective, I would definitely want to keep these attributes separate. It's only possible to completely replace an MS hotpatchable function, whereas XRay allows you to instrument function entry and exit with low overhead. You can achieve the same effect as XRay with MS hotpatchable prologues and a full trampoline with a stack frame, but probably not at the same runtime cost. They really are suited for different tasks.

In D20352#477203, @rnk wrote:

No, I'm not. I am worried about how this conflicts with another in-flight patch for supporting MS hotpatchable functions since it seems these two attributes do roughly the same thing. I'd like to understand how these two user-facing attributes will not be confusing to users, what should happen if both attributes wind up on a function, etc. I am hoping that we can wind up with only one attribute that covers both cases, if possible.

From a user perspective, I would definitely want to keep these attributes separate. It's only possible to completely replace an MS hotpatchable function, whereas XRay allows you to instrument function entry and exit with low overhead. You can achieve the same effect as XRay with MS hotpatchable prologues and a full trampoline with a stack frame, but probably not at the same runtime cost. They really are suited for different tasks.

Agreed.

-eric

aaron.ballman accepted this revision.Jul 7 2016, 12:50 PM
aaron.ballman edited edge metadata.
In D20352#477203, @rnk wrote:

No, I'm not. I am worried about how this conflicts with another in-flight patch for supporting MS hotpatchable functions since it seems these two attributes do roughly the same thing. I'd like to understand how these two user-facing attributes will not be confusing to users, what should happen if both attributes wind up on a function, etc. I am hoping that we can wind up with only one attribute that covers both cases, if possible.

From a user perspective, I would definitely want to keep these attributes separate. It's only possible to completely replace an MS hotpatchable function, whereas XRay allows you to instrument function entry and exit with low overhead. You can achieve the same effect as XRay with MS hotpatchable prologues and a full trampoline with a stack frame, but probably not at the same runtime cost. They really are suited for different tasks.

Okay, that was the key piece I was missing -- I was under the impression that this was two different ways to achieve the same task. Thank you for the explanation (and sorry if I was being dense). :-)

When the MS hot patching attribute lands, that will be the time to discuss and document conflicts between the two (I am assuming you can't enable XRay and MS hot patching for the same function, which may be an incorrect assumption). So I think this LGTM now.

This revision is now accepted and ready to land.Jul 7 2016, 12:50 PM

Thanks -- I don't have commit powers yet, do either of you mind landing this for me?

Cheers

I applied your patch locally and got a few compile errors with MSVC 2015:

'bool llvm::opt::ArgList::hasArg(llvm::opt::OptSpecifier,llvm::opt::OptSpecifier,llvm::opt::OptSpecifier) const': cannot convert argument 3 from 'bool' to 'llvm::opt::OptSpecifier' E:\llvm\llvm\tools\clang\lib\Driver\Tools.cpp 3187
'bool llvm::opt::ArgList::hasArg(llvm::opt::OptSpecifier,llvm::opt::OptSpecifier,llvm::opt::OptSpecifier) const': cannot convert argument 3 from 'bool' to 'llvm::opt::OptSpecifier' E:\llvm\llvm\tools\clang\lib\Driver\Tools.cpp 4608

There is no hasArg() overload that accepts a bool argument; perhaps that's a feature of one of the dependencies this patch depends on?

dberris updated this revision to Diff 63771.Jul 12 2016, 7:35 PM
dberris edited edge metadata.

Rebase

dberris updated this revision to Diff 63775.Jul 12 2016, 7:49 PM
  • Use hasFlag instead of hasArg

It looks like I was using hasArg instead of hasFlag. The dangerous part here is that OptSpecifier has an unsigned non-explicit argument, and I suspect bool is being promoted to unsigned silently with clang/gcc.

It looks like I was using hasArg instead of hasFlag. The dangerous part here is that OptSpecifier has an unsigned non-explicit argument, and I suspect bool is being promoted to unsigned silently with clang/gcc.

Still not quite right with the latest patch:

C2664 'bool llvm::opt::ArgList::hasArg(llvm::opt::OptSpecifier,llvm::opt::OptSpecifier,llvm::opt::OptSpecifier) const': cannot convert argument 3 from 'bool' to 'llvm::opt::OptSpecifier' E:\llvm\llvm\tools\clang\lib\Driver\Tools.cpp 4608

I changed this to use hasFlag() instead, and then it compiled and tests passed. I've commit in r275330.

aaron.ballman closed this revision.Jul 13 2016, 3:39 PM