[PATCH] D81995: [xray] Option to omit the function index

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:114
+///< Set with -fno-xray-function-index to omit the index section.
+CODEGENOPT(XRayOmitFunctionIndex , 1, 0)
+
----------------
Nit: a variable name with a positive meaning may be easier to understand.


================
Comment at: clang/test/Driver/XRay/xray-function-index-flags.cpp:6
+// RUN: %clang -fxray-instrument -fxray-function-index -target x86_64-linux- -### \
+// RUN:     -x c++ -std=c++11 -emit-llvm -c -o - %s 2>&1 \
+// RUN:     | FileCheck %s
----------------
If c++11 is not relevant, drop it.


================
Comment at: clang/test/Driver/XRay/xray-function-index-flags.cpp:20
+//
+// REQUIRES: x86_64 || x86_64h
----------------
I know some tests may be inconsistent but the prevailing style is to place `REQUIRES:` at the top.


================
Comment at: compiler-rt/lib/xray/xray_init.cpp:94
+
+      for (std::size_t I = 0; I < XRayInstrMap.Entries; I++) {
+        const auto &Sled = XRayInstrMap.Sleds[I];
----------------
`std::` is uncommon.

LLVM style is ++I


================
Comment at: compiler-rt/lib/xray/xray_init.cpp:95
+      for (std::size_t I = 0; I < XRayInstrMap.Entries; I++) {
+        const auto &Sled = XRayInstrMap.Sleds[I];
+        const auto Function = Sled.function();
----------------
spell out the concrete type if non-obvious.


================
Comment at: llvm/test/CodeGen/AArch64/xray-omit-function-index.ll:6
+; CHECK-NEXT:  b  #32
+; CHECK-NEXT:  nop
+; CHECK-NEXT:  nop
----------------
CHECK-COUNT


================
Comment at: llvm/test/CodeGen/AArch64/xray-omit-function-index.ll:34
+; CHECK-NOT: xray_fn_idx
\ No newline at end of file

----------------
No newline at end of file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81995