On Fri, Nov 20, 2015 at 2:08 PM, Ilya Enkovich <enkovich....@gmail.com> wrote: > On 19 Nov 18:19, Richard Biener wrote: >> On November 19, 2015 6:12:30 PM GMT+01:00, Bernd Schmidt >> <bschm...@redhat.com> 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 <enkovich....@gmail.com> > > * 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 <enkovich....@gmail.com> > > * 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)
|| tree_to_uhwi (len) >= POINTER_SIZE_UNITS > + /* 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))))) I don't think you can in any way rely on the pointer type of the src argument as all pointer conversions are useless and memcpy and friends take void * anyway. Note that you also disable memmove to memcpy simplification with this early check. Where is pointer transfer handled for MPX? I suppose it's not done transparently for all memory move instructions but explicitely by instrumented block copy routines in libmpx? In which case how does that identify pointers vs. non-pointers? Richard. > + 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)); > +}