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

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

pelikan added a comment.

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



================
Comment at: clang/include/clang/Driver/Options.td:1112
+  Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Select which bundle of XRay instrumentation points to emit. Options: all, none, function-extents, custom-only.">;
+
----------------
I'd suggest making them "fn-only" and "custom-only", or just plain "function" and "custom".


================
Comment at: clang/include/clang/Driver/Options.td:1112
+  Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Select which bundle of XRay instrumentation points to emit. Options: all, none, function-extents, custom-only.">;
+
----------------
pelikan wrote:
> I'd suggest making them "fn-only" and "custom-only", or just plain "function" and "custom".
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?


================
Comment at: clang/include/clang/Frontend/CodeGenOptions.h:110
 
+  enum XRayInstrumentationPointBundle {
+    XRay_All,             // Always emit all the instrumentation points.
----------------
To me, this feels like a bitfield would be a better match.
All = Function | Custom
None = 0


================
Comment at: clang/lib/Driver/XRayArgs.cpp:62
+          << (std::string(XRayInstrumentOption) +
+              " on non-supported target OS");
     }
----------------
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).


================
Comment at: clang/lib/Driver/XRayArgs.cpp:86
+            Args.getLastArg(options::OPT_fxray_instrumentation_bundle)) {
+      StringRef B = A->getValue();
+      if (B != "all" && B != "none" && B != "function-extents" &&
----------------
echristo wrote:
> How about a more descriptive name here and a string switch below?
+1


https://reviews.llvm.org/D44970