* [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument @ 2015-11-19 16:31 Ilya Enkovich 2015-11-19 17:12 ` Bernd Schmidt 0 siblings, 1 reply; 13+ messages in thread From: Ilya Enkovich @ 2015-11-19 16:31 UTC (permalink / raw) To: gcc-patches Hi, 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. Thanks, Ilya -- gcc/ 2015-11-19 Ilya Enkovich <enkovich.gnu@gmail.com> * gimple-fold.c (gimple_fold_builtin_memory_op): Don't fold non-useless call if we are going to instrument it. gcc/testsuite/ 2015-11-19 Ilya Enkovich <enkovich.gnu@gmail.com> * gcc.target/i386/mpx/pr68337.c: New test. diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c index 1ab20d1..b3a1229 100644 --- a/gcc/gimple-fold.c +++ b/gcc/gimple-fold.c @@ -53,6 +53,7 @@ along with GCC; see the file COPYING3. If not see #include "gomp-constants.h" #include "optabs-query.h" #include "omp-low.h" +#include "ipa-chkp.h" /* Return true when DECL can be referenced from current unit. @@ -664,6 +665,13 @@ 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. */ + if (flag_check_pointer_bounds && chkp_instrumentable_p (cfun->decl)) + 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.c b/gcc/testsuite/gcc.target/i386/mpx/pr68337.c new file mode 100644 index 0000000..3f8d79d --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/pr68337.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; +} ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-19 16:31 [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument Ilya Enkovich @ 2015-11-19 17:12 ` Bernd Schmidt 2015-11-19 17:19 ` Richard Biener 0 siblings, 1 reply; 13+ messages in thread From: Bernd Schmidt @ 2015-11-19 17:12 UTC (permalink / raw) To: Ilya Enkovich, gcc-patches 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. Bernd ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-19 17:12 ` Bernd Schmidt @ 2015-11-19 17:19 ` Richard Biener 2015-11-20 13:08 ` Ilya Enkovich 0 siblings, 1 reply; 13+ messages in thread From: Richard Biener @ 2015-11-19 17:19 UTC (permalink / raw) To: Bernd Schmidt, Ilya Enkovich, gcc-patches On November 19, 2015 6:12:30 PM GMT+01:00, Bernd Schmidt <bschmidt@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. Richard. > >Bernd ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-19 17:19 ` Richard Biener @ 2015-11-20 13:08 ` Ilya Enkovich 2015-11-20 13:54 ` Richard Biener 0 siblings, 1 reply; 13+ messages in thread From: Ilya Enkovich @ 2015-11-20 13:08 UTC (permalink / raw) To: Richard Biener; +Cc: Bernd Schmidt, gcc-patches On 19 Nov 18:19, Richard Biener wrote: > On November 19, 2015 6:12:30 PM GMT+01:00, Bernd Schmidt <bschmidt@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.gnu@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.gnu@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) + /* 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)); +} ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-20 13:08 ` Ilya Enkovich @ 2015-11-20 13:54 ` Richard Biener 2015-11-20 14:30 ` Ilya Enkovich 0 siblings, 1 reply; 13+ messages in thread From: Richard Biener @ 2015-11-20 13:54 UTC (permalink / raw) To: Ilya Enkovich; +Cc: Bernd Schmidt, GCC Patches On Fri, Nov 20, 2015 at 2:08 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > On 19 Nov 18:19, Richard Biener wrote: >> On November 19, 2015 6:12:30 PM GMT+01:00, Bernd Schmidt <bschmidt@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.gnu@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.gnu@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)); > +} ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-20 13:54 ` Richard Biener @ 2015-11-20 14:30 ` Ilya Enkovich 2015-11-23 9:40 ` Richard Biener 0 siblings, 1 reply; 13+ messages in thread From: Ilya Enkovich @ 2015-11-20 14:30 UTC (permalink / raw) To: Richard Biener; +Cc: Bernd Schmidt, GCC Patches On 20 Nov 14:54, Richard Biener wrote: > On Fri, Nov 20, 2015 at 2:08 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > > On 19 Nov 18:19, Richard Biener wrote: > >> On November 19, 2015 6:12:30 PM GMT+01:00, Bernd Schmidt <bschmidt@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.gnu@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.gnu@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. This check is looking for cases when we have type information indicating no pointers are copied. In case of 'void *' we have to assume pointers are copied and inlining is undesired. Test pr68337-2.c checks pointer type allows to enable inlining. Looks like this check misses || !COMPLETE_TYPE_P(TREE_TYPE (TREE_TYPE (src)))? > > Note that you also disable memmove to memcpy simplification with this > early check. Doesn't matter for MPX which uses the same implementation for both cases. > > 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? It is handled by instrumentation pass. Compiler checks type of stored data to find pointer stores. Each pointer store is instrumented with bndstx call. MPX versions of memcpy, memmove etc. don't make any assumptions about type of copied data and just copy whole chunk of bounds metadata corresponding to copied block. Thanks, Ilya > > Richard. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-20 14:30 ` Ilya Enkovich @ 2015-11-23 9:40 ` Richard Biener 2015-11-23 10:12 ` Ilya Enkovich 0 siblings, 1 reply; 13+ messages in thread From: Richard Biener @ 2015-11-23 9:40 UTC (permalink / raw) To: Ilya Enkovich; +Cc: Bernd Schmidt, GCC Patches On Fri, Nov 20, 2015 at 3:30 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > On 20 Nov 14:54, Richard Biener wrote: >> On Fri, Nov 20, 2015 at 2:08 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> > On 19 Nov 18:19, Richard Biener wrote: >> >> On November 19, 2015 6:12:30 PM GMT+01:00, Bernd Schmidt <bschmidt@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.gnu@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.gnu@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. > > This check is looking for cases when we have type information indicating > no pointers are copied. In case of 'void *' we have to assume pointers > are copied and inlining is undesired. Test pr68337-2.c checks pointer > type allows to enable inlining. Looks like this check misses > || !COMPLETE_TYPE_P(TREE_TYPE (TREE_TYPE (src)))? As said there is no information in the pointer / pointed-to type in GIMPLE. >> >> Note that you also disable memmove to memcpy simplification with this >> early check. > > Doesn't matter for MPX which uses the same implementation for both cases. > >> >> 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? > > It is handled by instrumentation pass. Compiler checks type of stored data to > find pointer stores. Each pointer store is instrumented with bndstx call. How does it identify "pointer store"? With -fno-strict-aliasing you can store pointers using an integer type. You can also always store pointers using a character type like void foo (int *p, int **dest) { ((char *)*dest)[0] = (((char *)&p)[0]; ((char *)*dest)[1] = (((char *)&p)[1]; ((char *)*dest)[2] = (((char *)&p)[2]; ((char *)*dest)[3] = (((char *)&p)[3]; } > MPX versions of memcpy, memmove etc. don't make any assumptions about > type of copied data and just copy whole chunk of bounds metadata corresponding > to copied block. So it handles copying a pointer in two pieces with two memcpy calls correctly. Good. Richard. > Thanks, > Ilya > >> >> Richard. >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-23 9:40 ` Richard Biener @ 2015-11-23 10:12 ` Ilya Enkovich 2015-11-23 10:55 ` Richard Biener 0 siblings, 1 reply; 13+ messages in thread From: Ilya Enkovich @ 2015-11-23 10:12 UTC (permalink / raw) To: Richard Biener; +Cc: Bernd Schmidt, GCC Patches On 23 Nov 10:39, Richard Biener wrote: > On Fri, Nov 20, 2015 at 3:30 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > > On 20 Nov 14:54, Richard Biener wrote: > >> On Fri, Nov 20, 2015 at 2:08 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > >> > On 19 Nov 18:19, Richard Biener wrote: > >> >> On November 19, 2015 6:12:30 PM GMT+01:00, Bernd Schmidt <bschmidt@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.gnu@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.gnu@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. > > > > This check is looking for cases when we have type information indicating > > no pointers are copied. In case of 'void *' we have to assume pointers > > are copied and inlining is undesired. Test pr68337-2.c checks pointer > > type allows to enable inlining. Looks like this check misses > > || !COMPLETE_TYPE_P(TREE_TYPE (TREE_TYPE (src)))? > > As said there is no information in the pointer / pointed-to type in GIMPLE. What does it mean? We do have TREE_TYPE for used pointer and nested TREE_TYPE holding pointed-to type. Is it some random invalid type? > > >> > >> Note that you also disable memmove to memcpy simplification with this > >> early check. > > > > Doesn't matter for MPX which uses the same implementation for both cases. > > > >> > >> 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? > > > > It is handled by instrumentation pass. Compiler checks type of stored data to > > find pointer stores. Each pointer store is instrumented with bndstx call. > > How does it identify "pointer store"? With -fno-strict-aliasing you can store > pointers using an integer type. You can also always store pointers using > a character type like > > void foo (int *p, int **dest) > { > ((char *)*dest)[0] = (((char *)&p)[0]; > ((char *)*dest)[1] = (((char *)&p)[1]; > ((char *)*dest)[2] = (((char *)&p)[2]; > ((char *)*dest)[3] = (((char *)&p)[3]; > } Pointer store is identified using type information. When pointer is casted to a non-pointer type its bounds are lost. Ilya > > > MPX versions of memcpy, memmove etc. don't make any assumptions about > > type of copied data and just copy whole chunk of bounds metadata corresponding > > to copied block. > > So it handles copying a pointer in two pieces with two memcpy calls > correctly. Good. > > Richard. > > > Thanks, > > Ilya > > > >> > >> Richard. > >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-23 10:12 ` Ilya Enkovich @ 2015-11-23 10:55 ` Richard Biener 2015-11-23 11:41 ` Ilya Enkovich 0 siblings, 1 reply; 13+ messages in thread From: Richard Biener @ 2015-11-23 10:55 UTC (permalink / raw) To: Ilya Enkovich; +Cc: Bernd Schmidt, GCC Patches On Mon, Nov 23, 2015 at 11:10 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > On 23 Nov 10:39, Richard Biener wrote: >> On Fri, Nov 20, 2015 at 3:30 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> > On 20 Nov 14:54, Richard Biener wrote: >> >> On Fri, Nov 20, 2015 at 2:08 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> >> > On 19 Nov 18:19, Richard Biener wrote: >> >> >> On November 19, 2015 6:12:30 PM GMT+01:00, Bernd Schmidt <bschmidt@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.gnu@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.gnu@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. >> > >> > This check is looking for cases when we have type information indicating >> > no pointers are copied. In case of 'void *' we have to assume pointers >> > are copied and inlining is undesired. Test pr68337-2.c checks pointer >> > type allows to enable inlining. Looks like this check misses >> > || !COMPLETE_TYPE_P(TREE_TYPE (TREE_TYPE (src)))? >> >> As said there is no information in the pointer / pointed-to type in GIMPLE. > > What does it mean? We do have TREE_TYPE for used pointer and nested TREE_TYPE > holding pointed-to type. Is it some random invalid type? Yes, it can be a "random" type. Like for void foo (float *f) { memcpy ((void *)f, ...); } int main() { int **a[10]; foo (a); } which tries to copy to an array of int * but the GIMPLE IL for foo will call memcpy with a float * typed argument. >> >> >> >> >> Note that you also disable memmove to memcpy simplification with this >> >> early check. >> > >> > Doesn't matter for MPX which uses the same implementation for both cases. >> > >> >> >> >> 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? >> > >> > It is handled by instrumentation pass. Compiler checks type of stored data to >> > find pointer stores. Each pointer store is instrumented with bndstx call. >> >> How does it identify "pointer store"? With -fno-strict-aliasing you can store >> pointers using an integer type. You can also always store pointers using >> a character type like >> >> void foo (int *p, int **dest) >> { >> ((char *)*dest)[0] = (((char *)&p)[0]; >> ((char *)*dest)[1] = (((char *)&p)[1]; >> ((char *)*dest)[2] = (((char *)&p)[2]; >> ((char *)*dest)[3] = (((char *)&p)[3]; >> } > > Pointer store is identified using type information. When pointer is casted to > a non-pointer type its bounds are lost. > > Ilya > >> >> > MPX versions of memcpy, memmove etc. don't make any assumptions about >> > type of copied data and just copy whole chunk of bounds metadata corresponding >> > to copied block. >> >> So it handles copying a pointer in two pieces with two memcpy calls >> correctly. Good. >> >> Richard. >> >> > Thanks, >> > Ilya >> > >> >> >> >> Richard. >> >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-23 10:55 ` Richard Biener @ 2015-11-23 11:41 ` Ilya Enkovich 2015-11-23 13:31 ` Richard Biener 0 siblings, 1 reply; 13+ messages in thread From: Ilya Enkovich @ 2015-11-23 11:41 UTC (permalink / raw) To: Richard Biener; +Cc: Bernd Schmidt, GCC Patches On 23 Nov 11:44, Richard Biener wrote: > On Mon, Nov 23, 2015 at 11:10 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > > On 23 Nov 10:39, Richard Biener wrote: > >> On Fri, Nov 20, 2015 at 3:30 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > >> > On 20 Nov 14:54, Richard Biener wrote: > >> >> > >> >> 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. > >> > > >> > This check is looking for cases when we have type information indicating > >> > no pointers are copied. In case of 'void *' we have to assume pointers > >> > are copied and inlining is undesired. Test pr68337-2.c checks pointer > >> > type allows to enable inlining. Looks like this check misses > >> > || !COMPLETE_TYPE_P(TREE_TYPE (TREE_TYPE (src)))? > >> > >> As said there is no information in the pointer / pointed-to type in GIMPLE. > > > > What does it mean? We do have TREE_TYPE for used pointer and nested TREE_TYPE > > holding pointed-to type. Is it some random invalid type? > > Yes, it can be a "random" type. Like for > > void foo (float *f) > { > memcpy ((void *)f, ...); > } > int main() > { > int **a[10]; > foo (a); > } > > which tries to copy to an array of int * but the GIMPLE IL for foo > will call memcpy with a float * typed argument. I see. But it should still be OK to check type in case of strict aliasing, right? Thanks, Ilya > > >> > >> >> > >> >> Note that you also disable memmove to memcpy simplification with this > >> >> early check. > >> > > >> > Doesn't matter for MPX which uses the same implementation for both cases. > >> > > >> >> > >> >> 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? > >> > > >> > It is handled by instrumentation pass. Compiler checks type of stored data to > >> > find pointer stores. Each pointer store is instrumented with bndstx call. > >> > >> How does it identify "pointer store"? With -fno-strict-aliasing you can store > >> pointers using an integer type. You can also always store pointers using > >> a character type like > >> > >> void foo (int *p, int **dest) > >> { > >> ((char *)*dest)[0] = (((char *)&p)[0]; > >> ((char *)*dest)[1] = (((char *)&p)[1]; > >> ((char *)*dest)[2] = (((char *)&p)[2]; > >> ((char *)*dest)[3] = (((char *)&p)[3]; > >> } > > > > Pointer store is identified using type information. When pointer is casted to > > a non-pointer type its bounds are lost. > > > > Ilya > > > >> > >> > MPX versions of memcpy, memmove etc. don't make any assumptions about > >> > type of copied data and just copy whole chunk of bounds metadata corresponding > >> > to copied block. > >> > >> So it handles copying a pointer in two pieces with two memcpy calls > >> correctly. Good. > >> > >> Richard. > >> > >> > Thanks, > >> > Ilya > >> > > >> >> > >> >> Richard. > >> >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-23 11:41 ` Ilya Enkovich @ 2015-11-23 13:31 ` Richard Biener 2015-11-23 13:47 ` Ilya Enkovich 0 siblings, 1 reply; 13+ messages in thread From: Richard Biener @ 2015-11-23 13:31 UTC (permalink / raw) To: Ilya Enkovich; +Cc: Bernd Schmidt, GCC Patches On Mon, Nov 23, 2015 at 12:33 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > On 23 Nov 11:44, Richard Biener wrote: >> On Mon, Nov 23, 2015 at 11:10 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> > On 23 Nov 10:39, Richard Biener wrote: >> >> On Fri, Nov 20, 2015 at 3:30 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> >> > On 20 Nov 14:54, Richard Biener wrote: >> >> >> >> >> >> 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. >> >> > >> >> > This check is looking for cases when we have type information indicating >> >> > no pointers are copied. In case of 'void *' we have to assume pointers >> >> > are copied and inlining is undesired. Test pr68337-2.c checks pointer >> >> > type allows to enable inlining. Looks like this check misses >> >> > || !COMPLETE_TYPE_P(TREE_TYPE (TREE_TYPE (src)))? >> >> >> >> As said there is no information in the pointer / pointed-to type in GIMPLE. >> > >> > What does it mean? We do have TREE_TYPE for used pointer and nested TREE_TYPE >> > holding pointed-to type. Is it some random invalid type? >> >> Yes, it can be a "random" type. Like for >> >> void foo (float *f) >> { >> memcpy ((void *)f, ...); >> } >> int main() >> { >> int **a[10]; >> foo (a); >> } >> >> which tries to copy to an array of int * but the GIMPLE IL for foo >> will call memcpy with a float * typed argument. > > I see. But it should still be OK to check type in case of strict aliasing, right? No, memcpy is always "no-strict-aliasing" > Thanks, > Ilya > >> >> >> >> >> >> >> >> >> Note that you also disable memmove to memcpy simplification with this >> >> >> early check. >> >> > >> >> > Doesn't matter for MPX which uses the same implementation for both cases. >> >> > >> >> >> >> >> >> 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? >> >> > >> >> > It is handled by instrumentation pass. Compiler checks type of stored data to >> >> > find pointer stores. Each pointer store is instrumented with bndstx call. >> >> >> >> How does it identify "pointer store"? With -fno-strict-aliasing you can store >> >> pointers using an integer type. You can also always store pointers using >> >> a character type like >> >> >> >> void foo (int *p, int **dest) >> >> { >> >> ((char *)*dest)[0] = (((char *)&p)[0]; >> >> ((char *)*dest)[1] = (((char *)&p)[1]; >> >> ((char *)*dest)[2] = (((char *)&p)[2]; >> >> ((char *)*dest)[3] = (((char *)&p)[3]; >> >> } >> > >> > Pointer store is identified using type information. When pointer is casted to >> > a non-pointer type its bounds are lost. >> > >> > Ilya >> > >> >> >> >> > MPX versions of memcpy, memmove etc. don't make any assumptions about >> >> > type of copied data and just copy whole chunk of bounds metadata corresponding >> >> > to copied block. >> >> >> >> So it handles copying a pointer in two pieces with two memcpy calls >> >> correctly. Good. >> >> >> >> Richard. >> >> >> >> > Thanks, >> >> > Ilya >> >> > >> >> >> >> >> >> Richard. >> >> >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-23 13:31 ` Richard Biener @ 2015-11-23 13:47 ` Ilya Enkovich 2015-11-23 15:35 ` Richard Biener 0 siblings, 1 reply; 13+ messages in thread From: Ilya Enkovich @ 2015-11-23 13:47 UTC (permalink / raw) To: Richard Biener; +Cc: Bernd Schmidt, GCC Patches On 23 Nov 14:29, Richard Biener wrote: > On Mon, Nov 23, 2015 at 12:33 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > > > > I see. But it should still be OK to check type in case of strict aliasing, right? > > No, memcpy is always "no-strict-aliasing" > Thanks a lot for help! Here is a variant with a size check only as you originally suggested. Is it OK for trunk and gcc-5-branch if no regressions? Thanks, Ilya -- gcc/ 2015-11-23 Ilya Enkovich <enkovich.gnu@gmail.com> * gimple-fold.c: Include ipa-chkp.h. (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-23 Ilya Enkovich <enkovich.gnu@gmail.com> * gcc.target/i386/mpx/pr68337-1.c: New test. * gcc.target/i386/mpx/pr68337-2.c: New test. diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c index 1ab20d1..6ff5e26 100644 --- a/gcc/gimple-fold.c +++ b/gcc/gimple-fold.c @@ -53,6 +53,7 @@ along with GCC; see the file COPYING3. If not see #include "gomp-constants.h" #include "optabs-query.h" #include "omp-low.h" +#include "ipa-chkp.h" /* Return true when DECL can be referenced from current unit. @@ -664,6 +665,18 @@ 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. */ + 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)) + 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..8845cca --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/pr68337-2.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ +/* { dg-final { scan-assembler-not "memcpy" } } */ + +void +test (void *dst, void *src) +{ + __builtin_memcpy (dst, src, sizeof (char *) / 2); +} ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument 2015-11-23 13:47 ` Ilya Enkovich @ 2015-11-23 15:35 ` Richard Biener 0 siblings, 0 replies; 13+ messages in thread From: Richard Biener @ 2015-11-23 15:35 UTC (permalink / raw) To: Ilya Enkovich; +Cc: Bernd Schmidt, GCC Patches On Mon, Nov 23, 2015 at 2:45 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > On 23 Nov 14:29, Richard Biener wrote: >> On Mon, Nov 23, 2015 at 12:33 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> > >> > I see. But it should still be OK to check type in case of strict aliasing, right? >> >> No, memcpy is always "no-strict-aliasing" >> > > Thanks a lot for help! Here is a variant with a size check only as > you originally suggested. Is it OK for trunk and gcc-5-branch if > no regressions? Ok. Thanks, Richard. > Thanks, > Ilya > -- > gcc/ > > 2015-11-23 Ilya Enkovich <enkovich.gnu@gmail.com> > > * gimple-fold.c: Include ipa-chkp.h. > (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-23 Ilya Enkovich <enkovich.gnu@gmail.com> > > * gcc.target/i386/mpx/pr68337-1.c: New test. > * gcc.target/i386/mpx/pr68337-2.c: New test. > > > diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c > index 1ab20d1..6ff5e26 100644 > --- a/gcc/gimple-fold.c > +++ b/gcc/gimple-fold.c > @@ -53,6 +53,7 @@ along with GCC; see the file COPYING3. If not see > #include "gomp-constants.h" > #include "optabs-query.h" > #include "omp-low.h" > +#include "ipa-chkp.h" > > > /* Return true when DECL can be referenced from current unit. > @@ -664,6 +665,18 @@ 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. */ > + 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)) > + 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..8845cca > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/mpx/pr68337-2.c > @@ -0,0 +1,9 @@ > +/* { dg-do compile } */ > +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ > +/* { dg-final { scan-assembler-not "memcpy" } } */ > + > +void > +test (void *dst, void *src) > +{ > + __builtin_memcpy (dst, src, sizeof (char *) / 2); > +} ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-11-23 15:33 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-19 16:31 [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument Ilya Enkovich 2015-11-19 17:12 ` Bernd Schmidt 2015-11-19 17:19 ` Richard Biener 2015-11-20 13:08 ` Ilya Enkovich 2015-11-20 13:54 ` Richard Biener 2015-11-20 14:30 ` Ilya Enkovich 2015-11-23 9:40 ` Richard Biener 2015-11-23 10:12 ` Ilya Enkovich 2015-11-23 10:55 ` Richard Biener 2015-11-23 11:41 ` Ilya Enkovich 2015-11-23 13:31 ` Richard Biener 2015-11-23 13:47 ` Ilya Enkovich 2015-11-23 15:35 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).