| Message ID | 20151120130827.GH42296@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
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)); > +}
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. >
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. >>
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. > >>
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. >> >>
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. > >> >>
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. >> >> >>
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)); +}