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

tejohnson added a comment. In D85948#2242370 , @tejohnson wrote: > In D85948#2242352 , @lebedev.ri > wrote: > >> This really needs some docs changes. > > Good point, I'll send a patch for that later today.

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

tejohnson added a comment. In D85948#2242352 , @lebedev.ri wrote: > This really needs some docs changes. Good point, I'll send a patch for that later today. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

lebedev.ri added a comment. This really needs some docs changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948 ___ cfe-commits mailing list

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7ed8124d46f9: [HeapProf] Clang and LLVM support for heap profiling instrumentation (authored by tejohnson). Repository: rG LLVM Github Monorepo

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

tejohnson updated this revision to Diff 288191. tejohnson added a comment. Remove unnecessary check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948 Files: clang/include/clang/Basic/CodeGenOptions.def

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

tejohnson added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:563 +return false; + if (!ClDebugFunc.empty() && ClDebugFunc == F.getName()) +return false; MaskRay wrote: > `ClDebugFunc != F.getName()` Your suggestion

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

MaskRay accepted this revision. MaskRay added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:563 +return false; + if (!ClDebugFunc.empty() && ClDebugFunc == F.getName()) +return false; `ClDebugFunc != F.getName()`

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

tejohnson updated this revision to Diff 287832. tejohnson marked 2 inline comments as done. tejohnson added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948 Files:

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

tejohnson marked 18 inline comments as done. tejohnson added a comment. I think I've addressed all the comments. Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:48 +// Size of memory mapped to a single shadow location. +static const uint64_t

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

MaskRay added a comment. I am still trying to read the logic. Some comments to the tests. Some comments are applicable to other tests. Comment at: llvm/test/Instrumentation/HeapProfiling/basic.ll:22 +; CHECK: %[[LOAD_ADDR:[^ ]*]] = ptrtoint i32* %a to i64 +; CHECK:

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

MaskRay added inline comments. Comment at: llvm/test/Instrumentation/HeapProfiling/basic.ll:1 +; Test basic address sanitizer instrumentation. +; The implementation file is HeapProfiler.cpp Naming the test directory `Heapprofiler` can reduce the number of terms

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

MaskRay added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.def:148 CODEGENOPT(MergeFunctions, 1, 0) ///< Set when -fmerge-functions is enabled. +CODEGENOPT(HeapProf , 1, 0) ///< Set when -fmemprof is enabled. CODEGENOPT(MSVolatile

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

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm. The user option should be documented after the runtime is ready. Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:71 + +// This flag may need to be

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

tejohnson updated this revision to Diff 285777. tejohnson marked an inline comment as done. tejohnson added a comment. address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948 Files:

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

tejohnson marked 12 inline comments as done. tejohnson added a comment. Addressed everyone's comments as well as the clang tidy warnings. I had hoped to get the runtime part ready and uploaded today, but it looks like I won't make it. I'll be out all next week, so will get that sent when I'm

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

davidxl added a comment. I think the user visible option needs to match the functionality. Internal naming just needs some comments to document. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948

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

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

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

tejohnson added a comment. Thanks for the comments @vitalybuka and @davidxl, will address those shortly. In D85948#2217476 , @davidxl wrote: > one nit: since the same instrumentation can be used to profiling global > variable accesses (especially those

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

davidxl added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:512 + +int ModuleHeapProfiler::GetHeapProfVersion(const Module ) const { return 1; } + define a macro for the version number? Repository: rG LLVM Github Monorepo

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

davidxl added a comment. one nit: since the same instrumentation can be used to profiling global variable accesses (especially those indirect accessed), the option name seems excluding those cases. Shall it be renamed to fmem-prof? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

vitalybuka added a comment. LGTM with some nits Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:124 + +namespace { + maybe remove all static about and extend namespace {} Comment at:

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

tejohnson created this revision. tejohnson added reviewers: vitalybuka, kcc, eugenis, davidxl. Herald added subscribers: dang, jfb, hiraditya, mgorny. Herald added projects: clang, LLVM. tejohnson requested review of this revision. See RFC for background: