On Sat, Nov 11, 2017 at 12:44 AM, Marc Glisse <marc.gli...@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.gli...@inria.fr>
>>
>> gcc/c/
>> * c-fold.c (c_fully_fold_internal): Handle POINTER_DIFF_EXPR.
>> * c-typeck.c (pointer_diff): Use POINTER_DIFF_EXPR.
>>
>> gcc/c-family/
>> * c-pretty-print.c (pp_c_additive_expression,
>> c_pretty_printer::expression): Handle POINTER_DIFF_EXPR.
>>
>> 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.
>> (pointer_diff): Use POINTER_DIFF_EXPR.
>>
>> gcc/
>> * doc/generic.texi: Document POINTER_DIFF_EXPR, update
>> POINTER_PLUS_EXPR.
>> * cfgexpand.c (expand_debug_expr): Handle POINTER_DIFF_EXPR.
>> * expr.c (expand_expr_real_2): Likewise.
>> * fold-const.c (const_binop, const_binop,
>> fold_addr_of_array_ref_difference, fold_binary_loc): Likewise.
>> * match.pd (X-X, P+(Q-P), &D-P, (P+N)-P, P-(P+N), (P+M)-(P+N),
>> P-Q==0): New transformations for POINTER_DIFF_EXPR, based on
>> MINUS_EXPR transformations.
>> * optabs-tree.c (optab_for_tree_code): Handle POINTER_DIFF_EXPR.
>> * tree-cfg.c (verify_expr, verify_gimple_assign_binary): Likewise.
>> * tree-inline.c (estimate_operator_cost): Likewise.
>> * tree-pretty-print.c (dump_generic_node, op_code_prio,
>> op_symbol_code): Likewise.
>> * tree-vect-stmts.c (vectorizable_operation): Likewise.
>> * tree-vrp.c (extract_range_from_binary_expr): Likewise.
>> * varasm.c (initializer_constant_valid_p_1): Likewise.
>> * tree.def: New tree code POINTER_DIFF_EXPR.
>
>
> --
> Marc Glisse
