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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D89086#2322033 <https://reviews.llvm.org/D89086#2322033>, @davidxl wrote:

> Profile runtime has 4 ways of setting output: 1) default 2) compiler command line arg 3) environment variable at runtime; and 4) user invocation of runtime API at runtime. The order of the precedence is 4> 3 > 2 > 1.  Is the behavior here consistent with that?

The runtime I recently upstreamed has #3, and this patch adds #1 (well there is a default in the already upstreamed stuff, but it is stderr) and #2. A follow on patch will add #4. The order of precedence will be the same as in the profile runtime (it is already 3>2>1).



================
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};
+
----------------
vitalybuka wrote:
> not needed?
This is to ensure that the if below at line 182 will fail when the compiler has not set up a strong version of this variable.


================
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();
----------------
vitalybuka wrote:
> looks correct, but it would be nice to have sanitizer_common as a separate patch
Ok, I can split this out.


================
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
----------------
vitalybuka wrote:
> It's not clear why this is needed before reading patch description.
> Seems unrelated to __memprof_profile_filename and can be a separate patch.
I can just move this to the follow on patch that changes the compiler which affects the defaults, requiring this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89086