[PATCH] D89086: [MemProf] Allow the binary to specify the profile output filename

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a comment.

LGTM but I have no opinion on David's request.



================
Comment at: compiler-rt/lib/memprof/memprof_rtl.cpp:31
+// Allow the user to specify a profile output file via the binary.
+SANITIZER_WEAK_ATTRIBUTE char __memprof_profile_filename[1] = {0};
+
----------------
not needed?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_file.cpp:71-92
 void ReportFile::SetReportPath(const char *path) {
-  if (!path)
-    return;
-  uptr len = internal_strlen(path);
-  if (len > sizeof(path_prefix) - 100) {
-    Report("ERROR: Path is too long: %c%c%c%c%c%c%c%c...\n",
-           path[0], path[1], path[2], path[3],
-           path[4], path[5], path[6], path[7]);
-    Die();
+  if (path) {
+    uptr len = internal_strlen(path);
+    if (len > sizeof(path_prefix) - 100) {
+      Report("ERROR: Path is too long: %c%c%c%c%c%c%c%c...\n", path[0], path[1],
+             path[2], path[3], path[4], path[5], path[6], path[7]);
+      Die();
----------------
looks correct, but it would be nice to have sanitizer_common as a separate patch


================
Comment at: compiler-rt/test/memprof/TestCases/atexit_stats.cpp:4
 // RUN: %clangxx_memprof -O0 %s -o %t
-// RUN: %env_memprof_opts=atexit=1 %run %t 2>&1 | FileCheck %s
-// RUN: %env_memprof_opts=atexit=0 %run %t 2>&1 | FileCheck %s --check-prefix=NOATEXIT
+// RUN: %env_memprof_opts=log_path=stderr:atexit=1 %run %t 2>&1 | FileCheck %s
+// RUN: %env_memprof_opts=log_path=stderr:atexit=0 %run %t 2>&1 | FileCheck %s --check-prefix=NOATEXIT
----------------
It's not clear why this is needed before reading patch description.
Seems unrelated to __memprof_profile_filename and can be a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89086