Re: [PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367)
On 12/6/18 1:32 PM, Jakub Jelinek wrote: >> For -fno-delete-null-pointer-checks ISTM >> we should indicate "we don't know anything about the result" of such an >> operation. > > There are cases where we still know something. The largest valid object > that can be supported is half of the address space, so without pointer > wrapping, positive additions to the pointer shouldn't wrap around and yield > NULL, negative ones can. With -fwrapv-pointers anything can happen, sure, > the only case handled in that case is &[ptr + 0] if ptr is ~[0, 0] then > &[ptr + 0] is also ~[0, 0]. Yea. I just didn't figure those were worth worrying about. jeff
Re: [PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367)
On Thu, Dec 06, 2018 at 01:08:34PM -0700, Jeff Law wrote: > > I hope we can still say that pointer wrapping even with > > -fno-delete-null-pointer-checks is UB, so this patch differentiates between > > positive offsets (in ssizetype), negative offsets (in ssizetype) and zero > > offsets and handles both the same for both ptr p+ offset and _REF[ptr, > > offset] > > If offset is 0 and ptr is ~[0, 0], then the result is ~[0, 0] as before. > > If offset is positive in ssizetype, then even for VARYING ptr the result is > > ~[0, 0] pointer. If the offset is (or maybe could be) negative in > > ssizetype, then for -fno-delete-null-pointer-checks the result is VARYING, > > as we could go from a non-NULL pointer back to NULL on those targets; for > > -fdelete-null-pointer-checks we do what we've done before, i.e. ~[0, 0]. > I'm not sure why we'd treat subtraction and addition any differently, > but maybe I'm missing something subtle (or not so subtle). > > ISTM that ~[0,0] +- still results in ~[0,0] for > -fdelete-null-pointer-checks. Yes, the patch preserves that (unless -fwrapv-pointers). Additionally, it does VARYING += ~[0,0] in that mode to ~[0,0]. > For -fno-delete-null-pointer-checks ISTM > we should indicate "we don't know anything about the result" of such an > operation. There are cases where we still know something. The largest valid object that can be supported is half of the address space, so without pointer wrapping, positive additions to the pointer shouldn't wrap around and yield NULL, negative ones can. With -fwrapv-pointers anything can happen, sure, the only case handled in that case is &[ptr + 0] if ptr is ~[0, 0] then &[ptr + 0] is also ~[0, 0]. Jakub
Re: [PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367)
On 12/5/18 11:45 PM, Jakub Jelinek wrote: > Hi! > > If we consider -fno-delete-null-pointer-checks as a way to support e.g. AVR > and other targets which can validly place objects at NULL rather than a way > to workaround UBs in code, I believe the following testcase must pass if > there is e.g. Well, the intent was to be able to turn off the assumption that *0 would cause a fault and halt the program. That assumption allows us to use an earlier pointer dereference to infer it currently has a non-NULL value and eliminate subsequent tests against NULL. It was never really meant to be a general escape hatch to allow other forms of undefined behavior. Though the name is particularly bad since it implies we never delete any null pointer checks. > > I hope we can still say that pointer wrapping even with > -fno-delete-null-pointer-checks is UB, so this patch differentiates between > positive offsets (in ssizetype), negative offsets (in ssizetype) and zero > offsets and handles both the same for both ptr p+ offset and _REF[ptr, > offset] > If offset is 0 and ptr is ~[0, 0], then the result is ~[0, 0] as before. > If offset is positive in ssizetype, then even for VARYING ptr the result is > ~[0, 0] pointer. If the offset is (or maybe could be) negative in > ssizetype, then for -fno-delete-null-pointer-checks the result is VARYING, > as we could go from a non-NULL pointer back to NULL on those targets; for > -fdelete-null-pointer-checks we do what we've done before, i.e. ~[0, 0]. I'm not sure why we'd treat subtraction and addition any differently, but maybe I'm missing something subtle (or not so subtle). ISTM that ~[0,0] +- still results in ~[0,0] for -fdelete-null-pointer-checks. For -fno-delete-null-pointer-checks ISTM we should indicate "we don't know anything about the result" of such an operation. Jeff
Re: [PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367, take 2)
On Thu, 6 Dec 2018, Jakub Jelinek wrote:
> On Thu, Dec 06, 2018 at 10:05:15AM +0100, Richard Biener wrote:
> > Note I wonder if with -fwrapv-pointer NULL automatically becomes a
> > valid address? Or is only wrapping around half of the address
> > space UB?
>
> Hadn't thought about -fwrapv-pointer, I guess we (especially with
> -fno-delete-null-pointer-checks) need to be even more conservative in that
> case.
>
> Furthermore, I've discovered that the ADDR_EXPR of MEM_REF case actually
> uses get_base_address and therefore the offset on MEM_REF is just one of the
> many possible offsets in the play.
>
> So, this patch punts for -fwrapv-pointer in some further cases, and
> adjusts the vr-values.c ADDR_EXPR handling code so that it sums up all 2 or
> 3 offsets together and looks at the resulting sign. If
> -fdelete-null-pointer-checks -fno-wrapv-pointer, it does what it did before
> in tree-vrp.c and in vr-values.c is even more aggressive than before, as in
> even if the base pointer is varying etc., if the sum of all the offsets
> is provably non-zero, the result is non-NULL. For
> -fno-delete-null-pointer-checks -fno-wrapv-pointer it does this only if the
> resulting offset is positive.
>
> Does this look ok?
Little bit more expensive than before but OK.
Thanks,
Richard.
> 2018-12-06 Jakub Jelinek
>
> PR c/88367
> * tree-vrp.c (extract_range_from_binary_expr): For POINTER_PLUS_EXPR
> with -fno-delete-null-pointer-checks, set_nonnull only if the pointer
> is non-NULL and offset is known to have most significant bit clear.
> * vr-values.c (vr_values::vrp_stmt_computes_nonzero): For ADDR_EXPR
> of MEM_EXPR, return true if the MEM_EXPR has non-zero offset with
> most significant bit clear. If offset does have most significant bit
> set and -fno-delete-null-pointer-checks, don't return true even if
> the base pointer is non-NULL.
>
> * gcc.dg/tree-ssa/pr88367.c: New test.
>
> --- gcc/tree-vrp.c.jj 2018-12-06 11:19:24.170939864 +0100
> +++ gcc/tree-vrp.c2018-12-06 11:50:12.104711210 +0100
> @@ -1673,9 +1673,26 @@ extract_range_from_binary_expr (value_ra
>else if (code == POINTER_PLUS_EXPR)
> {
> /* For pointer types, we are really only interested in asserting
> - whether the expression evaluates to non-NULL. */
> - if (!range_includes_zero_p ()
> - || !range_includes_zero_p ())
> + whether the expression evaluates to non-NULL.
> + With -fno-delete-null-pointer-checks we need to be more
> + conservative. As some object might reside at address 0,
> + then some offset could be added to it and the same offset
> + subtracted again and the result would be NULL.
> + E.g.
> + static int a[12]; where [0] is NULL and
> + ptr = [6];
> + ptr -= 6;
> + ptr will be NULL here, even when there is POINTER_PLUS_EXPR
> + where the first range doesn't include zero and the second one
> + doesn't either. As the second operand is sizetype (unsigned),
> + consider all ranges where the MSB could be set as possible
> + subtractions where the result might be NULL. */
> + if ((!range_includes_zero_p ()
> +|| !range_includes_zero_p ())
> + && !TYPE_OVERFLOW_WRAPS (expr_type)
> + && (flag_delete_null_pointer_checks
> + || (range_int_cst_p ()
> + && !tree_int_cst_sign_bit (vr1.max ()
> vr->set_nonnull (expr_type);
> else if (range_is_null () && range_is_null ())
> vr->set_null (expr_type);
> --- gcc/vr-values.c.jj2018-12-06 11:19:23.550950006 +0100
> +++ gcc/vr-values.c 2018-12-06 12:59:28.26920 +0100
> @@ -297,14 +297,48 @@ vr_values::vrp_stmt_computes_nonzero (gi
>&& gimple_assign_rhs_code (stmt) == ADDR_EXPR)
> {
>tree expr = gimple_assign_rhs1 (stmt);
> - tree base = get_base_address (TREE_OPERAND (expr, 0));
> + poly_int64 bitsize, bitpos;
> + tree offset;
> + machine_mode mode;
> + int unsignedp, reversep, volatilep;
> + tree base = get_inner_reference (TREE_OPERAND (expr, 0), ,
> +, , , ,
> +, );
>
>if (base != NULL_TREE
> && TREE_CODE (base) == MEM_REF
> && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
> {
> - value_range *vr = get_value_range (TREE_OPERAND (base, 0));
> - if (!range_includes_zero_p (vr))
> + poly_offset_int off = 0;
> + bool off_cst = false;
> + if (offset == NULL_TREE || TREE_CODE (offset) == INTEGER_CST)
> + {
> + off = mem_ref_offset (base);
> + if (offset)
> + off += poly_offset_int::from (wi::to_poly_wide (offset),
> + SIGNED);
> + off <<= LOG2_BITS_PER_UNIT;
> +
[PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367, take 2)
On Thu, Dec 06, 2018 at 10:05:15AM +0100, Richard Biener wrote:
> Note I wonder if with -fwrapv-pointer NULL automatically becomes a
> valid address? Or is only wrapping around half of the address
> space UB?
Hadn't thought about -fwrapv-pointer, I guess we (especially with
-fno-delete-null-pointer-checks) need to be even more conservative in that
case.
Furthermore, I've discovered that the ADDR_EXPR of MEM_REF case actually
uses get_base_address and therefore the offset on MEM_REF is just one of the
many possible offsets in the play.
So, this patch punts for -fwrapv-pointer in some further cases, and
adjusts the vr-values.c ADDR_EXPR handling code so that it sums up all 2 or
3 offsets together and looks at the resulting sign. If
-fdelete-null-pointer-checks -fno-wrapv-pointer, it does what it did before
in tree-vrp.c and in vr-values.c is even more aggressive than before, as in
even if the base pointer is varying etc., if the sum of all the offsets
is provably non-zero, the result is non-NULL. For
-fno-delete-null-pointer-checks -fno-wrapv-pointer it does this only if the
resulting offset is positive.
Does this look ok?
2018-12-06 Jakub Jelinek
PR c/88367
* tree-vrp.c (extract_range_from_binary_expr): For POINTER_PLUS_EXPR
with -fno-delete-null-pointer-checks, set_nonnull only if the pointer
is non-NULL and offset is known to have most significant bit clear.
* vr-values.c (vr_values::vrp_stmt_computes_nonzero): For ADDR_EXPR
of MEM_EXPR, return true if the MEM_EXPR has non-zero offset with
most significant bit clear. If offset does have most significant bit
set and -fno-delete-null-pointer-checks, don't return true even if
the base pointer is non-NULL.
* gcc.dg/tree-ssa/pr88367.c: New test.
--- gcc/tree-vrp.c.jj 2018-12-06 11:19:24.170939864 +0100
+++ gcc/tree-vrp.c 2018-12-06 11:50:12.104711210 +0100
@@ -1673,9 +1673,26 @@ extract_range_from_binary_expr (value_ra
else if (code == POINTER_PLUS_EXPR)
{
/* For pointer types, we are really only interested in asserting
-whether the expression evaluates to non-NULL. */
- if (!range_includes_zero_p ()
- || !range_includes_zero_p ())
+whether the expression evaluates to non-NULL.
+With -fno-delete-null-pointer-checks we need to be more
+conservative. As some object might reside at address 0,
+then some offset could be added to it and the same offset
+subtracted again and the result would be NULL.
+E.g.
+static int a[12]; where [0] is NULL and
+ptr = [6];
+ptr -= 6;
+ptr will be NULL here, even when there is POINTER_PLUS_EXPR
+where the first range doesn't include zero and the second one
+doesn't either. As the second operand is sizetype (unsigned),
+consider all ranges where the MSB could be set as possible
+subtractions where the result might be NULL. */
+ if ((!range_includes_zero_p ()
+ || !range_includes_zero_p ())
+ && !TYPE_OVERFLOW_WRAPS (expr_type)
+ && (flag_delete_null_pointer_checks
+ || (range_int_cst_p ()
+ && !tree_int_cst_sign_bit (vr1.max ()
vr->set_nonnull (expr_type);
else if (range_is_null () && range_is_null ())
vr->set_null (expr_type);
--- gcc/vr-values.c.jj 2018-12-06 11:19:23.550950006 +0100
+++ gcc/vr-values.c 2018-12-06 12:59:28.26920 +0100
@@ -297,14 +297,48 @@ vr_values::vrp_stmt_computes_nonzero (gi
&& gimple_assign_rhs_code (stmt) == ADDR_EXPR)
{
tree expr = gimple_assign_rhs1 (stmt);
- tree base = get_base_address (TREE_OPERAND (expr, 0));
+ poly_int64 bitsize, bitpos;
+ tree offset;
+ machine_mode mode;
+ int unsignedp, reversep, volatilep;
+ tree base = get_inner_reference (TREE_OPERAND (expr, 0), ,
+ , , , ,
+ , );
if (base != NULL_TREE
&& TREE_CODE (base) == MEM_REF
&& TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
{
- value_range *vr = get_value_range (TREE_OPERAND (base, 0));
- if (!range_includes_zero_p (vr))
+ poly_offset_int off = 0;
+ bool off_cst = false;
+ if (offset == NULL_TREE || TREE_CODE (offset) == INTEGER_CST)
+ {
+ off = mem_ref_offset (base);
+ if (offset)
+ off += poly_offset_int::from (wi::to_poly_wide (offset),
+ SIGNED);
+ off <<= LOG2_BITS_PER_UNIT;
+ off += bitpos;
+ off_cst = true;
+ }
+ /* If >a is equal to X and X is ~[0, 0], the result is too.
+For -fdelete-null-pointer-checks -fno-wrapv-pointer we
Re: [PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367)
On Thu, 6 Dec 2018, Jakub Jelinek wrote:
> Hi!
>
> If we consider -fno-delete-null-pointer-checks as a way to support e.g. AVR
> and other targets which can validly place objects at NULL rather than a way
> to workaround UBs in code, I believe the following testcase must pass if
> there is e.g.
> char a[32]; // And this object ends up at address 0
> void bar (void);
> int main () { foo ([3]); baz ([6]); }
> but fails right now. As mentioned in the PR, in GCC 8 we used to do:
> else if (code == POINTER_PLUS_EXPR)
> {
> /* For pointer types, we are really only interested in asserting
> whether the expression evaluates to non-NULL. */
> if (range_is_nonnull () || range_is_nonnull ())
> set_value_range_to_nonnull (vr, expr_type);
> and that triggered pretty much never, as range_is_nonnull requires that the
> offset is ~[0, 0] exactly, e.g. if it is a constant, it is never that way,
> but now we do:
> if (!range_includes_zero_p () || !range_includes_zero_p ())
> which is e.g. always if the offset is constant non-zero.
>
> I hope we can still say that pointer wrapping even with
> -fno-delete-null-pointer-checks is UB, so this patch differentiates between
> positive offsets (in ssizetype), negative offsets (in ssizetype) and zero
> offsets and handles both the same for both ptr p+ offset and _REF[ptr,
> offset]
> If offset is 0 and ptr is ~[0, 0], then the result is ~[0, 0] as before.
> If offset is positive in ssizetype, then even for VARYING ptr the result is
> ~[0, 0] pointer. If the offset is (or maybe could be) negative in
> ssizetype, then for -fno-delete-null-pointer-checks the result is VARYING,
> as we could go from a non-NULL pointer back to NULL on those targets; for
> -fdelete-null-pointer-checks we do what we've done before, i.e. ~[0, 0].
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Note I wonder if with -fwrapv-pointer NULL automatically becomes a
valid address? Or is only wrapping around half of the address
space UB?
Richard.
> 2018-12-06 Jakub Jelinek
>
> PR c/88367
> * tree-vrp.c (extract_range_from_binary_expr): For POINTER_PLUS_EXPR
> with -fno-delete-null-pointer-checks, set_nonnull only if the pointer
> is non-NULL and offset is known to have most significant bit clear.
> * vr-values.c (vr_values::vrp_stmt_computes_nonzero): For ADDR_EXPR
> of MEM_EXPR, return true if the MEM_EXPR has non-zero offset with
> most significant bit clear. If offset does have most significant bit
> set and -fno-delete-null-pointer-checks, don't return true even if
> the base pointer is non-NULL.
>
> * gcc.dg/tree-ssa/pr88367.c: New test.
>
> --- gcc/tree-vrp.c.jj 2018-12-04 13:00:02.408635579 +0100
> +++ gcc/tree-vrp.c2018-12-05 19:07:36.187567781 +0100
> @@ -1673,9 +1673,25 @@ extract_range_from_binary_expr (value_ra
>else if (code == POINTER_PLUS_EXPR)
> {
> /* For pointer types, we are really only interested in asserting
> - whether the expression evaluates to non-NULL. */
> - if (!range_includes_zero_p ()
> - || !range_includes_zero_p ())
> + whether the expression evaluates to non-NULL.
> + With -fno-delete-null-pointer-checks we need to be more
> + conservative. As some object might reside at address 0,
> + then some offset could be added to it and the same offset
> + subtracted again and the result would be NULL.
> + E.g.
> + static int a[12]; where [0] is NULL and
> + ptr = [6];
> + ptr -= 6;
> + ptr will be NULL here, even when there is POINTER_PLUS_EXPR
> + where the first range doesn't include zero and the second one
> + doesn't either. As the second operand is sizetype (unsigned),
> + consider all ranges where the MSB could be set as possible
> + subtractions where the result might be NULL. */
> + if ((!range_includes_zero_p ()
> +|| !range_includes_zero_p ())
> + && (flag_delete_null_pointer_checks
> + || (range_int_cst_p ()
> + && !tree_int_cst_sign_bit (vr1.max ()
> vr->set_nonnull (expr_type);
> else if (range_is_null () && range_is_null ())
> vr->set_null (expr_type);
> --- gcc/vr-values.c.jj2018-11-29 08:41:33.152749436 +0100
> +++ gcc/vr-values.c 2018-12-05 19:37:56.222582823 +0100
> @@ -303,8 +303,17 @@ vr_values::vrp_stmt_computes_nonzero (gi
> && TREE_CODE (base) == MEM_REF
> && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
> {
> - value_range *vr = get_value_range (TREE_OPERAND (base, 0));
> - if (!range_includes_zero_p (vr))
> + if (integer_zerop (TREE_OPERAND (base, 1))
> + || flag_delete_null_pointer_checks)
> + {
> + value_range *vr = get_value_range
[PATCH] Fix VRP with -fno-delete-null-pointer-checks (PR c/88367)
Hi!
If we consider -fno-delete-null-pointer-checks as a way to support e.g. AVR
and other targets which can validly place objects at NULL rather than a way
to workaround UBs in code, I believe the following testcase must pass if
there is e.g.
char a[32]; // And this object ends up at address 0
void bar (void);
int main () { foo ([3]); baz ([6]); }
but fails right now. As mentioned in the PR, in GCC 8 we used to do:
else if (code == POINTER_PLUS_EXPR)
{
/* For pointer types, we are really only interested in asserting
whether the expression evaluates to non-NULL. */
if (range_is_nonnull () || range_is_nonnull ())
set_value_range_to_nonnull (vr, expr_type);
and that triggered pretty much never, as range_is_nonnull requires that the
offset is ~[0, 0] exactly, e.g. if it is a constant, it is never that way,
but now we do:
if (!range_includes_zero_p () || !range_includes_zero_p ())
which is e.g. always if the offset is constant non-zero.
I hope we can still say that pointer wrapping even with
-fno-delete-null-pointer-checks is UB, so this patch differentiates between
positive offsets (in ssizetype), negative offsets (in ssizetype) and zero
offsets and handles both the same for both ptr p+ offset and _REF[ptr,
offset]
If offset is 0 and ptr is ~[0, 0], then the result is ~[0, 0] as before.
If offset is positive in ssizetype, then even for VARYING ptr the result is
~[0, 0] pointer. If the offset is (or maybe could be) negative in
ssizetype, then for -fno-delete-null-pointer-checks the result is VARYING,
as we could go from a non-NULL pointer back to NULL on those targets; for
-fdelete-null-pointer-checks we do what we've done before, i.e. ~[0, 0].
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2018-12-06 Jakub Jelinek
PR c/88367
* tree-vrp.c (extract_range_from_binary_expr): For POINTER_PLUS_EXPR
with -fno-delete-null-pointer-checks, set_nonnull only if the pointer
is non-NULL and offset is known to have most significant bit clear.
* vr-values.c (vr_values::vrp_stmt_computes_nonzero): For ADDR_EXPR
of MEM_EXPR, return true if the MEM_EXPR has non-zero offset with
most significant bit clear. If offset does have most significant bit
set and -fno-delete-null-pointer-checks, don't return true even if
the base pointer is non-NULL.
* gcc.dg/tree-ssa/pr88367.c: New test.
--- gcc/tree-vrp.c.jj 2018-12-04 13:00:02.408635579 +0100
+++ gcc/tree-vrp.c 2018-12-05 19:07:36.187567781 +0100
@@ -1673,9 +1673,25 @@ extract_range_from_binary_expr (value_ra
else if (code == POINTER_PLUS_EXPR)
{
/* For pointer types, we are really only interested in asserting
-whether the expression evaluates to non-NULL. */
- if (!range_includes_zero_p ()
- || !range_includes_zero_p ())
+whether the expression evaluates to non-NULL.
+With -fno-delete-null-pointer-checks we need to be more
+conservative. As some object might reside at address 0,
+then some offset could be added to it and the same offset
+subtracted again and the result would be NULL.
+E.g.
+static int a[12]; where [0] is NULL and
+ptr = [6];
+ptr -= 6;
+ptr will be NULL here, even when there is POINTER_PLUS_EXPR
+where the first range doesn't include zero and the second one
+doesn't either. As the second operand is sizetype (unsigned),
+consider all ranges where the MSB could be set as possible
+subtractions where the result might be NULL. */
+ if ((!range_includes_zero_p ()
+ || !range_includes_zero_p ())
+ && (flag_delete_null_pointer_checks
+ || (range_int_cst_p ()
+ && !tree_int_cst_sign_bit (vr1.max ()
vr->set_nonnull (expr_type);
else if (range_is_null () && range_is_null ())
vr->set_null (expr_type);
--- gcc/vr-values.c.jj 2018-11-29 08:41:33.152749436 +0100
+++ gcc/vr-values.c 2018-12-05 19:37:56.222582823 +0100
@@ -303,8 +303,17 @@ vr_values::vrp_stmt_computes_nonzero (gi
&& TREE_CODE (base) == MEM_REF
&& TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
{
- value_range *vr = get_value_range (TREE_OPERAND (base, 0));
- if (!range_includes_zero_p (vr))
+ if (integer_zerop (TREE_OPERAND (base, 1))
+ || flag_delete_null_pointer_checks)
+ {
+ value_range *vr = get_value_range (TREE_OPERAND (base, 0));
+ if (!range_includes_zero_p (vr))
+ return true;
+ }
+ /* If MEM_REF has a "positive" offset, consider it non-NULL
+always. */
+ if (integer_nonzerop (TREE_OPERAND (base, 1))
+ &&
