Page MenuHomePhabricator

[XRay][clang] Add flag to choose instrumentation bundles
ClosedPublic

Authored by dberris on Mar 27 2018, 10:36 PM.

Details

Summary

This change addresses http://llvm.org/PR36926 by allowing users to pick
which instrumentation bundles to use, when instrumenting with XRay. In
particular, the flag -fxray-instrumentation-bundle= has four valid
values:

  • all: the default, emits all instrumentation kinds
  • none: equivalent to -fnoxray-instrument
  • function: emits the entry/exit instrumentation
  • custom: emits the custom event instrumentation

These can be combined either as comma-separated values, or as
repeated flag values.

Diff Detail

Repository
rC Clang

Event Timeline

dberris created this revision.Mar 27 2018, 10:36 PM
echristo added inline comments.Mar 28 2018, 9:50 AM
clang/include/clang/Frontend/CodeGenOptions.h
113 ↗(On Diff #140042)

"function" might spell easier? :)

clang/lib/CodeGen/CGBuiltin.cpp
3248 ↗(On Diff #140042)

I'd probably spell this code like the block above it rather than this.

clang/lib/Driver/XRayArgs.cpp
61 ↗(On Diff #140042)

Extraneous reformat.

86 ↗(On Diff #140042)

How about a more descriptive name here and a string switch below?

clang/lib/Frontend/CompilerInvocation.cpp
452 ↗(On Diff #140042)

StringSwitch maybe?

I would probably add more tests for the different configurations, but I suspect more diffs are coming after this.

clang/include/clang/Driver/Options.td
1112 ↗(On Diff #140042)

I'd suggest making them "fn-only" and "custom-only", or just plain "function" and "custom".

1112 ↗(On Diff #140042)

I also don't get why "none" is an option here, if it's equivalent to -fnoxray-instrument. Why duplicate the functionality and add more points the users will have to debug?

clang/include/clang/Frontend/CodeGenOptions.h
110 ↗(On Diff #140042)

To me, this feels like a bitfield would be a better match.
All = Function | Custom
None = 0

clang/lib/Driver/XRayArgs.cpp
62 ↗(On Diff #140042)

I would also print the triple. Something like:

"-fomit-bugs is not supported on target win64-ppc-none"

will be much more informative, especially when you collect logs from build machines on lots of architectures (like Linux distro/BSD package builders do).

86 ↗(On Diff #140042)

+1

dberris updated this revision to Diff 140731.Apr 2 2018, 8:13 PM
dberris marked 8 inline comments as done.
  • fixup: address comments
clang/include/clang/Frontend/CodeGenOptions.h
110 ↗(On Diff #140042)

Thought about that, but the ergonomics from the user-side isn't as good. Having to know about which kinds of sleds specifically to enable seems much harder to explain. Using bundles that we control from the beginning keeps this much simpler.

clang/lib/CodeGen/CGBuiltin.cpp
3248 ↗(On Diff #140042)

The codegen options don't work like pointers, and we can't use C++17 if initialisers yet. Would have loved this to be:

if (auto XRayBundle = ...; XRayBundle == ... || XRayBundle == ...)
  return RValue::getIgnored();

Removed the scoping instead.

clang/lib/Driver/XRayArgs.cpp
62 ↗(On Diff #140042)

Probably not today. Good idea though, could be a different patch.

dberris updated this revision to Diff 140735.Apr 2 2018, 9:42 PM
  • fixup: Fix tests for better coverage of settings
echristo added inline comments.Apr 10 2018, 6:19 PM
clang/include/clang/Frontend/CodeGenOptions.h
110 ↗(On Diff #140042)

I dunno, I think I agree with the other commenter. This feels awkward.

clang/lib/CodeGen/CGBuiltin.cpp
3245 ↗(On Diff #140735)

Break this out into some sort of function to determine which one we want? We do this a couple times.

dberris updated this revision to Diff 141958.Apr 11 2018, 1:38 AM
dberris marked an inline comment as done.
  • Rebase
  • Address comments to use a bitmask/bitset for instrumentation bundle
dberris edited the summary of this revision. (Show Details)Apr 11 2018, 1:40 AM
pelikan accepted this revision.Apr 12 2018, 5:31 AM

Most of my comments are minor enough I'd be OK if this went in. But please consider them before committing.

clang/include/clang/Driver/XRayArgs.h
29 ↗(On Diff #141958)

Since the class already has "XRay" in its name, I would rename the member to just "InstrumentationBundle", just as most of the other members are.

clang/include/clang/Frontend/CodeGenOptions.h
111 ↗(On Diff #141958)

Now I fail to spot where is this enum used. IIUC this should work even when it's not here, as the code uses the things in namespace XRayInstrKind.

clang/lib/CodeGen/CodeGenFunction.cpp
469 ↗(On Diff #141958)

typo: builin -> builtin

471 ↗(On Diff #141958)

I kind of don't like how the "-fxray-instrument" variable is called "XRayInstrumentFunctions" because that's not what it means any more. I think in a later diff, we should clean this up. Or maybe even clean up some of the old flags whose functionality has been superseded by this. But the logic here is fine.

Same with the misleading "ShouldXRayInstrumentFunction()" which controls custom events too, and not just functions.

clang/lib/Driver/XRayArgs.cpp
107–109 ↗(On Diff #141958)

nitpick: I'd rewrite it to

if (!Valid) {
  D.Diag
  continue; // or break or return
}

<stuff if Valid below>

but the current code logic is fine.

clang/lib/Frontend/CompilerInvocation.cpp
457 ↗(On Diff #141958)

did you mean: "(Mask == None)"?

This revision is now accepted and ready to land.Apr 12 2018, 5:31 AM
dberris updated this revision to Diff 142315.Apr 12 2018, 7:26 PM
dberris marked 5 inline comments as done.
  • fixup: Address comments

Thanks, Martin! Landing now, after suggested changes.

clang/lib/CodeGen/CodeGenFunction.cpp
471 ↗(On Diff #141958)

Good point. Yes, we could make this cleaner.

This revision was automatically updated to reflect the committed changes.