Re: [RFTesting] New POINTER_DIFF_EXPR

[Jason Merrill <jason@redhat.com>]

On Fri, Nov 17, 2017 at 7:56 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Sat, Nov 11, 2017 at 12:44 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> Adding some random cc: to people who might be affected. Hopefully I am not
>> breaking any of your stuff...
>>
>> Ulrich Weigand (address space)
>> Ilya Enkovich (pointer bound check)
>> DJ Delorie (target with 24-bit partial mode pointer)
>>
>> If you want to give it a try, or just take a quick look to check that you
>> are obviously not affected, that would be nice, but don't feel forced.
>>
>> Here is an updated version of the patch, with just a few more
>> transformations in match.pd, to match some MINUS_EXPR optimizations I missed
>> the first time: -(A-B), X-Z<Y-Z, (X-Z)-(Y-Z), Z-X<Z-Y, (Z-X)-(Z-Y),
>> (A-B)+(C-A)
>>
>> The exact undefined-behavior status should probably be clarified more.
>> First, I'd like to say that POINTER_DIFF_EXPR may only take 2 pointers into
>> the same "object" (like in C/C++), so they differ by at most half the size
>> of the address space, and a-b is not allowed to be the minimum negative
>> value (so I can safely use b-a), and if we compute a-b and b-c, then a and c
>> are in the same object so I can safely compute a-c, etc. Second, we probably
>> need modes without undefined behavior, wrapping with either -fwrapv or a new
>> -fwrapp, or sanitized. For the sanitized version, we could keep using
>> POINTER_DIFF_EXPR and check TYPE_OVERFLOW_SANITIZED on the pointer type as
>> we currently do for integers. For wrapping, either we say that
>> POINTER_DIFF_EXPR has wrapping semantics when that option is in effect, or
>> we do not use POINTER_DIFF_EXPR and instead cast to unsigned before applying
>> a MINUS_EXPR (my preference).
>
> CCing C/C++ FE folks as well.
>
> +      /* It is better if the caller provides the return type.  */
> +      if (code == POINTER_DIFF_EXPR)
> +       {
> +         offset_int res = wi::sub (wi::to_offset (arg1),
> +                                   wi::to_offset (arg2));
> +         return force_fit_type (signed_type_for (TREE_TYPE (arg1)), res, 1,
> +                                TREE_OVERFLOW (arg1) | TREE_OVERFLOW (arg2));
> +       }
> +
>
> there's an overload that provides the type... (we should probably go
> over all callers
> and use that).  There is already a folding that is only done when the type is
> available.  So I'd simply remove the above from the non-type overload handling.
> What breaks then?
>
> With the patch it's of course somewhat ugly in that we need to deal with both
> MINUS_EXPR on pointers and POINTER_DIFF_EXPR - at least that's what
>
> +    case POINTER_DIFF_EXPR:
>      case MINUS_EXPR:
> +      /* Fold &a[i] - &a[j] to i-j.  */
> +      if (TREE_CODE (arg0) == ADDR_EXPR
> +         && TREE_CODE (TREE_OPERAND (arg0, 0)) == ARRAY_REF
> +         && TREE_CODE (arg1) == ADDR_EXPR
> +         && TREE_CODE (TREE_OPERAND (arg1, 0)) == ARRAY_REF)
> +        {
> +         tree tem = fold_addr_of_array_ref_difference (loc, type,
> +                                                       TREE_OPERAND (arg0, 0),
> +                                                       TREE_OPERAND (arg1, 0),
> +                                                       code
> +                                                       == POINTER_DIFF_EXPR);
>
> suggests.  But is that really so?  The FEs today should never build
> a MINUS_EXPR with pointer operands and your patch also doesn't.
> I suppose we only arrive above because STRIP_NOPS strips the
> pointer to integer conversion?
>
> +    case POINTER_DIFF_EXPR:
> +      if (!POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (t, 0)))
> +         || !POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (t, 1))))
> +       {
> +         error ("invalid operand to pointer diff, operand is not a pointer");
> +         return t;
> +       }
> +      CHECK_OP (0, "invalid operand to pointer diff");
> +      CHECK_OP (1, "invalid operand to pointer diff");
> +      break;
>
> can you add
>   || TREE_CODE (TREE_TYPE (t)) != INTEGER_TYPE
>   || TYPE_UNSIGNED (TREE_TYPE (t))
>
> ?  Note that if the precision of the pointer type arguments are not equal to the
> precision of the return value then foldings like
>
>    (simplify
> +   (plus:c (pointer_diff @0 @1) (pointer_diff @2 @0))
> +   (if (TYPE_OVERFLOW_UNDEFINED (type)
> +       && !TYPE_OVERFLOW_SANITIZED (TREE_TYPE (@0)))
> +    (pointer_diff @2 @1)))
>
> might not be correct as we drop a possible truncation here.  Shall we
> require (and document) that the result of the pointer difference in
> a POINTER_DIFF_EXPR has to be representable in the type of the
> expression, otherwise
> the behavior is undefined?  Shall we allow an unsigned return type for
> differences known to be representable?  Probably not...  Does the behavior
> depend on TYPE_OVERFLOW_UNDEFINED?  It would be nice to amend
> the generic.texi docs somewhat here.
>
> Ah, the gimple checking adds some more bits here that clarify things:
>
> +    case POINTER_DIFF_EXPR:
> +      {
> +       if (!POINTER_TYPE_P (rhs1_type)
> +           || !POINTER_TYPE_P (rhs2_type)
> +           || !types_compatible_p (rhs1_type, rhs2_type)
> +           || TREE_CODE (lhs_type) != INTEGER_TYPE
> +           || TYPE_UNSIGNED (lhs_type)
> +           || TYPE_PRECISION (lhs_type) != TYPE_PRECISION (rhs1_type))
> +         {
> +           error ("type mismatch in pointer diff expression");
> +           debug_generic_stmt (lhs_type);
> +           debug_generic_stmt (rhs1_type);
> +           debug_generic_stmt (rhs2_type);
> +           return true;
> +         }
>
>
> I think the patch is ok for trunk, the FE bits should get approval
> from their maintainers.
> Joseph may have an idea about the address-space issue.
>
> With this in place we should be able to fix some of the issues Jakub ran into
> with pointer sanitizing?
>
> Also with this in place it would be nice to do the corresponding adjustment to
> POINTER_PLUS_EXPR, that is, require a _signed_ offset type that matches
> the precision of the other argument.
>
> Richard.
>
>>
>>
>> On Sat, 28 Oct 2017, Marc Glisse wrote:
>>
>>> Hello,
>>>
>>> first, if you are doing anything unusual with pointers (address spaces,
>>> pointer/sizetype with weird sizes, instrumentation, etc), it would be great
>>> if you could give this patch a try. It was bootstrapped and regtested on
>>> powerpc64le-unknown-linux-gnu (gcc112), and a slightly older version on
>>> x86_64-pc-linux-gnu (skylake laptop). I also built bare cross-compilers (no
>>> sysroot or anything) for avr, m32c, alpha64-vms, s390-linux, and visium to
>>> check that on trivial examples it behaves as expected (by the way, m32c
>>> seems broken for unrelated reasons at the moment), but I wouldn't count that
>>> as complete testing.
>>>
>>> This was previously discussed in the thread "Fix pointer diff (was:
>>> -fsanitize=pointer-overflow support (PR sanitizer/80998))" (
>>> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg02128.html for the latest
>>> message).
>>>
>>> Front-ends other than C/C++ can be changed later (I took a quick look at
>>> fortran and ada, but they are way too unfamiliar to me), I did not remove
>>> any handling for the other representations.
>>>
>>> 2017-10-28  Marc Glisse  <marc.glisse@inria.fr>
>>>
>>> gcc/cp/
>>>         * constexpr.c (cxx_eval_constant_expression,
>>>         potential_constant_expression_1): Handle POINTER_DIFF_EXPR.
>>>         * cp-gimplify.c (cp_fold): Likewise.
>>>         * error.c (dump_expr): Likewise.
>>>         * typeck.c (cp_build_binary_op): Likewise.

It's not clear to me that cp_build_binary_op needs to handle
POINTER_DIFF_EXPR, it should get MINUS_EXPR and produce
POINTER_DIFF_EXPR.

>>>         * tree.def: New tree code POINTER_DIFF_EXPR.

The comment might mention that the value is the same for all pointer
types, not divided by the size of the pointed-to type.

Jason