Page MenuHomePhabricator

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

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



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

  • 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

rC Clang

Event Timeline

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

"function" might spell easier? :)

3248 ↗(On Diff #140042)

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

61 ↗(On Diff #140042)

Extraneous reformat.

86 ↗(On Diff #140042)

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

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.

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?

110 ↗(On Diff #140042)

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

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)


dberris updated this revision to Diff 140731.Apr 2 2018, 8:13 PM
dberris marked 8 inline comments as done.
  • fixup: address comments
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.

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.

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
110 ↗(On Diff #140042)

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

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.

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.

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.

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.

107–109 ↗(On Diff #141958)

nitpick: I'd rewrite it to

if (!Valid) {
  continue; // or break or return

<stuff if Valid below>

but the current code logic is fine.

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.

471 ↗(On Diff #141958)

Good point. Yes, we could make this cleaner.

This revision was automatically updated to reflect the committed changes.