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

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

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