[PATCH] [ASan] Add process basename to log name and error message to simplify analysis of sanitized systems logs.

[Alexey Samsonov]

Looks mostly good. I'd rather land this in three changes: (1) refactor Asan dynamic initialization (2) move CacheBinaryName to sanitizer_common and call it early (3) implement log_exe_name.


REPOSITORY
  rL LLVM

================
Comment at: lib/asan/asan_linux.cc:116
@@ -115,9 +115,3 @@
 void AsanCheckDynamicRTPrereqs() {
-  // Ensure that dynamic RT is the first DSO in the list
-  const char *first_dso_name = 0;
-  dl_iterate_phdr(FindFirstDSOCallback, &first_dso_name);
-  if (first_dso_name && !IsDynamicRTName(first_dso_name)) {
-    Report("ASan runtime does not come first in initial library list; "
-           "you should either link runtime to your application or "
-           "manually preload it with LD_PRELOAD.\n");
-    Die();
+  if (ASAN_DYNAMIC) {
+    // Ensure that dynamic RT is the first DSO in the list
----------------
just if (!ASAN_DYNAMIC) return;

================
Comment at: lib/asan/asan_rtl.cc:372
@@ +371,3 @@
+
+  AsanCheckIncompatibleRT();
+  AsanCheckDynamicRTPrereqs();
----------------
I think that moving these functions to the AsanInitInternal is a positive change, do you think you can land it independently (as a refactoring), before submitting the rest of this patch?

================
Comment at: lib/sanitizer_common/sanitizer_common.cc:237
@@ -229,2 +236,3 @@
     return slash_pos + 1;
-  }
+  else if (const char *backslash_pos = internal_strrchr(module, '\\'))
+    return backslash_pos + 1;
----------------
That's weird, don't we handle backslash on Windows above?

================
Comment at: lib/sanitizer_common/sanitizer_common.h:218
@@ -217,1 +217,3 @@
 // OS
+uptr ReadBinaryName(/*out*/char *buf, uptr buf_len);
+const char *GetBinaryName();
----------------
It is already declared below.

================
Comment at: lib/sanitizer_common/sanitizer_printf.cc:261
@@ +260,3 @@
+      }
+      needed_length += internal_snprintf(buffer + needed_length,
+                                         buffer_size - needed_length,
----------------
You should probably check that needed_length is not greater than buffer_size here as well.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:362
@@ -361,8 +361,3 @@
 
-  void PlatformPrepareForSandboxing() override {
-#if SANITIZER_LINUX && !SANITIZER_ANDROID
-    // Cache /proc/self/exe on Linux.
-    CacheBinaryName();
-#endif
-  }
+  void PlatformPrepareForSandboxing() override {}
 
----------------
Note that moving CacheBinaryName() to sanitizer_common (and calling it early during initialization) may also be separated to an independent change, that shouldn't change user-visible behavior.

================
Comment at: lib/tsan/rtl/tsan_rtl.cc:309
@@ -308,2 +308,3 @@
   is_initialized = true;
+  CacheBinaryName();
   // We are not ready to handle interceptors yet.
----------------
I'd rather do this after setting SanitizerToolName and parsing the flags, as you do in different sanitizers.
Also, what about MSan?

================
Comment at: test/asan/TestCases/verbose-log-path_test.cc:1
@@ +1,2 @@
+// RUN: %clangxx_asan %s -o %t
+
----------------
Consider changing the name of the output executable (%t-verbose-log-path_test-binary)...

================
Comment at: test/asan/TestCases/verbose-log-path_test.cc:6
@@ +5,3 @@
+// RUN: env ASAN_OPTIONS=log_path=%t.log:log_exe_name=1 not %run %t 2> %t.out
+// RUN: FileCheck %s --check-prefix=CHECK-ERROR < %t.log.verbose-log-path_test.cc*
+
----------------
... and matching against this executable name here.

================
Comment at: test/asan/TestCases/verbose-log-path_test.cc:9
@@ +8,3 @@
+// FIXME: only FreeBSD and Linux have verbose log paths now.
+// XFAIL: win32
+// XFAIL: android
----------------
  XFAIL: win32,android,darwin

http://reviews.llvm.org/D7333

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/