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

[Dean Michael Berris via Phabricator via cfe-commits]

dberris added inline comments.


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


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3248
+    // custom events.
+    {
+      auto XRayBundle = CGM.getCodeGenOpts().getXRayInstrumentationBundle();
----------------
echristo wrote:
> I'd probably spell this code like the block above it rather than this.
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.


================
Comment at: clang/lib/Driver/XRayArgs.cpp:62
+          << (std::string(XRayInstrumentOption) +
+              " on non-supported target OS");
     }
----------------
pelikan wrote:
> 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).
Probably not today. Good idea though, could be a different patch.


https://reviews.llvm.org/D44970