From patchwork Fri Nov 20 13:08:27 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Enkovich X-Patchwork-Id: 546915 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 5AEF71402B4 for ; Sat, 21 Nov 2015 00:09:06 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=ytgUTWF1; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=Mi9b2zdVeeK7WRD+T EwNerIwIszl4ipc/Z/FJgw+RTW3uUoFlkTrfddQjzb54dUDcY+63b+SK6IFTLH1Z b9h29ACFvxnccuX9tah2+Qj8z3MUOlQZAzMWIMgo8yF2Yr6cuGe9fMfyolkTZLQm foUXkUttioQovPQwRLNDeF1zCE= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=UVYpDa/nyTmBE6OMvDYs41v NQQo=; b=ytgUTWF1xFT+/jTfdhWiVsB+1HQXAgyPG5eaiwEG+KbyTQwwrr2/KB+ jnkcY6rY2Q2mYjbd3jjtdLBQPy2hucxH6V0G8xWMniXeLkDsYw2G1Hj2VknNS5/7 IwGKUCq22VjZdOHH4THQXj1KTA/CDBi0CMsvMwvie4TYaikByd84= Received: (qmail 43387 invoked by alias); 20 Nov 2015 13:08:58 -0000 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 Received: (qmail 43356 invoked by uid 89); 20 Nov 2015 13:08:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ig0-f173.google.com Received: from mail-ig0-f173.google.com (HELO mail-ig0-f173.google.com) (209.85.213.173) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 20 Nov 2015 13:08:56 +0000 Received: by igl9 with SMTP id 9so10257881igl.0 for ; Fri, 20 Nov 2015 05:08:54 -0800 (PST) X-Received: by 10.50.66.69 with SMTP id d5mr1549565igt.30.1448024934444; Fri, 20 Nov 2015 05:08:54 -0800 (PST) Received: from msticlxl57.ims.intel.com ([134.134.139.76]) by smtp.gmail.com with ESMTPSA id u12sm1002282igr.22.2015.11.20.05.08.52 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 20 Nov 2015 05:08:54 -0800 (PST) Date: Fri, 20 Nov 2015 16:08:27 +0300 From: Ilya Enkovich To: Richard Biener Cc: Bernd Schmidt , gcc-patches@gcc.gnu.org Subject: Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument Message-ID: <20151120130827.GH42296@msticlxl57.ims.intel.com> References: <20151119163110.GG42296@msticlxl57.ims.intel.com> <564E02FE.5020503@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes On 19 Nov 18:19, Richard Biener wrote: > On November 19, 2015 6:12:30 PM GMT+01:00, Bernd Schmidt wrote: > >On 11/19/2015 05:31 PM, Ilya Enkovich wrote: > >> Currently we fold all memcpy/memmove calls with a known data size. > >> It causes two problems when used with Pointer Bounds Checker. > >> The first problem is that we may copy pointers as integer data > >> and thus loose bounds. The second problem is that if we inline > >> memcpy, we also have to inline bounds copy and this may result > >> in a huge amount of code and significant compilation time growth. > >> This patch disables folding for functions we want to instrument. > >> > >> Does it look reasonable for trunk and GCC5 branch? Bootstrapped > >> and regtested on x86_64-unknown-linux-gnu. > > > >Can't see anything wrong with it. Ok. > > But for small sizes this can have a huge impact on optimization. Which is why we have the code in the first place. I'd make the check less broad, for example inlining copies of size less than a pointer shouldn't be affected. Right. We also may inline in case we know no pointers are copied. Below is a version with extended condition and a couple more tests. Bootstrapped and regtested on x86_64-unknown-linux-gnu. Does it OK for trunk and gcc-5-branch? > > Richard. > > > > >Bernd > > Thanks, Ilya --- gcc/ 2015-11-20 Ilya Enkovich * gimple-fold.c (gimple_fold_builtin_memory_op): Don't fold call if we are going to instrument it and it may copy pointers. gcc/testsuite/ 2015-11-20 Ilya Enkovich * gcc.target/i386/mpx/pr68337-1.c: New test. * gcc.target/i386/mpx/pr68337-2.c: New test. * gcc.target/i386/mpx/pr68337-3.c: New test. diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c index 1ab20d1..dd9f80b 100644 --- a/gcc/gimple-fold.c +++ b/gcc/gimple-fold.c @@ -53,6 +53,8 @@ along with GCC; see the file COPYING3. If not see #include "gomp-constants.h" #include "optabs-query.h" #include "omp-low.h" +#include "tree-chkp.h" +#include "ipa-chkp.h" /* Return true when DECL can be referenced from current unit. @@ -664,6 +666,23 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi, unsigned int src_align, dest_align; tree off0; + /* Inlining of memcpy/memmove may cause bounds lost (if we copy + pointers as wide integer) and also may result in huge function + size because of inlined bounds copy. Thus don't inline for + functions we want to instrument in case pointers are copied. */ + if (flag_check_pointer_bounds + && chkp_instrumentable_p (cfun->decl) + /* Even if data may contain pointers we can inline if copy + less than a pointer size. */ + && (!tree_fits_uhwi_p (len) + || compare_tree_int (len, POINTER_SIZE_UNITS) >= 0) + /* Check data type for pointers. */ + && (!TREE_TYPE (src) + || !TREE_TYPE (TREE_TYPE (src)) + || VOID_TYPE_P (TREE_TYPE (TREE_TYPE (src))) + || chkp_type_has_pointer (TREE_TYPE (TREE_TYPE (src))))) + return false; + /* Build accesses at offset zero with a ref-all character type. */ off0 = build_int_cst (build_pointer_type_for_mode (char_type_node, ptr_mode, true), 0); diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr68337-1.c b/gcc/testsuite/gcc.target/i386/mpx/pr68337-1.c new file mode 100644 index 0000000..3f8d79d --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/pr68337-1.c @@ -0,0 +1,32 @@ +/* { dg-do run } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + +#include "mpx-check.h" + +#define N 2 + +extern void abort (); + +static int +mpx_test (int argc, const char **argv) +{ + char ** src = (char **)malloc (sizeof (char *) * N); + char ** dst = (char **)malloc (sizeof (char *) * N); + int i; + + for (i = 0; i < N; i++) + src[i] = __bnd_set_ptr_bounds (argv[0] + i, i + 1); + + __builtin_memcpy(dst, src, sizeof (char *) * N); + + for (i = 0; i < N; i++) + { + char *p = dst[i]; + if (p != argv[0] + i + || __bnd_get_ptr_lbound (p) != p + || __bnd_get_ptr_ubound (p) != p + i) + abort (); + } + + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr68337-2.c b/gcc/testsuite/gcc.target/i386/mpx/pr68337-2.c new file mode 100644 index 0000000..16736b4 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/pr68337-2.c @@ -0,0 +1,27 @@ +/* { dg-do compile } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ +/* { dg-final { scan-assembler-not "memcpy" } } */ + +void +test1 (char *dst, char *src) +{ + __builtin_memcpy (dst, src, sizeof (char *) * 2); +} + +void +test2 (void *dst, void *src) +{ + __builtin_memcpy (dst, src, sizeof (char *) / 2); +} + +struct s +{ + int a; + int b; +}; + +void +test3 (struct s *dst, struct s *src) +{ + __builtin_memcpy (dst, src, sizeof (struct s)); +} diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr68337-3.c b/gcc/testsuite/gcc.target/i386/mpx/pr68337-3.c new file mode 100644 index 0000000..095425a --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/pr68337-3.c @@ -0,0 +1,27 @@ +/* { dg-do compile } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ +/* { dg-final { scan-assembler-times "bnd\[^\n\]*__mpx_wrapper_memcpy" 3 } } */ + +void +test1 (char **dst, char **src) +{ + __builtin_memcpy (dst, src, sizeof (char *) * 2); +} + +void +test2 (void *dst, void *src) +{ + __builtin_memcpy (dst, src, sizeof (char *)); +} + +struct s +{ + int a; + int *b; +}; + +void +test3 (struct s *dst, struct s *src) +{ + __builtin_memcpy (dst, src, sizeof (struct s)); +}