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

[Konstantin Serebryany <konstantin.s.serebryany@gmail.com>]

Looks good.

Long term the fact that we need to completely fork these tests makes me sad.
I'd really love to see a way to make them compiler-neutral.
All we need is to mock FileCheck (or put it into libsanitizer/aux somehow).
Ideas?

--kcc


On Sat, Dec 1, 2012 at 12:35 AM, Wei Mi <wmi@google.com> wrote:
> Thanks for the comments! Here is the second version patch. Please see
> if it is ok.
> (-Wno-attributes is kept or else we will get a warning because of
> __attribute__((always_inline))).
>
> These tests are excluded for now because unsupported features or needs
> some twists to be included.
> blacklist-1.c                             (use -asan-blacklist=)
> initialization-blacklist-1.C         (use -asan-blacklist=)
> initialization-bug-1.c                (use -asan-initialization-order)
> initialization-nobug-1.C           (use -asan-initialization-order)
> initialization-bug-any-order.cc (use -asan-initialization-order)
> interface-symbols-1.c              (needs to be added to
> libsanitizer/Makefile.am)
> log-path_test.cc                       (needs to twist dg-final
> commands, skip for now)
>
> Test results:
> gcc summary
> # of expected passes            248
> # of unexpected failures                39
> # of unsupported tests          27
>
> g++ summary
> # of expected passes            468
> # of unexpected failures        56
> # of unsupported tests          50
>
> Thanks,
> Wei.
>
> On Wed, Nov 28, 2012 at 2:14 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> 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