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

Tue Sep 8 20:51:10 PDT 2020

tejohnson added a comment.

Responses to some of @MaskRay's comments below (if no explicit response then ack).

In D87120#2257311 <https://reviews.llvm.org/D87120#2257311>, @MaskRay wrote:

> I pushed a5d6af421d625c78bfb0f63830b51863ff0f0877 <https://reviews.llvm.org/rGa5d6af421d625c78bfb0f63830b51863ff0f0877> to remove annoying clang-tidy readability-identifier-naming warnings. There is a minor compilation error related to stdlib.h. After fixing it, many tests fail because `-fmemprof` was recently renamed to `-fmemory-profile`. Can you upload a new diff?

Thanks for fixing those. I'll fix the option in the tests when I upload a new diff addressing the comments.

In D87120#2259773 <https://reviews.llvm.org/D87120#2259773>, @MaskRay wrote:

> 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?

Yes this came up on the instrumentation thread, and hence I made the option name more general (-fmemprof and now -fmemory-profile). I'll go ahead and do a mass rename to MemProf on this patch. But I will probably do that as a follow on after addressing the other comments, so that it is easier to see the more substantive changes in a single diff.

> 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.

I kept the same file structure as asan, with the assumption that it made sense. I'm not an OS expert, so unfortunately not sure how to distinguish linux vs posix requirement.

> 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:59
+    cf.malloc_context_size = kDefaultMallocContextSize;
+    cf.intercept_tls_get_addr = true;
+    cf.exitcode = 1;
----------------
MaskRay wrote:
> 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.
This is a sanitizer_common flag and the handling is there. I didn't see a reason to disable it here (it seems to be enabled for all the other clients). I did find an asan test for this, will port that over.

================
Comment at: compiler-rt/lib/heapprof/heapprof_linux.cpp:1
+//===-- heapprof_linux.cpp ------------------------------------------------===//
+//
----------------
MaskRay wrote:
> 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 ...
Indeed, but as noted above I kept the same structure (with the same functionality mapped over) as for asan, since I assume it makes sense. Note that POSIX seems to include other OS such as Mac. 

================
Comment at: compiler-rt/lib/heapprof/heapprof_malloc_linux.cpp:1
+//===-- heapprof_malloc_linux.cpp -----------------------------------------===//
+//
----------------
MaskRay wrote:
> heapprof_malloc_posix.cpp may be more appropriate for future platform additions.
This again mirrors the asan naming. 

================
Comment at: compiler-rt/lib/heapprof/heapprof_report.cpp:83
+    // Print memory stats.
+    if (flags()->print_stats)
+      __heapprof_print_accumulated_stats();
----------------
MaskRay wrote:
> 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.
I thought so but I guess not, will add.

================
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
+//
----------------
MaskRay wrote:
> Just write a space, instead of using `[[:space:]]`
> 
> `[[#%u,STACKIDP:]]` can match an unsigned decimal.
I am using [[:space:]] per the instructions at:
https://llvm.org/docs/CommandGuide/FileCheck.html#matching-newline-characters

Since I need to match newlines

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87120