pelikan accepted this revision.
pelikan added a comment.
This revision is now accepted and ready to land.
Most of my comments are minor enough I'd be OK if this went in. But please
consider them before committing.
================
Comment at: clang/include/clang/Driver/XRayArgs.h:29
std::vector<std::string> Modes;
+ XRayInstrSet XRayInstrumentationBundle;
bool XRayInstrument = false;
----------------
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.
================
Comment at: clang/include/clang/Frontend/CodeGenOptions.h:111
+ enum XRayInstrumentationTypes {
+ XRay_Function,
----------------
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.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:469
/// AlwaysEmitXRayCustomEvents - Return true if we should emit IR for calls to
/// the __xray_customevent(...) builin calls, when doing XRay instrumentation.
bool CodeGenFunction::AlwaysEmitXRayCustomEvents() const {
----------------
typo: builin -> builtin
================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:471
bool CodeGenFunction::AlwaysEmitXRayCustomEvents() const {
- return CGM.getCodeGenOpts().XRayAlwaysEmitCustomEvents;
+ return CGM.getCodeGenOpts().XRayInstrumentFunctions &&
+ (CGM.getCodeGenOpts().XRayAlwaysEmitCustomEvents ||
----------------
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.
================
Comment at: clang/lib/Driver/XRayArgs.cpp:107-109
+ } else
+ D.Diag(clang::diag::err_drv_invalid_value)
+ << "-fxray-instrumentation-bundle=" << P;
----------------
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.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:457
+ auto Mask = parseXRayInstrValue(B);
+ if (Mask == 0)
+ if (B != "none")
----------------
did you mean: "(Mask == None)"?
https://reviews.llvm.org/D44970
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
