From patchwork Sat Feb 16 09:15:14 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dodji Seketeli X-Patchwork-Id: 220952 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id C28FC2C0080 for ; Sat, 16 Feb 2013 20:15:34 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1361610935; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Received:From:To:Cc:Subject:References:Date:In-Reply-To: Message-ID:User-Agent:MIME-Version:Content-Type:Mailing-List: Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:Sender:Delivered-To; bh=Zt8EsXDWGD2kOGfNNYyn8pgOGd4=; b=ET5fjbIcDV8U2rqhgBD8tUZgPylSvSe3gaed1OzOjwdlotddaLGC8rmd+BCoZD 6UQkTIpFg7tBJhTXPPr07qVVc8XnbwasAa5GMUCEelMgxFZ04KWtVCNb4ggr11kG OgVy4emwsnb8yzh3DfUkSY7BpZa4VhajQOAo9S9K0mLLc= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Received:From:To:Cc:Subject:References:X-URL:Date:In-Reply-To:Message-ID:User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=NjQDTAp+tOIq7z6VflmWe7ASeo41VJHKJibAzhj5cRImePUI+8CBV1vQqGrXAb 9U77HINp2/RNr5TSS8ReCxhFZ8K+q0MCCnhbITxN4zUOJ5uoFvcgWQ7s9YRwtmiI D5oVDbTPNHSu7G7lm5tOeHA0AEn5XAWh1m/B/NR2ljraE=; Received: (qmail 27433 invoked by alias); 16 Feb 2013 09:15:28 -0000 Received: (qmail 27424 invoked by uid 22791); 16 Feb 2013 09:15:27 -0000 X-SWARE-Spam-Status: No, hits=-5.9 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, KHOP_SPAMHAUS_DROP, RCVD_IN_DNSWL_HI, RCVD_IN_HOSTKARMA_W, RP_MATCHES_RCVD, SPF_HELO_PASS, TW_TM X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 16 Feb 2013 09:15:18 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r1G9FHQ0018486 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sat, 16 Feb 2013 04:15:17 -0500 Received: from localhost (ovpn-116-33.ams2.redhat.com [10.36.116.33]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r1G9FFoZ016187 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 16 Feb 2013 04:15:16 -0500 Received: by localhost (Postfix, from userid 1000) id 03D274064A; Sat, 16 Feb 2013 10:15:15 +0100 (CET) From: Dodji Seketeli To: Jakub Jelinek Cc: Jack Howarth , Konstantin Serebryany , Dmitry Vyukov , GCC Patches Subject: Re: [PATCH] [asan] Fix for PR asan/56330 References: <874nhdb8n8.fsf@redhat.com> <20130215230105.GA2210@bromo.med.uc.edu> <20130216083730.GU1215@tucnak.zalov.cz> <87r4kgabr0.fsf@redhat.com> <20130216085335.GV1215@tucnak.zalov.cz> X-URL: http://www.redhat.com Date: Sat, 16 Feb 2013 10:15:14 +0100 In-Reply-To: <20130216085335.GV1215@tucnak.zalov.cz> (Jakub Jelinek's message of "Sat, 16 Feb 2013 09:53:35 +0100") Message-ID: <87fw0waacd.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Jakub Jelinek writes: > On Sat, Feb 16, 2013 at 09:44:51AM +0100, Dodji Seketeli wrote: >> Right. My first bootstrap actually caught this, I updated the patch >> locally to just modify that test and sent out the wrong one. Sorry for >> this. This is the patch that actually passed bootstrap. > > Ok. Actually, please change gcc/testsuite/c-c++-common/asan/pr56330.c > to the same thing, Oops, right. Done below. > I've just verified it ICEs the same way without the patch > even when it is > int > foo (void) > { > int d = __builtin_memcmp (s.a, e, 100); > d += __builtin_memcmp (s.a, e, 200); > return d; > } Yes, I have just verified this too, thanks. > > (while with > __builtin_memcpy (s.a, e, 100); > __builtin_memcpy (s.a, e, 200); > it doesn't ICE without the patch). Hmmh, interesting. > While pr56330.c test look to be redundant, it is at least tortured at all > compilation options, so it tests something that the other tests don't. Yes, that is why I kept it in the set of tests. > No need to rebootstrap/retest. I re-ran the tests locally w/o bootstrap though. I am going to commit this now. Thanks. gcc/ * asan.c (get_mem_refs_of_builtin_call): White space and style cleanup. (instrument_mem_region_access): Do not forget to always put instrumentation of the of 'base' and 'base + len' in a "if (len != 0) statement, even for cases where either 'base' or 'base + len' are not instrumented -- because they have been previously instrumented. Simplify the logic by putting all the statements instrument 'base + len' inside a sequence, and then insert that sequence right before the current insertion point. Then, to instrument 'base + len', just get an iterator on that statement. And do not forget to update the pointer to iterator the function received as argument. gcc/testsuite/ * c-c++-common/asan/no-redundant-instrumentation-4.c: New test file. * c-c++-common/asan/no-redundant-instrumentation-5.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-6.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-7.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-8.c: Likewise. * c-c++-common/asan/pr56330.c: Likewise. * c-c++-common/asan/no-redundant-instrumentation-1.c (test1): Ensure the size argument of __builtin_memcpy is a constant. --- gcc/ChangeLog | 16 ++++ gcc/asan.c | 97 ++++++++++++---------- gcc/testsuite/ChangeLog | 12 +++ .../asan/no-redundant-instrumentation-1.c | 2 +- .../asan/no-redundant-instrumentation-4.c | 13 +++ .../asan/no-redundant-instrumentation-5.c | 13 +++ .../asan/no-redundant-instrumentation-6.c | 14 ++++ .../asan/no-redundant-instrumentation-7.c | 23 +++++ .../asan/no-redundant-instrumentation-8.c | 14 ++++ gcc/testsuite/c-c++-common/asan/pr56330.c | 24 ++++++ 10 files changed, 183 insertions(+), 45 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c create mode 100644 gcc/testsuite/c-c++-common/asan/pr56330.c diff --git a/gcc/asan.c b/gcc/asan.c index a569479..67236a9 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -747,20 +747,17 @@ get_mem_refs_of_builtin_call (const gimple call, got_reference_p = true; } - else - { - if (dest) - { - dst->start = dest; - dst->access_size = access_size; - *dst_len = NULL_TREE; - *dst_is_store = is_store; - *dest_is_deref = true; - got_reference_p = true; - } - } + else if (dest) + { + dst->start = dest; + dst->access_size = access_size; + *dst_len = NULL_TREE; + *dst_is_store = is_store; + *dest_is_deref = true; + got_reference_p = true; + } - return got_reference_p; + return got_reference_p; } /* Return true iff a given gimple statement has been instrumented. @@ -1535,8 +1532,15 @@ instrument_mem_region_access (tree base, tree len, /* If the beginning of the memory region has already been instrumented, do not instrument it. */ - if (has_mem_ref_been_instrumented (base, 1)) - goto after_first_instrumentation; + bool start_instrumented = has_mem_ref_been_instrumented (base, 1); + + /* If the end of the memory region has already been instrumented, do + not instrument it. */ + tree end = asan_mem_ref_get_end (base, len); + bool end_instrumented = has_mem_ref_been_instrumented (end, 1); + + if (start_instrumented && end_instrumented) + return; if (!is_gimple_constant (len)) { @@ -1562,37 +1566,39 @@ instrument_mem_region_access (tree base, tree len, /* The 'then block' of the 'if (len != 0) condition is where we'll generate the asan instrumentation code now. */ - gsi = gsi_start_bb (then_bb); + gsi = gsi_last_bb (then_bb); } - /* Instrument the beginning of the memory region to be accessed, - and arrange for the rest of the intrumentation code to be - inserted in the then block *after* the current gsi. */ - build_check_stmt (location, base, &gsi, /*before_p=*/true, is_store, 1); - - if (then_bb) - /* We are in the case where the length of the region is not - constant; so instrumentation code is being generated in the - 'then block' of the 'if (len != 0) condition. Let's arrange - for the subsequent instrumentation statements to go in the - 'then block'. */ - gsi = gsi_last_bb (then_bb); - else - *iter = gsi; - - update_mem_ref_hash_table (base, 1); + if (!start_instrumented) + { + /* Instrument the beginning of the memory region to be accessed, + and arrange for the rest of the intrumentation code to be + inserted in the then block *after* the current gsi. */ + build_check_stmt (location, base, &gsi, /*before_p=*/true, is_store, 1); + + if (then_bb) + /* We are in the case where the length of the region is not + constant; so instrumentation code is being generated in the + 'then block' of the 'if (len != 0) condition. Let's arrange + for the subsequent instrumentation statements to go in the + 'then block'. */ + gsi = gsi_last_bb (then_bb); + else + { + *iter = gsi; + /* Don't remember this access as instrumented, if length + is unknown. It might be zero and not being actually + instrumented, so we can't rely on it being instrumented. */ + update_mem_ref_hash_table (base, 1); + } + } - after_first_instrumentation: + if (end_instrumented) + return; /* We want to instrument the access at the end of the memory region, which is at (base + len - 1). */ - /* If the end of the memory region has already been instrumented, do - not instrument it. */ - tree end = asan_mem_ref_get_end (base, len); - if (has_mem_ref_been_instrumented (end, 1)) - return; - /* offset = len - 1; */ len = unshare_expr (len); tree offset; @@ -1639,8 +1645,6 @@ instrument_mem_region_access (tree base, tree len, base, NULL); gimple_set_location (region_end, location); gimple_seq_add_stmt_without_update (&seq, region_end); - gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT); - gsi_prev (&gsi); /* _2 = _1 + offset; */ region_end = @@ -1649,13 +1653,18 @@ instrument_mem_region_access (tree base, tree len, gimple_assign_lhs (region_end), offset); gimple_set_location (region_end, location); - gsi_insert_after (&gsi, region_end, GSI_NEW_STMT); + gimple_seq_add_stmt_without_update (&seq, region_end); + gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT); /* instrument access at _2; */ + gsi = gsi_for_stmt (region_end); build_check_stmt (location, gimple_assign_lhs (region_end), &gsi, /*before_p=*/false, is_store, 1); - update_mem_ref_hash_table (end, 1); + if (then_bb == NULL) + update_mem_ref_hash_table (end, 1); + + *iter = gsi_for_stmt (gsi_stmt (*iter)); } /* Instrument the call (to the builtin strlen function) pointed to by @@ -1783,7 +1792,7 @@ instrument_builtin_call (gimple_stmt_iterator *iter) } else if (src0_len || src1_len || dest_len) { - if (src0.start) + if (src0.start != NULL_TREE) instrument_mem_region_access (src0.start, src0_len, iter, loc, /*is_store=*/false); if (src1.start != NULL_TREE) diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c index cc98fdb..6cf6441 100644 --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c @@ -45,7 +45,7 @@ test1 () /* There are 2 calls to __builtin___asan_report_store1 and 2 calls to __builtin___asan_report_load1 to instrument the store to (subset of) the memory region of tab. */ - __builtin_memcpy (&tab[1], foo, sizeof (tab) - 1); + __builtin_memcpy (&tab[1], foo, 3); /* This should not generate a __builtin___asan_report_load1 because the reference to tab[1] has been already instrumented above. */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c new file mode 100644 index 0000000..b2e7284 --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c @@ -0,0 +1,13 @@ +/* { dg-options "-fdump-tree-asan0" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +void +foo (int *a, char *b, char *c) +{ + __builtin_memmove (c, b, a[c[0]]); +} + +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 3 "asan0" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 1 "asan0" } } */ +/* { dg-final { cleanup-tree-dump "asan0" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c new file mode 100644 index 0000000..ead3f58 --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c @@ -0,0 +1,13 @@ +/* { dg-options "-fdump-tree-asan0" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +void +foo (int *a, char *b, char *c) +{ + __builtin_memmove (c, b, a[b[0]]); +} + +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 2 "asan0" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2 "asan0" } } */ +/* { dg-final { cleanup-tree-dump "asan0" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c new file mode 100644 index 0000000..e4691bc --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c @@ -0,0 +1,14 @@ +/* { dg-options "-fdump-tree-asan0" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +void +foo (int *a, char *b, char *c) +{ + __builtin_memmove (c, b, a[c[0]]); + __builtin_memmove (c, b, a[b[0]]); +} + +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 5 "asan0" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2 "asan0" } } */ +/* { dg-final { cleanup-tree-dump "asan0" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c new file mode 100644 index 0000000..075e9cf --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c @@ -0,0 +1,23 @@ +/* { dg-options "-fdump-tree-asan0" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +char e[200]; + +struct S +{ + char a[100]; + char b[100]; +} s; + +int +foo (int *a, char *b, char *c) +{ + int d = __builtin_memcmp (s.a, e, 100); + d += __builtin_memcmp (s.a, e, 200); + return d; +} + +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load" 6 "asan0" } } */ +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_store" "asan0" } } */ +/* { dg-final { cleanup-tree-dump "asan" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c new file mode 100644 index 0000000..38ea7a2 --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c @@ -0,0 +1,14 @@ +/* { dg-options "-fdump-tree-asan0" } */ +/* { dg-do compile } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ + +char +foo (int *a, char *b, char *c) +{ + __builtin_memmove (c, b, a[b[0]]); + return c[0] + b[0]; +} + +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 3 "asan0" } } */ +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2 "asan0" } } */ +/* { dg-final { cleanup-tree-dump "asan0" } } */ diff --git a/gcc/testsuite/c-c++-common/asan/pr56330.c b/gcc/testsuite/c-c++-common/asan/pr56330.c new file mode 100644 index 0000000..25759f4 --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/pr56330.c @@ -0,0 +1,24 @@ +/* PR sanitizer/56330 */ +/* { dg-do compile } */ + +char e[200]; + +struct S +{ + char a[100]; + char b[100]; +} s; + +int +foo (void) +{ + int i = __builtin_memcmp (s.a, e, 100); + i += __builtin_memcmp (s.a, e, 200); + return i; +} + +void +bar (int *a, char *b, char *c) +{ + __builtin_memmove (c, b, a[b[0]]); +}