[PATCH] D87120: [MemProf] Memory profiling runtime support

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a comment.

Tests mostly LGTM.
I'lll send more comments a little bit later today.



================
Comment at: compiler-rt/CMakeLists.txt:77-88
+  if (NOT (COMPILER_RT_MEMPROF_SHADOW_SCALE GREATER -1 AND
+	  COMPILER_RT_MEMPROF_SHADOW_SCALE LESS 8))
+    message(FATAL_ERROR "
+    Invalid Memprof Shadow Scale '${COMPILER_RT_MEMPROF_SHADOW_SCALE}'.")
+  endif()
+
+  set(COMPILER_RT_MEMPROF_SHADOW_SCALE_LLVM_FLAG
----------------
these flags were added recently to support some platform with memory constraints do not allow to use default vales.
I would recommend do copy them, as corresponding -mllvm flags unless memprof is actually needs them.


================
Comment at: compiler-rt/test/memprof/TestCases/default_options.cpp:1-2
+// RUN: %clangxx_memprof -O2 %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
----------------
for 1:1 compile:run statements this way more convenient to copy entire testcase command line when debugging
same other tests


================
Comment at: compiler-rt/test/memprof/TestCases/dump_process_map.cpp:4
+// RUN: %clangxx_memprof -O0 %s -o %t
+// RUN: %env_memprof_opts=dump_process_map=1 %run %t 2>&1 | FileCheck %s
+// RUN: %env_memprof_opts=dump_process_map=0 %run %t 2>&1 | FileCheck %s --check-prefix=NOMAP
----------------
maybe better to add print_module_map=3 and document something
3 - print module map for debugging
and assume sanitizer may use it when ever it wants.


================
Comment at: compiler-rt/test/memprof/TestCases/free_hook_realloc.cpp:3
+// RUN: %clangxx_memprof -O2 %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
----------------
Looks like this test belong to sanitizer_common


================
Comment at: compiler-rt/test/memprof/TestCases/function-sections-are-bad.cpp:2
+// Check that --gc-sections does not throw away (or localize) parts of sanitizer
+// interface.
+// RUN: %clang_memprof %s -Wl,--gc-sections -ldl -o %t
----------------
Why this test is needed?


================
Comment at: compiler-rt/test/memprof/TestCases/memprof_rt_conflict_test.cpp:8
+// REQUIRES: memprof-dynamic-runtime
+// XFAIL: android
+
----------------
Is anyone is going to make it work for android?
If not yet, then I'd remove such UNSUPPORTED and XFAILs .




================
Comment at: compiler-rt/test/memprof/TestCases/sleep_after_init.c:2
+// RUN: %clang_memprof -O2 %s -o %t
+// RUN: %env_memprof_opts=sleep_after_init=1 %run %t 2>&1 | FileCheck %s
+
----------------
I never used this sleep_*, they are so specific, not sure why they are even exist. Also no one bother to introduce them into other sanitizers.
I recommend to not copy them


================
Comment at: compiler-rt/test/memprof/TestCases/stress_dtls.c:5
+//
+// Note that glibc 2.15 seems utterly broken on this test,
+// it fails with ~17 DSOs dlopen-ed.
----------------
2.15 is quite old, I don't think anyone care. I guess it's copied from asan. I recommend to remove this comment to avoid confusion.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87120