Currently when we set log_path in ASAN_OPTIONS, Asan runtime appends process ID to name to distinguish log files for different processes. This patch adds process name to log name. This is handy for inspection of Asan logs of fully sanitized system runs (for common Linux distributions this would be hundreds of logs). I've also added process name to Asan error report. Currently it prints just process ID so to identify the actual process one needs to manually scan the stacktrace (which is not always available) and check the PC of top-most frame.
|566 ↗||(On Diff #14710)|
Why do you need this?
This is wrong. W/o some kind of synchronization on the read path it would not prevent, for example, prefetching of proc_self_exe_cache_str before checking proc_self_exe_cache_initialized.
You need release store and acquire load.
This should be changed to reading the first chunk of /proc/self/cmdline. It is normally the same as /exe, but on Android it would return the "nice name" of an application if it has one.
This is probably out of the scope of this CL.
Not part of this CL but just noticed while reviewing: it looks like this comment and error message was missed in the SANITIZER_FREEBSD work. This should either be moved into the individual #if cases above or made into a generic error ("WARNING: reading executable name failed with errno %d")
Guys, I had to revert the commit because it caused warnings in compiler-rt when compiled with clang:
/home/ygribov/src/compiler-rt-master/lib/sanitizer_common/sanitizer_common.cc:230:22: warning: declaration requires a global constructor [-Wglobal-constructors] static BlockingMutex binary_name_cache_lock(LINKER_INITIALIZED); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
It looks like LINKER_INITIALIZED trick isn't working after all - a global (empty) ctor is generated anyways. Any ideas on how to fix this? I could e.g. replace this global with array of uptrs and the reinterpret_cast them to BlockingMutex. This could be wrapped in a macro to hide all the ugliness.
Something like this seems to work
+#define DEFINE_LINKER_INITIALIZED_OBJECT(type, name) \ + static uptr __ ## name ## _bytes [(sizeof(type) + sizeof(u64) - 1) / sizeof(u64)]; \ + static type & name = reinterpret_cast<type &>(__ ## name ## _bytes); ... -static BlockingMutex binary_name_cache_lock(LINKER_INITIALIZED); +DEFINE_LINKER_INITIALIZED_OBJECT(BlockingMutex, binary_name_cache_lock); static char binary_name_cache_str[kMaxPathLength]; static bool binary_name_cache_initialized = false;
Does this approach make sense?
Yea. For historical reasons we do allow LINKER_INITIALIZED in asan, but not in tsan and not in sanitizer_common.
It probably makes sense to remove all instances of LINKER_INITIALIZED in asan too
Your suggestion with DEFINE_LINKER_INITIALIZED_OBJECT makes sense, go for it. (as a separate CL, preferably)
Can we enforce the correct alignment of the array of uptr's?
Yeah, I just went for u64 because I thought it'd force largest alignment on a platform. But this indeed wouldn't handle user-aligned types properly. Perhaps I should just use array of chars with ALIGNED(alignof(type)).