[PATCH] D87953: [xray] Function coverage groups

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

The idea seems fine.

> By selecting different groups over time you could cover the entire application incrementally with lower overhead than instrumenting the entire application at once.

How large the overhead is? This is somewhat surprising to me.



================
Comment at: clang/include/clang/Driver/Options.td:1343
+def fxray_function_groups :
+  JoinedOrSeparate<["-"], "fxray-function-groups=">,
+  Group<f_Group>, Flags<[CC1Option]>,
----------------
Joined.

Separate is for a space between the option name and its value.


================
Comment at: clang/include/clang/Driver/Options.td:1345
+  Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Only instrument 1 of N functions">;
+
----------------
It is "N groups", not "N functions".


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:814
+    auto FuncGroups = CGM.getCodeGenOpts().XRayTotalFunctionGroups;
+    if (FuncGroups > 1) {
+      auto FuncName = ArrayRef<uint8_t>(CurFn->getName().bytes_begin(),
----------------
For one group, the branch is skipped, which does not seem correct


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:815
+    if (FuncGroups > 1) {
+      auto FuncName = ArrayRef<uint8_t>(CurFn->getName().bytes_begin(),
+                                        CurFn->getName().bytes_end());
----------------
`const ArrayRef<uint8_t> FuncName(..., ...)`


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:820
+          !AlwaysXRayAttr) {
+        Fn->addFnAttr("function-instrument", "xray-never");
+      }
----------------
Omit braces around one-line simple statements.


================
Comment at: clang/test/CodeGen/xray-function-groups.cpp:4
+// RUN:            -fxray-selected-function-group=0 \
+// RUN:            -x c++ -std=c++11 -emit-llvm -o - %s \
+// RUN:            -triple x86_64-unknown-linux-gnu | FileCheck --check-prefix=GROUP0 %s
----------------
You can omit `-x c++ -std=c++11 `. They are insignificant. 

Then consider packing more options on one line to reduce the total number of lines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87953/new/

https://reviews.llvm.org/D87953