[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles

[Martin Pelikán via Phabricator via cfe-commits]

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