Re: [PATCH] asan unit tests from llvm lit-test

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

On Wed, Nov 28, 2012 at 01:15:20AM -0800, Wei Mi wrote:
> I try to migrate the left asan lit-tests from llvm (class3). This is a
> preliminary version patch. Please forgive it has many mistakes.

Thanks for working on it.

> A known problems: I hardcoded -m32 in (set link_flags
> "[asan_link_flags [get_multilibs -m32]] $link_flags") in
> gcc/testsuite/lib/asan-dg.exp to make 32 bit library path included in
> ld_library_path. I don't know the elegant way to fix it.

That is wrong, no *.exp file should do anything with -m32/-m64.
If user wants to test both -m32 and -m64, it should be done through
RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}'
on the command line of make check if desired (or any other options deemed
necessary to test).  Not all targets support both -m32 and -m64 (e.g. even
i686-linux doesn't), some targets have other ABI options (e.g. -m31/-m64 on
s390x, mips has more variants, etc.).  It must be user's choice what he
wants to test for what multilibs.

>         * g++.dg/asan/linux: New, migrate from llvm asan lit-test.
>         * g++.dg/asan/linux/interception-test-1.C: Likewise.
>         * g++.dg/asan/linux/interception-failure-test-1.C: Likewise.
>         * g++.dg/asan/linux/interception-malloc-test-1.C: Likewise.

Why the linux/ subdirectories (which you seem to run for all targets
anyway)?  That doesn't make any sense.  All tests that won't run on certain
targets because of some required features (whether it is e.g. dlopen, mmap,
pthreads) should be guarded, e.g.
// { dg-require-effective-target pthread }
or
/* { dg-run { target pthread } } */
and similar.  If some check_effective_target_* tcl test is missing, it can
be always added (e.g. dlopen doesn't have any, and you can't assume dlopen
works everywhere).

>         * g++.dg/asan/Helpers: Likewise.
>         * g++.dg/asan/Helpers/initialization-blacklist-1.tmp: Likewise.
>         * g++.dg/asan/Helpers/initialization-blacklist-extra-1.C: Likewise.

We aren't a CamelCase shop, I'd strongly prefer if we could avoid that
ugliness.  Ditto for SharedLibs/ etc. subdirs.  And why you need the subdirs
at all?  The usual way how to handle e.g. the dg-additional-sources is just
make sure the additional sources are either named in a way that doesn't
match the normal wildcard (for C++ e.g. *.cc instead of *.C) or add some dg
directive in there that it won't run, or be dg-do compile only test etc.

> +    if { [string match "*blacklist-1.c" $source] } {
> +      set blacklist_options $options
> +      set blist_tmp [glob $srcdir/c-c++-common/asan/Helpers/blacklist-1.tmp]
> +      lappend blacklist_options "additional_flags=-asan-blacklist=$blist_tmp"
> +      set result [eval [list saved_asan_target_compile $source $dest $type $blacklist_options]]
> +      return $result
> +    } elseif { [string match "*interface-symbols-1.c" $source] } {
> +      set result [eval [list saved_asan_target_compile \
> +                        $source "interface-symbols-1.exe" \
> +                        "executable" $options]]
> +      if { [string match "" $result] } {
> +        set exefile [glob interface-symbols-1.exe]
> +        set asan_interface_h [glob $srcdir/../../libsanitizer/include/sanitizer/asan_interface.h]
> +        set script [glob $srcdir/c-c++-common/asan/Helpers/interface_symbols.sh]
> +        set diff_result [exec sh $script $exefile $asan_interface_h]
> +        if { ![string match "" $diff_result] } {
> +          fail "$source -- diff result not empty: $diff_result"
> +        }
> +      }
> +    } elseif { [string match "*initialization-bug-any-order-1.c" $source] } {
> +      set auxfile [glob $srcdir/c-c++-common/asan/Helpers/initialization-bug-extra-1.c]
> +      global subtest
> +      if { [string match "subtest1" $subtest] } {
> +        set source "$source $auxfile"
> +      } else {
> +        set source "$auxfile $source"
> +      }
> +      set result [eval [list saved_asan_target_compile $source $dest $type $options]]
> +    } else {
> +      set result [eval [list saved_asan_target_compile $source $dest $type $options]]
> +    }

This is too ugly.  asan.exp shouldn't turn into yet another vect.exp, the
ideal is that for adding new tests you don't need to tweak any *.exp and add
exceptions for that, unless there is no other way.  So, preferrably in asan/
dir should stay tests that can be just handled the standard way, and there
can be some extra subdirectory that will handle hard to handle tests.
Say g++.dg/asan/special/ could have its own asan-special.exp or similar.
Note that e.g. for building shared libraries you really need to guard it
with appropriate target checks.

> +foreach srcfile [lsort [glob -nocomplain \
> +                        $srcdir/$subdir/*.c \
> +                        $srcdir/c-c++-common/asan/*.c \
> +                        $srcdir/c-c++-common/asan/linux/*.c]] {
> +  set asan_torture_options $default_asan_torture_options
> +  if { [string match "*force-inline-opt0-1.c" $srcfile] } { 
> +    set asan_torture_options [list { -O0 -m64 } { -O1 -m64 }]

As said earlier, no -m64/-m32 here, and if at all possible, no special
casing of tests in *.exp.  If you want to change the set of options
at which some test is run, i.e. you don't want to iterate over all the
options, just use dg-skip-if.  See
http://gcc.gnu.org/onlinedocs/gccint/Directives.html
for details about it.

> +  } elseif { [string match "*sleep-before-dying-1.c" $srcfile] } {
> +    setenv ASAN_OPTIONS "sleep_before_dying=1"
> +    set asan_torture_options [list { -O2 }]

For env options, I believe we don't have any dg directive right now to
set env vars for runtime tests, but the best way would be to add it,
dg-env-var or similar.  Or better yet, does libasan have a way to set
the options through some function call?  Then you wouldn't have to
set env vars...

> --- gcc/testsuite/g++.dg/asan/deep-thread-stack-1.C	(revision 0)
> +++ gcc/testsuite/g++.dg/asan/deep-thread-stack-1.C	(revision 0)
> @@ -0,0 +1,55 @@
> +/* { dg-do run } */
> +/* { dg-shouldfail "asan" } */

In C++ only tests, just use // comments instead of /* ... */
> +
> +#include <pthread.h>

That is exactly where you need to require effective target pthread...

> +/* { dg-options "-asan-initialization-order" } */

AFAIK gcc doesn't have that option, and if it had, it wouldn't be of this
form.  -fasan-initialization-order, --param asan-initialization-order=1 or
similar, perhaps, but not -asan-initialization-order.
But more generally, adding tests into GCC testsuite for unimplemented
features is undesirable, you can prepare the tests and post a rough patch
how they could look like, but it shouldn't be committed until the feature
is implemented.

> +/* { dg-additional-sources "Helpers/initialization-blacklist-extra-1.C" } */

> +//
> +//                     The LLVM Compiler Infrastructure

I believe we've been removing the above two lines from libsanitizer, so they
should probably be removed also from the tests?

> --- gcc/testsuite/lib/asan-dg.exp	(revision 193881)
> +++ gcc/testsuite/lib/asan-dg.exp	(working copy)
> @@ -75,6 +75,7 @@ proc asan_init { args } {
>  	    set link_flags "[asan_link_flags [get_multilibs ${TOOL_OPTIONS}]]"
>  	} else {
>  	    set link_flags "[asan_link_flags [get_multilibs]]"
> +	    set link_flags "[asan_link_flags [get_multilibs -m32]] $link_flags"

As has been said earlier, please don't do this.

> --- gcc/testsuite/c-c++-common/asan/interface-symbols-1.c	(revision 0)
> +++ gcc/testsuite/c-c++-common/asan/interface-symbols-1.c	(revision 0)
> @@ -0,0 +1,24 @@
> +// Check the presense of interface symbols in compiled file.
> +
> +// RUN: %clang -fsanitize=address -dead_strip -O2 %s -o %t.exe
> +// RUN: nm %t.exe | egrep " [TW] " | sed "s/.* T //" | sed "s/.* W //" \
> +// RUN:    | grep "__asan_" | sed "s/___asan_/__asan_/" > %t.symbols
> +// RUN: cat %p/../../../include/sanitizer/asan_interface.h \
> +// RUN:    | sed "s/\/\/.*//" | sed "s/typedef.*//" \
> +// RUN:    | grep "__asan_.*(" | sed "s/.* __asan_/__asan_/;s/(.*//" \
> +// RUN:    > %t.interface
> +// RUN: echo __asan_report_load1 >> %t.interface
> +// RUN: echo __asan_report_load2 >> %t.interface
> +// RUN: echo __asan_report_load4 >> %t.interface
> +// RUN: echo __asan_report_load8 >> %t.interface
> +// RUN: echo __asan_report_load16 >> %t.interface
> +// RUN: echo __asan_report_store1 >> %t.interface
> +// RUN: echo __asan_report_store2 >> %t.interface
> +// RUN: echo __asan_report_store4 >> %t.interface
> +// RUN: echo __asan_report_store8 >> %t.interface
> +// RUN: echo __asan_report_store16 >> %t.interface
> +// RUN: cat %t.interface | sort -u | diff %t.symbols -
> +
> +/* { dg-options "-static-libasan -lpthread -ldl" } */
> +
> +int main() { return 0; }

This kind of test IMHO doesn't belong to the dejagnu testsuite,
if you really want to do it, it should be done somewhere in
libsanitizer/asan/ Makefile.am as part of building the library.

> --- gcc/testsuite/c-c++-common/asan/force-inline-opt0-1.c	(revision 0)
> +++ gcc/testsuite/c-c++-common/asan/force-inline-opt0-1.c	(revision 0)
> @@ -0,0 +1,15 @@
> +// This test checks that we are no instrumenting a memory access twice
> +// (before and after inlining)
> +
> +/* { dg-do run } */
> +/* { dg-options "-Wno-attributes" } */
> +__attribute__((always_inline))

Why -Wno-attributes?

> +#include <string.h>
> +int main(int argc, char **argv) {
> +  static char XXX[10];
> +  static char YYY[10];
> +  static char ZZZ[10];
> +  memset(XXX, 0, 10);
> +  memset(YYY, 0, 10);
> +  memset(ZZZ, 0, 10);
> +  int res = YYY[argc * 10];  // BOOOM
> +  res += XXX[argc] + ZZZ[argc];

argc/argv using tests are not portable to all targets, you can't rely
argc isn't e.g. zero.  Better just have some global variable, say,
int one = 1;
and at the beginning of main do asm volatile ("" : : : "memory");
to let compiler forget about the value it has, or just make the variable
volatile int one = 1;

> @@ -0,0 +1,22 @@
> +/* { dg-do run } */

I'd expect you want /* { dg-options "-fno-builtin-strncpy" } */
here, otherwise it is reported inside of main directly, rather than in the
strncpy interceptor.

> +/* { dg-shouldfail "asan" } */
> +
> +#include <string.h>
> +#include <stdlib.h>
> +int main(int argc, char **argv) {
> +  char *hello = (char*)malloc(6);
> +  strcpy(hello, "hello");
> +  char *short_buffer = (char*)malloc(9);
> +  strncpy(short_buffer, hello, 10);  // BOOM
> +  return short_buffer[8];
> +}
> +
> +/* { dg-output "WRITE of size 1 at 0x\[0-9a-f\]+ thread T0\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "    #0 0x\[0-9a-f\]+ (in _*(interceptor_|)strncpy|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "    #1 0x\[0-9a-f\]+ (in _*main \[^\n\r]*(strncpy-overflow-1.c:10|\[?]\[?]:0)|\[(\]).*(\n|\r\n|\r)" } */
> +/* { dg-output "0x\[0-9a-f\]+ is located 0 bytes to the right of 9-byte region\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "allocated by thread T0 here:\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "    #0 0x\[0-9a-f\]+ (in _*(interceptor_|)malloc|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "    #1 0x\[0-9a-f\]+ (in _*main \[^\n\r]*(strncpy-overflow-1.c:9|\[?]\[?]:0)|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */

	Jakub