[PATCH] D87120: [HeapProf] Heap profiling runtime support

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

It is difficult to differentiate stack/static data/heap memory (`int foo(int *a) { return *a; }` may access static `.data`). The instrumentation implementation/runtime is actually generic and the shadow memory works for stack/static data as well. Will the profiler be focused on heap so 'heapprof' will still be an appropriate name? Can .rodata/.data allocation be optimized if their access patterns are known?

When porting `lib/asan/asan_linux.cpp lib/asan/asan_malloc_linux.cpp`, it may make sense renaming `linux` to `posix` because the files aren't really entirely Linux specific.

One question to sanitizers folks: test/asan and test/hwasan have a TestCases subdirectory. test/tsan, test/msan and test/cfi don't. Does heapprof need a TestCases subdirectory?



================
Comment at: compiler-rt/lib/heapprof/heapprof_flags.cpp:26
+static const char *MaybeCallHeapprofDefaultOptions() {
+  return (&__heapprof_default_options) ? __heapprof_default_options() : "";
+}
----------------
We can get rid of such functions (`Maybe*DefaultOptions`). See D87175


================
Comment at: compiler-rt/lib/heapprof/heapprof_flags.cpp:59
+    cf.malloc_context_size = kDefaultMallocContextSize;
+    cf.intercept_tls_get_addr = true;
+    cf.exitcode = 1;
----------------
intercept_tls_get_addr is untested

How will it be used? At runtime the PT_TLS block is copied to memory allocated by mmap by individual threads, so the TLS blocks can be counted as heap memory. If the runtime is not ready to recognize such memory references, intercept_tls_get_addr should be postponed.


================
Comment at: compiler-rt/lib/heapprof/heapprof_interface.inc:1
+//===-- heapprof_interface.inc --------------------------------------------===//
+//
----------------
This file is only needed when the Windows port is implemented.


================
Comment at: compiler-rt/lib/heapprof/heapprof_internal.h:47
+    // run-time functions from the instrumented user code in a profile.
+    namespace __heapprof {
+
----------------
This does not need indentation


================
Comment at: compiler-rt/lib/heapprof/heapprof_linux.cpp:1
+//===-- heapprof_linux.cpp ------------------------------------------------===//
+//
----------------
The name of asan_linux.cpp is actually confusing. *BSD and Solaris implementations also share that file.

Rename this to heapprof_posix.cpp? Though, on asan side, there is also an asan_posix.cpp ...


================
Comment at: compiler-rt/lib/heapprof/heapprof_linux.cpp:90
+  return internal_strstr(libname, "libclang_rt.heapprof") ||
+         internal_strstr(libname, "libheapprof.so");
+}
----------------
Remove `internal_strstr(libname, "libheapprof.so")`

There is no gcc port.


================
Comment at: compiler-rt/lib/heapprof/heapprof_linux.cpp:132
+        if (IsDynamicRTName(segment.filename)) {
+          Report("Your application is linked against "
+                 "incompatible HeapProf runtimes.\n");
----------------
This logic needs a port of `asan_rt_conflict_test-1.cpp`


================
Comment at: compiler-rt/lib/heapprof/heapprof_malloc_linux.cpp:1
+//===-- heapprof_malloc_linux.cpp -----------------------------------------===//
+//
----------------
heapprof_malloc_posix.cpp may be more appropriate for future platform additions.


================
Comment at: compiler-rt/lib/heapprof/heapprof_report.cpp:83
+    // Print memory stats.
+    if (flags()->print_stats)
+      __heapprof_print_accumulated_stats();
----------------
Does print_stats=1 have a test demonstrating how it works?

I picked one test and added `HEAPPROF_OPTIONS=print_stats=1` but did not observe any difference.


================
Comment at: compiler-rt/lib/heapprof/heapprof_report.cpp:143
+void ReportCallocOverflow(uptr count, uptr size, BufferedStackTrace *stack) {
+  ScopedInErrorReport in_report(/*fatal*/ true);
+  ErrorCallocOverflow error(GetCurrentTidOrInvalid(), stack, count, size);
----------------
`/*fatal=*/true`

https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html


================
Comment at: compiler-rt/lib/heapprof/heapprof_rtl.cpp:107
+    case 1: __heapprof_record_access(nullptr); break;
+    case 2: __heapprof_record_access_range(nullptr,0); break;
+  }
----------------
Space after `,`


================
Comment at: compiler-rt/lib/heapprof/heapprof_rtl.cpp:313
+extern "C" SANITIZER_INTERFACE_ATTRIBUTE u16
+__sanitizer_unaligned_load16(const uu16 *p) {
+  __heapprof_record_access(p);
----------------
There needs to be a port of asan/TestCases/unaligned_loads_and_stores.cpp to test these interfaces.


================
Comment at: compiler-rt/lib/heapprof/heapprof_stats.cpp:1
+//===-- heapprof_stats.cpp ------------------------------------------------===//
+//
----------------
It seems that this important file is untested.


================
Comment at: compiler-rt/test/heapprof/TestCases/test_memintrin.cpp:11
+// but we need to look for them in the same CHECK to get the correct STACKIDP.
+// CHECK-DAG:  Heap allocation stack id = [[STACKIDP:[0-9]+]]{{[[:space:]].*}} alloc_count 1, size (ave/min/max) 40.00 / 40 / 40{{[[:space:]].*}} access_count (ave/min/max): 3.00 / 3 / 3
+//
----------------
Just write a space, instead of using `[[:space:]]`

`[[#%u,STACKIDP:]]` can match an unsigned decimal.


================
Comment at: compiler-rt/test/heapprof/lit.cfg.py:36
+  config.environment['HEAPPROF_OPTIONS'] = default_heapprof_opts_str
+  default_heapprof_opts_str += ':'
+config.substitutions.append(('%env_heapprof_opts=',
----------------
This line is not needed


================
Comment at: compiler-rt/test/heapprof/lit.cfg.py:43
+
+libdl_flag = "-ldl"
+
----------------
Prefer single quotes for string literals. ditto below


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87120