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) {
    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)"?

cfe-commits mailing list

Reply via email to