From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9528 invoked by alias); 3 Dec 2012 07:16:55 -0000 Received: (qmail 9500 invoked by uid 22791); 3 Dec 2012 07:16:50 -0000 X-SWARE-Spam-Status: No, hits=-5.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_CP,TW_KC,TW_LV X-Spam-Check-By: sourceware.org Received: from mail-la0-f47.google.com (HELO mail-la0-f47.google.com) (209.85.215.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 03 Dec 2012 07:16:39 +0000 Received: by mail-la0-f47.google.com with SMTP id u2so1877958lag.20 for ; Sun, 02 Dec 2012 23:16:37 -0800 (PST) Received: by 10.112.83.133 with SMTP id q5mr3930608lby.40.1354518997439; Sun, 02 Dec 2012 23:16:37 -0800 (PST) MIME-Version: 1.0 Received: by 10.112.135.105 with HTTP; Sun, 2 Dec 2012 23:16:17 -0800 (PST) In-Reply-To: References: <20121128101420.GG2315@tucnak.redhat.com> From: Konstantin Serebryany Date: Mon, 03 Dec 2012 07:16:00 -0000 Message-ID: Subject: Re: [PATCH] asan unit tests from llvm lit-test To: Wei Mi Cc: Jakub Jelinek , GCC Patches , David Li , Diego Novillo , Kostya Serebryany , Dodji Seketeli Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2012-12/txt/msg00096.txt.bz2 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 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 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 >> >> 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 >>> +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 >>> +#include >>> +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