| Message ID | 20121203091815.GP2315@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Dec 3, 2012 at 1:18 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Mon, Dec 03, 2012 at 11:06:48AM +0400, Konstantin Serebryany wrote: >> This patch copies the asan tests almost, but not quite, verbatim from upstream. >> Since the patch is not in attachment (and gmail messes up with inlined >> patches) I can't see the exact changes. > > Sending patches inline rather than as attachments is the preferred way for > gcc-patches, you can always grab it from the gcc-patches ml archive: > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg02557.html > (then click on Raw text). Got it, thanks. > > I'm attaching the diff for asan_test.cc from llvm anyway. > >> I see #ifdef ASAN_AVOID_EXPENSIVE_TESTS, which I don't really like >> because I'd rather fix the test than disable it. > > The test isn't disabled, just by default limited to 30 threads instead of > 1000, because that really will ruin testing for everybody with ulimit -u > in 1024-ish range. Even 500 threads would be undesirable for that. Which is the same as disabling it. Unfortunately, we don't have a good automated way to test asan performance, so this test is guarding us from performance degradation in the asan's pthread wrappers. If there is a bug there, we may not notice it on 30 threads, but may (with a high probability) on 1000 threads. Anyway, does http://llvm.org/viewvc/llvm-project?rev=169118&view=rev solve the problem? It adds ASAN_AVOID_EXPENSIVE_TESTS and checks the results of all pthread_{create,join} calls in tests. --kcc > >> Can we commit the tests 100% verbatim, and then fix them as separate commits >> (preferably, by fixing the tests upstream and doing the merge with >> libsanitizer/merge.sh)? > > I'd prefer delay committing the patch over causing random testsuite failures > elsewhere. > > Jakub
On Mon, Dec 03, 2012 at 01:51:50PM +0400, Konstantin Serebryany wrote: > > I'm attaching the diff for asan_test.cc from llvm anyway. > > > >> I see #ifdef ASAN_AVOID_EXPENSIVE_TESTS, which I don't really like > >> because I'd rather fix the test than disable it. > > > > The test isn't disabled, just by default limited to 30 threads instead of > > 1000, because that really will ruin testing for everybody with ulimit -u > > in 1024-ish range. Even 500 threads would be undesirable for that. > > Which is the same as disabling it. > Unfortunately, we don't have a good automated way to test asan performance, > so this test is guarding us from performance degradation in the asan's > pthread wrappers. I understand that, that is why the test by default, when not run as part of dejagnu, or even in dejagnu when requested expensive tests, runs 1000 threads instead of 30. > Anyway, does http://llvm.org/viewvc/llvm-project?rev=169118&view=rev > solve the problem? > It adds ASAN_AVOID_EXPENSIVE_TESTS and checks the results of all > pthread_{create,join} calls in tests. Yes, thanks. So, is the patch ok to commit to GCC with the imported tests remerged from upstream (or do I need to repost the patch for that)? Jakub
On Mon, Dec 3, 2012 at 3:05 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Mon, Dec 03, 2012 at 01:51:50PM +0400, Konstantin Serebryany wrote: >> > I'm attaching the diff for asan_test.cc from llvm anyway. >> > >> >> I see #ifdef ASAN_AVOID_EXPENSIVE_TESTS, which I don't really like >> >> because I'd rather fix the test than disable it. >> > >> > The test isn't disabled, just by default limited to 30 threads instead of >> > 1000, because that really will ruin testing for everybody with ulimit -u >> > in 1024-ish range. Even 500 threads would be undesirable for that. >> >> Which is the same as disabling it. >> Unfortunately, we don't have a good automated way to test asan performance, >> so this test is guarding us from performance degradation in the asan's >> pthread wrappers. > > I understand that, that is why the test by default, when not run as part of > dejagnu, or even in dejagnu when requested expensive tests, runs 1000 > threads instead of 30. > >> Anyway, does http://llvm.org/viewvc/llvm-project?rev=169118&view=rev >> solve the problem? >> It adds ASAN_AVOID_EXPENSIVE_TESTS and checks the results of all >> pthread_{create,join} calls in tests. > > Yes, thanks. So, is the patch ok to commit to GCC with the imported tests remerged > from upstream Yes, ok to commit. Thanks! --kcc > (or do I need to repost the patch for that)? > > Jakub
--- /usr/src/llvm/projects/compiler-rt/lib/asan/tests/asan_test.cc 2012-11-30 14:23:48.525229790 +0100 +++ asan_test.cc 2012-12-03 10:12:14.503366704 +0100 @@ -1,7 +1,5 @@ //===-- asan_test.cc ------------------------------------------------------===// // -// The LLVM Compiler Infrastructure -// // This file is distributed under the University of Illinois Open Source // License. See LICENSE.TXT for details. // @@ -481,10 +479,16 @@ void *ManyThreadsWorker(void *a) { } TEST(AddressSanitizer, ManyThreadsTest) { - const size_t kNumThreads = SANITIZER_WORDSIZE == 32 ? 30 : 1000; - pthread_t t[kNumThreads]; +#ifdef ASAN_AVOID_EXPENSIVE_TESTS + const size_t kMaxThreads = 30; +#else + const size_t kMaxThreads = SANITIZER_WORDSIZE == 32 ? 30 : 1000; +#endif + pthread_t t[kMaxThreads]; + size_t kNumThreads = kMaxThreads; for (size_t i = 0; i < kNumThreads; i++) { - pthread_create(&t[i], 0, (void* (*)(void *x))ManyThreadsWorker, (void*)i); + if (pthread_create(&t[i], 0, (void* (*)(void *x))ManyThreadsWorker, (void*)i)) + kNumThreads = i; } for (size_t i = 0; i < kNumThreads; i++) { pthread_join(t[i], 0);