On Fri, Nov 20, 2015 at 3:30 PM, Ilya Enkovich <enkovich....@gmail.com> wrote:
> On 20 Nov 14:54, Richard Biener wrote:
>> On Fri, Nov 20, 2015 at 2:08 PM, Ilya Enkovich <enkovich....@gmail.com>
>> wrote:
>> > On 19 Nov 18:19, Richard Biener wrote:
>> >> On November 19, 2015 6:12:30 PM GMT+01:00, Bernd Schmidt
>> >> <bschm...@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....@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....@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.
>>
