On Mon, Nov 23, 2015 at 11:10 AM, Ilya Enkovich <enkovich....@gmail.com> wrote:
> On 23 Nov 10:39, Richard Biener wrote:
>> 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.
>
> 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.
>> >>
