I've upstreamed most of your changes, please take a look.
Looks like we will be able to use libsanitizer/merge.sh to update the
test code as well.
On Fri, Nov 30, 2012 at 2:14 PM, Jakub Jelinek <ja...@redhat.com> wrote:
> On Fri, Nov 30, 2012 at 01:32:52PM +0400, Konstantin Serebryany wrote:
>> Ideally, I would like to limit the differences from upstream.
>> I'll put some of your changes upstream, for others I'd ask you to
>> consider other choices.
>>
>> -#include "gtest/gtest.h"
>> +#include "dejagnu-gtest.h"
>>
>> Maybe like this?
>>
>> #if ASAN_USE_DEJAGNU_GTEST
>> #include "dejagnu-gtest.h"
>> #else
>> #include "gtest/gtest.h"
>> #endif
>
> Sure, I'm fine with that.
>
>> Can we have gcc_asan_test.C which will #include the unchanged (modulo
>> the comment header) asan_test.cc
>> and have dejagnu lines there?
>>
>> Like this:
>> // { dg-do run { target { mmap && pthread } } }
>> ...
>> #include asan_test.cc
>
> Yeah, I can do that, advantage will be that the testcase is named the same,
> asan.exp currently runs only *.C testcases (so that *.cc is left to helper
> files etc.). So there will be a small asan_test.C with the dg markup.
>
>> +#elif defined(__SANITIZE_ADDRESS__)
>> + bool asan = 1;
>>
>> I'll put this upstream.
>
> Thanks, if GCC ever introduces the __has_feature magic macro, it can be
> dropped, but for now... Note elsewhere I'm using the __SANITIZE_ADDRESS__
> define as a test whether this is for GCC or other compilers, because
> I believe clang defines __GNUC__ macro or similar, I'd need to
> #if defined (__GNUC__) && !defined (__clang__) or something?
I'd prefer to avoid such hairy ifdefs...
>
>> +#ifdef __SANITIZE_ADDRESS__
>> + // Avoid this test during dejagnu testing, it is too expensive
>> + if (getenv ("GCC_TEST_RUN_EXPENSIVE") == NULL)
>> + return;
>> +#endif
>>
>> I'd prefer to simply put this w/o ifdef.
>
> Then even in llvm the expensive test won't run unless you put
> GCC_TEST_RUN_EXPENSIVE=1 into environment. That would be weird
> for a llvm testcase to use GCC in the name. Alternative would be
> to use some other env var name, like
> ASAN_TEST_RUN_EXPENSIVE, and I could adjust asan.exp to set that
> env var, or it could be a macro instead, guarding the whole test,
> #ifndef ASAN_AVOID_EXPENSIVE_TESTS
> (TEST(AddressSanitizer, ManyThreadsTest) {
> ...
> }
> #endif
> and I could in asan_test.C add:
> // { dg-additional-options "-DASAN_AVOID_EXPENSIVE_TESTS" { target { !
> run_expensive_tests } } }
I can add ASAN_ALLOW_SLOW_TESTS to asan_test_config.h and then #if
ASAN_ALLOW_SLOW_TESTS around ManyThreadsTest.
I did not do it yet, because I would like to understand why it's slow.
On my box (with clang+gtest) it runs in 0.6 seconds, and the entire
test suite takes 60 seconds.
Note that when running gtest tests in llvm test harness the tests are
sharded, so on a multicore machine the test takes < 10 seconds.
>
> Or, if you want even upstream to avoid it by default, it could
> be a positive test, #ifdef ASAN_RUN_EXPENSIVE_TESTS or similar.
>
>>
>> -# error "please define ASAN_HAS_BLACKLIST"
>> +//# error "please define ASAN_HAS_BLACKLIST"
>>
>> You can add -DASAN_HAS_BLACKLIST=0 to the command line.
>> If/when gcc gets blacklist support, we'll redefine it to 1
>
> Okay.
>
>> +#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
>>
>> this is upstreamable
>>
>> +#ifdef __GNUC__
>> +# define break_optimization(arg) __asm__ __volatile__ ("" : : "r"
>> (arg) : "memory")
>> +#endif
>> +
>>
>> That's a nice piece of magic, let me use this too.
>
> For pointers/integers it can be even as simple as
> __asm__ ("" : "+g" (val));
> making compiler forget about val, obviously that doesn't work for
> aggregates. Anyway, the reason for the macro is both that I wanted to
> avoid compiling in another file, and for LTO it wouldn't work anyway.
>
> BTW, I had to add -Wno-unused-but-set-variable option because without
> that there was a warning about one of the variables, I think it was
> stack_string in StrLenOOBTest. Adding (void) stack_string; somewhere
> would shut it up.
I added break_optimization instead.
>
> Jakub
