This is an archive of the discontinued LLVM Phabricator instance.

Add process name to Asan logfile name and report message
ClosedPublic

Authored by ygribov on Oct 10 2014, 3:44 AM.

Details

Summary

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.

Diff Detail

Event Timeline

ygribov updated this revision to Diff 14710.Oct 10 2014, 3:44 AM
ygribov retitled this revision from to Add process name to Asan logfile name and report message.
ygribov updated this object.
ygribov edited the test plan for this revision. (Show Details)
ygribov added reviewers: kcc, samsonov, eugenis.
ygribov added a subscriber: Unknown Object (MLST).
eugenis edited edge metadata.Oct 10 2014, 4:32 AM

A test?

lib/asan/asan_rtl.cc
566 ↗(On Diff #14710)

Why do you need this?

lib/sanitizer_common/sanitizer_common.cc
241

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.

lib/sanitizer_common/sanitizer_linux.cc
699

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.

ygribov added inline comments.Oct 10 2014, 4:52 AM
lib/asan/asan_rtl.cc
566 ↗(On Diff #14710)

Sorry, forgot to remove this.

lib/sanitizer_common/sanitizer_common.cc
241

Doesn't mutex take care of that?

ygribov edited edge metadata.Oct 10 2014, 4:52 AM
eugenis added inline comments.Oct 10 2014, 4:58 AM
lib/sanitizer_common/sanitizer_common.cc
241
emaste added a subscriber: emaste.Oct 10 2014, 6:05 AM
emaste added inline comments.
lib/sanitizer_common/sanitizer_linux.cc
696–700

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")

ygribov updated this revision to Diff 14790.Oct 13 2014, 3:15 AM

Addressed reviewers' comments.

ygribov added inline comments.Oct 13 2014, 3:52 AM
lib/sanitizer_common/sanitizer_common.cc
239

Ok, these should be removed.

ygribov updated this revision to Diff 14802.Oct 13 2014, 9:04 AM

Updated tests and removed the now redundant code.

eugenis accepted this revision.Oct 20 2014, 10:13 AM
eugenis edited edge metadata.

LGTM

lib/sanitizer_common/sanitizer_common.cc
252

There is Max() definition in sanitizer_common.

lib/sanitizer_common/sanitizer_common.h
179

You've got 2 overloads of ReadBinayName that do very different things.
Rename to GetBinaryName?

This revision is now accepted and ready to land.Oct 20 2014, 10:13 AM
ygribov added inline comments.Oct 21 2014, 1:13 AM
lib/sanitizer_common/sanitizer_common.cc
252

Ah, ok, I looked at min/max in tsan_defs.h and thought that it's ok to reinvent adhoc max.

lib/sanitizer_common/sanitizer_common.h
179

Good point.

ygribov updated this revision to Diff 15173.Oct 21 2014, 1:50 AM
ygribov edited edge metadata.

Updated based on Eugene's comments.

The ReadBinaryName change wrt my FreeBSD comment LGTM.

Evgeny, is anything else needed here?

LGTM
Sorry for the delay

samsonov edited edge metadata.Nov 5 2014, 7:02 PM

Please fix a couple of things below.

Thanks for working on this!

lib/sanitizer_common/sanitizer_common.cc
230

This should be proc_self_exe_cache_lock(LINKER_INITIALIZED);

248

I think we already have it: StripModuleName().
You may need to fix its implementation.

ygribov updated this revision to Diff 15844.Nov 6 2014, 1:32 AM
ygribov edited edge metadata.

Updated based on recent review.

samsonov accepted this revision.Nov 6 2014, 11:06 AM
samsonov edited edge metadata.

LGTM with one last nit. Feel free to commit after fixing it.

lib/sanitizer_common/sanitizer_mac.cc
324

Shouldn't you return 0 in this case? (if we decide to keep "uptr" as return value of ReadBinaryName, we never use it anywhere)

lib/sanitizer_common/sanitizer_win.cc
534

ditto

Perhaps it's time to ask for a commit access...

lib/sanitizer_common/sanitizer_mac.cc
324

Looks like you are right, readlink does not include the terminating 0 into the return value. Not sure about FreeBSD though.

Sure, please email Chris (http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). Feel free to CC me and/or Kostya.

emaste added inline comments.Nov 9 2014, 6:58 PM
lib/sanitizer_common/sanitizer_common.cc
230–232

should probably be binary_name_cache_str or similar instead so the name is appropriate on all OSes.

Landed in r221896.

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[0]);
...
-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?

kcc edited edge metadata.Dec 2 2014, 5:05 PM

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)
Thanks!

In D5724#35, @ygribov wrote:

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[0]);
...
-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?

Can we enforce the correct alignment of the array of uptr's?

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)).

ygribov closed this revision.Jan 27 2015, 12:09 AM