Re: [PATCH, PR68337] Don't fold memcpy/memmove we want to instrument

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

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.
> >>