Re: [PATCH] asan_test.cc from llvm

[Jakub Jelinek <jakub@redhat.com>]

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