[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:995
 
+def fheapprof : Flag<["-"], "fheapprof">, Group<f_Group>,
+  Flags<[CoreOption, CC1Option]>, HelpText<"Enable heap profiling">;
----------------
`OptInFFlag<"heapprof", "Enable", "Disable", " heap profiling">`


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1027
 
+  Opts.HeapProf = Args.hasFlag(OPT_fheapprof, OPT_fno_heapprof, false);
+
----------------
If there is a default, the convention is to check just that flag:

`Args.hasArg(OPT_fheapprof);`

and remove -fno-heapprof as a CC1 option.


================
Comment at: clang/test/Driver/fheapprof.cpp:1
+// RUN: %clang++ -target x86_64-linux-gnu -fheapprof %s -### 2>&1 | FileCheck %s
+// CHECK: -cc1{{.*}}-fheapprof
----------------
`%clang++` does not work on Windows -> `clang.exe++`

Use `%clangxx`


================
Comment at: clang/test/Driver/fheapprof.cpp:2
+// RUN: %clang++ -target x86_64-linux-gnu -fheapprof %s -### 2>&1 | FileCheck %s
+// CHECK: -cc1{{.*}}-fheapprof
+// CHECK: ld{{.*}}libclang_rt.heapprof{{.*}}libclang_rt.heapprof_cxx
----------------
It is better surrounding the argument with double quotes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85948