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

[Richard Biener <richard.guenther@gmail.com>]

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