Add XRay flags to Clang

[dberris]

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.

[echristo]

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]

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]

• Merge branch 'master' of http://llvm.org/git/clang into xray
• Merge branch 'master' of http://llvm.org/git/clang into xray
• Add runtime support for XRay

[dberris]

• Address review comments; see details.

[dberris]

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]

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]

• 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]

Rebase

[dberris]

Rebase harder.

[aaron.ballman]

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]

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]

• Formatting changes

[aaron.ballman]

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]

• Check first whether D is actually not nullptr

[dberris]

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

[dberris]

• Check D is valid before using it

[rnk]

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.

[aaron.ballman]

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.

[rnk]

In D20352#477084, @aaron.ballman 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.

[echristo]

In D20352#477203, @rnk wrote:

In D20352#477084, @aaron.ballman 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]

In D20352#477203, @rnk wrote:

In D20352#477084, @aaron.ballman 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.

[dberris]

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

Cheers

[aaron.ballman]

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]

Rebase

[dberris]

• Use hasFlag instead of hasArg

[dberris]

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.

[aaron.ballman]

In D20352#482590, @dberris wrote:

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.