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?
> +#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 } } }
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.
Jakub
