Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On Tue, May 31, 2016 at 1:18 PM, Martin Sebor wrote: > On 03/31/2016 11:54 AM, Jason Merrill wrote: >> >> OK, thanks. > > This bug was fixed in 6.1 but it is also a GCC 5 regression and > the patch hasn't been applied there yet. Should I go ahead and > backport it to the 5.x branch? It seems safe enough to me, but since it touches fold-const let's get a release manager opinion as well. Jakub? Jason
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/31/2016 11:54 AM, Jason Merrill wrote: OK, thanks. This bug was fixed in 6.1 but it is also a GCC 5 regression and the patch hasn't been applied there yet. Should I go ahead and backport it to the 5.x branch? Martin
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
OK, thanks. Jason
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
+ /* Avoid folding references to struct members at offset 0 to + prevent tests like '&ptr->firstmember == 0' from getting + eliminated. When ptr is null, although the -> expression + is strictly speaking invalid, GCC retains it as a matter + of QoI. See PR c/44555. */ + && (TREE_CODE (op0) != ADDR_EXPR + || TREE_CODE (TREE_OPERAND (op0, 0)) != COMPONENT_REF + || compare_tree_int (DECL_FIELD_OFFSET ((TREE_OPERAND + (TREE_OPERAND (op0, 0), 1))), 0)) Can we look at offset/bitpos here rather than examine the tree structure of op0? get_inner_reference already examined it for us. Good suggestion, thanks! But you're still examining the tree structure: + && (TREE_CODE (op0) != ADDR_EXPR + || TREE_CODE (TREE_OPERAND (op0, 0)) != COMPONENT_REF + || bitpos0 != 0) Here instead of looking at op0 we can check (offset0 == NULL_TREE && bitpos0 != 0), which indicates a constant non-zero offset. + && TREE_CODE (arg1) == INTEGER_CST + && integer_zerop (arg1)) And here you don't need the check for INTEGER_CST. I've updated the patch to simplify the tests. I have to confess I don't yet find it straightforward to tell exactly which tests are essential to avoid an ICE and which ones can safely be simplified. Martin PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end of array fails inside constant expression PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript gcc/testsuite/ChangeLog: 2016-03-31 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/70228 * g++.dg/cpp0x/constexpr-array-ptr10.C: New test. * g++.dg/cpp0x/constexpr-array-ptr9.C: New test. * g++.dg/cpp0x/constexpr-nullptr-1.C: New test. * g++.dg/cpp0x/constexpr-array5.C: Adjust text of expected diagnostic. * g++.dg/cpp0x/constexpr-string.C: Same. * g++.dg/cpp0x/constexpr-wstring2.C: Same. * g++.dg/cpp0x/pr65398.C: Same. * g++.dg/ext/constexpr-vla1.C: Same. * g++.dg/ext/constexpr-vla2.C: Same. * g++.dg/ext/constexpr-vla3.C: Same. * g++.dg/ubsan/pr63956.C: Same. gcc/cp/ChangeLog: 2016-03-31 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/70228 * constexpr.c (diag_array_subscript): New function. (cxx_eval_array_reference): Detect out of bounds array indices. gcc/ChangeLog: 2016-03-31 Martin Sebor PR c++/67376 * fold-const.c (maybe_nonzero_address): New function. (fold_comparison): Call it. Fold equality and relational expressions involving null pointers. (tree_single_nonzero_warnv_p): Call maybe_nonzero_address. diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 8ea7111..5d1b8b3 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -1837,6 +1837,30 @@ find_array_ctor_elt (tree ary, tree dindex, bool insert = false) return -1; } +/* Under the control of CTX, issue a detailed diagnostic for + an out-of-bounds subscript INDEX into the expression ARRAY. */ + +static void +diag_array_subscript (const constexpr_ctx *ctx, tree array, tree index) +{ + if (!ctx->quiet) +{ + tree arraytype = TREE_TYPE (array); + + /* Convert the unsigned array subscript to a signed integer to avoid + printing huge numbers for small negative values. */ + tree sidx = fold_convert (ssizetype, index); + if (DECL_P (array)) + { + error ("array subscript value %qE is outside the bounds " + "of array %qD of type %qT", sidx, array, arraytype); + inform (DECL_SOURCE_LOCATION (array), "declared here"); + } + else + error ("array subscript value %qE is outside the bounds " + "of array type %qT", sidx, arraytype); +} +} /* Subroutine of cxx_eval_constant_expression. Attempt to reduce a reference to an array slot. */ @@ -1885,8 +1909,7 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, if (!tree_fits_shwi_p (index) || (i = tree_to_shwi (index)) < 0) { - if (!ctx->quiet) - error ("negative array subscript"); + diag_array_subscript (ctx, ary, index); *non_constant_p = true; return t; } @@ -1898,8 +1921,7 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, VERIFY_CONSTANT (nelts); if (!tree_int_cst_lt (index, nelts)) { - if (!ctx->quiet) - error ("array subscript out of bound"); + diag_array_subscript (ctx, ary, index); *non_constant_p = true; return t; } diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 44fe2a2..eb66143 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -8336,6 +8336,20 @@ pointer_may_wrap_p (tree base, tree offset, HOST_WIDE_INT bitpos) return total.to_uhwi () > (unsigned HOST_WIDE_INT) size; }
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/30/2016 01:25 PM, Jason Merrill wrote: On 03/30/2016 12:32 PM, Martin Sebor wrote: On 03/30/2016 09:30 AM, Jason Merrill wrote: On 03/29/2016 11:57 PM, Martin Sebor wrote: Are we confident that arr[0] won't make it here as POINTER_PLUS_EXPR or some such? I'm as confident as I can be given that this is my first time working in this area. Which piece of code or what assumption in particular are you concerned about? I want to be sure that we don't fold these conditions to false. constexpr int *ip = 0; constexpr struct A { int ar[3]; } *ap = 0; static_assert(&ip[0] == 0); static_assert(&(ap->ar[0]) == 0); I see. Thanks for clarifying. The asserts pass. The expressions are folded earlier on (in fact, as we discussed, the second one too early and is accepted even though it's undefined and should be rejected in a constexpr context) and never reach fold_comparison. Good, then let's add at least the first to one of the tests. + /* Avoid folding references to struct members at offset 0 to + prevent tests like '&ptr->firstmember == 0' from getting + eliminated. When ptr is null, although the -> expression + is strictly speaking invalid, GCC retains it as a matter + of QoI. See PR c/44555. */ + && (TREE_CODE (op0) != ADDR_EXPR + || TREE_CODE (TREE_OPERAND (op0, 0)) != COMPONENT_REF + || compare_tree_int (DECL_FIELD_OFFSET ((TREE_OPERAND + (TREE_OPERAND (op0, 0), 1))), 0)) Can we look at offset/bitpos here rather than examine the tree structure of op0? get_inner_reference already examined it for us. Also, it looks like you aren't handling the case with the operands switched, i.e. 0 == p and such. Presumably all this stuff runs prior to the call to shorten_compare and friends which would canonicalize 0 == p into p == 0? And yes, I still want to separate the canonicalization, warning and optimization that's done by shorten_compare and friends. It just fell out of gcc-6 due to lack of time. jeff Jason
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/30/2016 06:50 PM, Martin Sebor wrote: On 03/30/2016 01:25 PM, Jason Merrill wrote: On 03/30/2016 12:32 PM, Martin Sebor wrote: On 03/30/2016 09:30 AM, Jason Merrill wrote: On 03/29/2016 11:57 PM, Martin Sebor wrote: Are we confident that arr[0] won't make it here as POINTER_PLUS_EXPR or some such? I'm as confident as I can be given that this is my first time working in this area. Which piece of code or what assumption in particular are you concerned about? I want to be sure that we don't fold these conditions to false. constexpr int *ip = 0; constexpr struct A { int ar[3]; } *ap = 0; static_assert(&ip[0] == 0); static_assert(&(ap->ar[0]) == 0); I see. Thanks for clarifying. The asserts pass. The expressions are folded earlier on (in fact, as we discussed, the second one too early and is accepted even though it's undefined and should be rejected in a constexpr context) and never reach fold_comparison. Good, then let's add at least the first to one of the tests. I've enhanced the new constexpr-nullptr-1.C test to verify this. I added assertions exercising the relational expressions as well and for sanity compiled the test with CLang. It turns out that it rejects the relational expressions with null pointers like the one below complaining they aren't constant. constexpr int i = 0; constexpr const int *p = &i; constexpr int *q = 0; static_assert (q < p, "q < p"); I ended up not using a static_assert for the unspecified subset even though GCC accepts it. It seems that they really aren't valid constant expressions (their results are unspecified for null pointers) and should be rejected. Do you agree? (If you do, I'll add these cases to c++/70248 that's already tracking another unspecified case that GCC incorrectly accepts). I agree. + /* Avoid folding references to struct members at offset 0 to + prevent tests like '&ptr->firstmember == 0' from getting + eliminated. When ptr is null, although the -> expression + is strictly speaking invalid, GCC retains it as a matter + of QoI. See PR c/44555. */ + && (TREE_CODE (op0) != ADDR_EXPR + || TREE_CODE (TREE_OPERAND (op0, 0)) != COMPONENT_REF + || compare_tree_int (DECL_FIELD_OFFSET ((TREE_OPERAND + (TREE_OPERAND (op0, 0), 1))), 0)) Can we look at offset/bitpos here rather than examine the tree structure of op0? get_inner_reference already examined it for us. Good suggestion, thanks! But you're still examining the tree structure: + && (TREE_CODE (op0) != ADDR_EXPR + || TREE_CODE (TREE_OPERAND (op0, 0)) != COMPONENT_REF + || bitpos0 != 0) Here instead of looking at op0 we can check (offset0 == NULL_TREE && bitpos0 != 0), which indicates a constant non-zero offset. + && TREE_CODE (arg1) == INTEGER_CST + && integer_zerop (arg1)) And here you don't need the check for INTEGER_CST. Also, it looks like you aren't handling the case with the operands switched, i.e. 0 == p and such. Based on my testing and reading the code I believe the caller (fold_binary_loc) arranges for the constant argument to always come second in comparisons. I've added a comment to the code to make it clear. Great. Jason
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/30/2016 01:25 PM, Jason Merrill wrote: On 03/30/2016 12:32 PM, Martin Sebor wrote: On 03/30/2016 09:30 AM, Jason Merrill wrote: On 03/29/2016 11:57 PM, Martin Sebor wrote: Are we confident that arr[0] won't make it here as POINTER_PLUS_EXPR or some such? I'm as confident as I can be given that this is my first time working in this area. Which piece of code or what assumption in particular are you concerned about? I want to be sure that we don't fold these conditions to false. constexpr int *ip = 0; constexpr struct A { int ar[3]; } *ap = 0; static_assert(&ip[0] == 0); static_assert(&(ap->ar[0]) == 0); I see. Thanks for clarifying. The asserts pass. The expressions are folded earlier on (in fact, as we discussed, the second one too early and is accepted even though it's undefined and should be rejected in a constexpr context) and never reach fold_comparison. Good, then let's add at least the first to one of the tests. I've enhanced the new constexpr-nullptr-1.C test to verify this. I added assertions exercising the relational expressions as well and for sanity compiled the test with CLang. It turns out that it rejects the relational expressions with null pointers like the one below complaining they aren't constant. constexpr int i = 0; constexpr const int *p = &i; constexpr int *q = 0; static_assert (q < p, "q < p"); I ended up not using a static_assert for the unspecified subset even though GCC accepts it. It seems that they really aren't valid constant expressions (their results are unspecified for null pointers) and should be rejected. Do you agree? (If you do, I'll add these cases to c++/70248 that's already tracking another unspecified case that GCC incorrectly accepts). + /* Avoid folding references to struct members at offset 0 to + prevent tests like '&ptr->firstmember == 0' from getting + eliminated. When ptr is null, although the -> expression + is strictly speaking invalid, GCC retains it as a matter + of QoI. See PR c/44555. */ + && (TREE_CODE (op0) != ADDR_EXPR + || TREE_CODE (TREE_OPERAND (op0, 0)) != COMPONENT_REF + || compare_tree_int (DECL_FIELD_OFFSET ((TREE_OPERAND + (TREE_OPERAND (op0, 0), 1))), 0)) Can we look at offset/bitpos here rather than examine the tree structure of op0? get_inner_reference already examined it for us. Good suggestion, thanks! Also, it looks like you aren't handling the case with the operands switched, i.e. 0 == p and such. Based on my testing and reading the code I believe the caller (fold_binary_loc) arranges for the constant argument to always come second in comparisons. I've added a comment to the code to make it clear. Attached is an updated patch retested on x86_64. Martin PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end of array fails inside constant expression PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript gcc/testsuite/ChangeLog: 2016-03-30 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/70228 * g++.dg/cpp0x/constexpr-array-ptr10.C: New test. * g++.dg/cpp0x/constexpr-array-ptr9.C: New test. * g++.dg/cpp0x/constexpr-nullptr-1.C: New test. * g++.dg/cpp0x/constexpr-array5.C: Adjust text of expected diagnostic. * g++.dg/cpp0x/constexpr-string.C: Same. * g++.dg/cpp0x/constexpr-wstring2.C: Same. * g++.dg/cpp0x/pr65398.C: Same. * g++.dg/ext/constexpr-vla1.C: Same. * g++.dg/ext/constexpr-vla2.C: Same. * g++.dg/ext/constexpr-vla3.C: Same. * g++.dg/ubsan/pr63956.C: Same. gcc/cp/ChangeLog: 2016-03-30 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/70228 * constexpr.c (diag_array_subscript): New function. (cxx_eval_array_reference): Detect out of bounds array indices. gcc/ChangeLog: 2016-03-30 Martin Sebor PR c++/67376 * fold-const.c (maybe_nonzero_address): New function. (fold_comparison): Call it. Fold equality and relational expressions involving null pointers. (tree_single_nonzero_warnv_p): Call maybe_nonzero_address. diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 8ea7111..5d1b8b3 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -1837,6 +1837,30 @@ find_array_ctor_elt (tree ary, tree dindex, bool insert = false) return -1; } +/* Under the control of CTX, issue a detailed diagnostic for + an out-of-bounds subscript INDEX into the expression ARRAY. */ + +static void +diag_array_subscript (const constexpr_ctx *ctx, tree array, tree index) +{ + if (!ctx->quiet) +{ + tree arraytype = TREE_TYPE (array); + + /* Convert the unsigned array subscript to a signed integer to avoid + printing huge numbers for small negative v
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/30/2016 12:32 PM, Martin Sebor wrote: On 03/30/2016 09:30 AM, Jason Merrill wrote: On 03/29/2016 11:57 PM, Martin Sebor wrote: Are we confident that arr[0] won't make it here as POINTER_PLUS_EXPR or some such? I'm as confident as I can be given that this is my first time working in this area. Which piece of code or what assumption in particular are you concerned about? I want to be sure that we don't fold these conditions to false. constexpr int *ip = 0; constexpr struct A { int ar[3]; } *ap = 0; static_assert(&ip[0] == 0); static_assert(&(ap->ar[0]) == 0); I see. Thanks for clarifying. The asserts pass. The expressions are folded earlier on (in fact, as we discussed, the second one too early and is accepted even though it's undefined and should be rejected in a constexpr context) and never reach fold_comparison. Good, then let's add at least the first to one of the tests. + /* Avoid folding references to struct members at offset 0 to + prevent tests like '&ptr->firstmember == 0' from getting + eliminated. When ptr is null, although the -> expression + is strictly speaking invalid, GCC retains it as a matter + of QoI. See PR c/44555. */ + && (TREE_CODE (op0) != ADDR_EXPR + || TREE_CODE (TREE_OPERAND (op0, 0)) != COMPONENT_REF + || compare_tree_int (DECL_FIELD_OFFSET ((TREE_OPERAND + (TREE_OPERAND (op0, 0), 1))), 0)) Can we look at offset/bitpos here rather than examine the tree structure of op0? get_inner_reference already examined it for us. Also, it looks like you aren't handling the case with the operands switched, i.e. 0 == p and such. Jason
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/30/2016 09:30 AM, Jason Merrill wrote: On 03/29/2016 11:57 PM, Martin Sebor wrote: Are we confident that arr[0] won't make it here as POINTER_PLUS_EXPR or some such? I'm as confident as I can be given that this is my first time working in this area. Which piece of code or what assumption in particular are you concerned about? I want to be sure that we don't fold these conditions to false. constexpr int *ip = 0; constexpr struct A { int ar[3]; } *ap = 0; static_assert(&ip[0] == 0); static_assert(&(ap->ar[0]) == 0); I see. Thanks for clarifying. The asserts pass. The expressions are folded earlier on (in fact, as we discussed, the second one too early and is accepted even though it's undefined and should be rejected in a constexpr context) and never reach fold_comparison. Martin
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/29/2016 11:57 PM, Martin Sebor wrote: Are we confident that arr[0] won't make it here as POINTER_PLUS_EXPR or some such? I'm as confident as I can be given that this is my first time working in this area. Which piece of code or what assumption in particular are you concerned about? I want to be sure that we don't fold these conditions to false. constexpr int *ip = 0; constexpr struct A { int ar[3]; } *ap = 0; static_assert(&ip[0] == 0); static_assert(&(ap->ar[0]) == 0); Jason
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/29/2016 12:54 PM, Jason Merrill wrote: On 03/28/2016 06:04 PM, Martin Sebor wrote: + && compare_tree_int (arg1, 0) == 0) This can be integer_zerop. Sure. +case GE_EXPR: +case EQ_EXPR: +case LE_EXPR: + return boolean_false_node; +case GT_EXPR: +case LT_EXPR: +case NE_EXPR: + return boolean_true_node; EQ and NE make sense, but I would expect both > and >= to be true, < and <= to be false. I was convinced I had a reason for this but it doesn't seem to affect regression test results so I must have been wrong. Relational expressions involving object and null pointers are undefined in C and I thought unspecified in C++, but given that GCC evaluates (0 < p) to true it looks like you're right and C++ does seem to require all the others to evaluate as you said. With the decision to remove the nullptr changes I tried to keep the amount of testing of null pointers to the minimum necessary to exercise the fix for comment #10 on 67376. In light of your expectation I've added a test to better exercise the relational expressions involving pointers to struct data members. Interestingly, stepping through it revealed that the problem cases you pointed out above are actually handled by generic_simplify() and never end up in fold_comparison(). Attached is an updated patch. Are we confident that arr[0] won't make it here as POINTER_PLUS_EXPR or some such? I'm as confident as I can be given that this is my first time working in this area. Which piece of code or what assumption in particular are you concerned about? Martin PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end of array fails inside constant expression PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript gcc/testsuite/ChangeLog: 2016-03-29 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/70228 * g++.dg/cpp0x/constexpr-array-ptr10.C: New test. * g++.dg/cpp0x/constexpr-array-ptr9.C: New test. * g++.dg/cpp0x/constexpr-nullptr-1.C: New test. * g++.dg/cpp0x/constexpr-array5.C: Adjust text of expected diagnostic. * g++.dg/cpp0x/constexpr-string.C: Same. * g++.dg/cpp0x/constexpr-wstring2.C: Same. * g++.dg/cpp0x/pr65398.C: Same. * g++.dg/ext/constexpr-vla1.C: Same. * g++.dg/ext/constexpr-vla2.C: Same. * g++.dg/ext/constexpr-vla3.C: Same. * g++.dg/ubsan/pr63956.C: Same. gcc/cp/ChangeLog: 2016-03-29 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/70228 * constexpr.c (diag_array_subscript): New function. (cxx_eval_array_reference): Detect out of bounds array indices. gcc/ChangeLog: 2016-03-29 Martin Sebor PR c++/67376 * fold-const.c (maybe_nonzero_address): New function. (fold_comparison): Call it. Fold equality and relational expressions involving null pointers. (tree_single_nonzero_warnv_p): Call maybe_nonzero_address. diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 8ea7111..2415094 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -1837,6 +1837,30 @@ find_array_ctor_elt (tree ary, tree dindex, bool insert = false) return -1; } +/* Under the control of CTX, issue a detailed diagnostic for + an out-of-bounds subscript INDEX into the expression ARRAY. */ + +static void +diag_array_subscript (const constexpr_ctx *ctx, tree array, tree index) +{ + if (!ctx->quiet) +{ + tree arraytype = TREE_TYPE (array); + + /* Convert the unsigned array subscript to a signed integer to avoid + printing huge numbers for small negative values. */ + tree sidx = fold_convert (ssizetype, index); + if (DECL_P (array)) + { + error ("array subscript value %qE is outside the bounds " + "of array %qD of type %qT", sidx, array, arraytype); + inform (DECL_SOURCE_LOCATION (array), "declared here"); + } + else + error ("array subscript value %qE is outside the bounds " + "of array type %qT", sidx, arraytype); +} +} /* Subroutine of cxx_eval_constant_expression. Attempt to reduce a reference to an array slot. */ @@ -1861,6 +1885,7 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, false, non_constant_p, overflow_p); VERIFY_CONSTANT (index); + if (lval && ary == oldary && index == oldidx) return t; else if (lval) @@ -1885,8 +1910,7 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, if (!tree_fits_shwi_p (index) || (i = tree_to_shwi (index)) < 0) { - if (!ctx->quiet) - error ("negative array subscript"); + diag_array_subscript (ctx, ary, index); *non_constant_p = true; return t; } @@ -1898,8 +1922,7 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, VERIFY_CONSTANT (nelts); i
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/28/2016 06:04 PM, Martin Sebor wrote: + && compare_tree_int (arg1, 0) == 0) This can be integer_zerop. + case GE_EXPR: + case EQ_EXPR: + case LE_EXPR: + return boolean_false_node; + case GT_EXPR: + case LT_EXPR: + case NE_EXPR: + return boolean_true_node; EQ and NE make sense, but I would expect both > and >= to be true, < and <= to be false. Are we confident that arr[0] won't make it here as POINTER_PLUS_EXPR or some such? Jason
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
I think let's defer the fix for c++/60760 (i.e. the nullptr_p bits) until stage 1, when it can be combined with the POINTER_PLUS_EXPR fix, and put the rest of this patch in now. I can split up the patch into two and post the subset without the fix for c++/60760, though I don't expect to be done with it after I get back (next week). I'd like to understand your concern with the fix for c++/60760. Is it that it's incomplete (doesn't reject taking the address of the first member of a struct, as in &null->first_member), or are you worried that the changes may not be stable enough? More the latter; it seems like significant new code and doesn't fix a regression. Attached is an updated patch without the fix for c++/60760, retested on x86_64. Martin PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end of array fails inside constant expression PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript gcc/testsuite/ChangeLog: 2016-03-18 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/70228 * g++.dg/cpp0x/constexpr-array-ptr10.C: New test. * g++.dg/cpp0x/constexpr-array-ptr9.C: New test. * g++.dg/cpp0x/constexpr-array5.C: Adjust text of expected diagnostic. * g++.dg/cpp0x/constexpr-string.C: Same. * g++.dg/cpp0x/constexpr-wstring2.C: Same. * g++.dg/cpp0x/pr65398.C: Same. * g++.dg/ext/constexpr-vla1.C: Same. * g++.dg/ext/constexpr-vla2.C: Same. * g++.dg/ext/constexpr-vla3.C: Same. * g++.dg/ubsan/pr63956.C: Same. gcc/cp/ChangeLog: 2016-03-18 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/70228 * constexpr.c (diag_array_subscript): New function. (cxx_eval_array_reference): Detect out of bounds array indices. gcc/ChangeLog: 2016-03-18 Martin Sebor PR c++/67376 * fold-const.c (maybe_nonzero_address): New function. (fold_comparison): Call it. Fold equality and relational expressions involving null pointers. (tree_single_nonzero_warnv_p): Call maybe_nonzero_address. diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 7776cac..c900080 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -1837,6 +1837,30 @@ find_array_ctor_elt (tree ary, tree dindex, bool insert = false) return -1; } +/* Under the control of CTX, issue a detailed diagnostic for + an out-of-bounds subscript INDEX into the expression ARRAY. */ + +static void +diag_array_subscript (const constexpr_ctx *ctx, tree array, tree index) +{ + if (!ctx->quiet) +{ + tree arraytype = TREE_TYPE (array); + + /* Convert the unsigned array subscript to a signed integer to avoid + printing huge numbers for small negative values. */ + tree sidx = fold_convert (ssizetype, index); + if (DECL_P (array)) + { + error ("array subscript value %qE is outside the bounds " + "of array %qD of type %qT", sidx, array, arraytype); + inform (DECL_SOURCE_LOCATION (array), "declared here"); + } + else + error ("array subscript value %qE is outside the bounds " + "of array type %qT", sidx, arraytype); +} +} /* Subroutine of cxx_eval_constant_expression. Attempt to reduce a reference to an array slot. */ @@ -1861,6 +1885,7 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, false, non_constant_p, overflow_p); VERIFY_CONSTANT (index); + if (lval && ary == oldary && index == oldidx) return t; else if (lval) @@ -1885,8 +1910,7 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, if (!tree_fits_shwi_p (index) || (i = tree_to_shwi (index)) < 0) { - if (!ctx->quiet) - error ("negative array subscript"); + diag_array_subscript (ctx, ary, index); *non_constant_p = true; return t; } @@ -1898,8 +1922,7 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, VERIFY_CONSTANT (nelts); if (!tree_int_cst_lt (index, nelts)) { - if (!ctx->quiet) - error ("array subscript out of bound"); + diag_array_subscript (ctx, ary, index); *non_constant_p = true; return t; } diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 44fe2a2..3f65243 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -8336,6 +8336,20 @@ pointer_may_wrap_p (tree base, tree offset, HOST_WIDE_INT bitpos) return total.to_uhwi () > (unsigned HOST_WIDE_INT) size; } +/* Return a positive integer when the symbol DECL is known to have + a nonzero address, zero when it's known not to (e.g., it's a weak + symbol), and a negative integer when the symbol is not yet in the + symbol table and so whether or not its address is zero is unknown. */ +static int +maybe_nonzero_address (tree decl) +{ + if (DECL_P (decl) && decl_in_symtab_p (decl)) +if (struct symtab_node *symbol = symtab_node
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/22/2016 04:01 PM, Martin Sebor wrote: On 03/22/2016 12:52 PM, Jason Merrill wrote: On 03/21/2016 06:09 PM, Jeff Law wrote: On 03/21/2016 11:54 AM, Jason Merrill wrote: Both b0 and b1 are invalid and should be diagnosed, but only b1 is. b1 isn't because because by the time we see its initializer in constexpr.c it's been transformed into the equivalent of "b1 = (int*)ps" (though we don't see the cast which would also make it invalid). But if we can avoid these early simplifying transformations and retain a more faithful representation of the original source then doing the checking later will likely be simpler and result in detecting more problems with greater consistency and less effort. Do we know where the folding is happening for this case and is it something we can reasonably defer?ie, is this just a case we missed as part of the deferred folding work and hence should have its own distinct BZ to track? Yes, why is it already folded? Let's pull that out into a separate BZ and tackle it for gcc-7. I need to understand the issue before I agree to defer it. It turns out that the problem is with how cp_build_binary_op calls cp_pointer_int_sum and thus the c-common pointer_int_sum, which folds. The POINTER_PLUS_EXPRs thus created have been a source of many issues with constexpr evaluation, since it's impossible to reconstruct the original expression, especially because POINTER_PLUS_EXPR uses an unsigned second operand. Deferring lowering to POINTER_PLUS_EXPR would help a lot. But it would indeed be a significant risk at this point. I think let's defer the fix for c++/60760 (i.e. the nullptr_p bits) until stage 1, when it can be combined with the POINTER_PLUS_EXPR fix, and put the rest of this patch in now. I can split up the patch into two and post the subset without the fix for c++/60760, though I don't expect to be done with it after I get back (next week). I'd like to understand your concern with the fix for c++/60760. Is it that it's incomplete (doesn't reject taking the address of the first member of a struct, as in &null->first_member), or are you worried that the changes may not be stable enough? More the latter; it seems like significant new code and doesn't fix a regression. Jason
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/22/2016 12:52 PM, Jason Merrill wrote: On 03/21/2016 06:09 PM, Jeff Law wrote: On 03/21/2016 11:54 AM, Jason Merrill wrote: Both b0 and b1 are invalid and should be diagnosed, but only b1 is. b1 isn't because because by the time we see its initializer in constexpr.c it's been transformed into the equivalent of "b1 = (int*)ps" (though we don't see the cast which would also make it invalid). But if we can avoid these early simplifying transformations and retain a more faithful representation of the original source then doing the checking later will likely be simpler and result in detecting more problems with greater consistency and less effort. Do we know where the folding is happening for this case and is it something we can reasonably defer?ie, is this just a case we missed as part of the deferred folding work and hence should have its own distinct BZ to track? Yes, why is it already folded? Let's pull that out into a separate BZ and tackle it for gcc-7. I need to understand the issue before I agree to defer it. It turns out that the problem is with how cp_build_binary_op calls cp_pointer_int_sum and thus the c-common pointer_int_sum, which folds. The POINTER_PLUS_EXPRs thus created have been a source of many issues with constexpr evaluation, since it's impossible to reconstruct the original expression, especially because POINTER_PLUS_EXPR uses an unsigned second operand. Deferring lowering to POINTER_PLUS_EXPR would help a lot. But it would indeed be a significant risk at this point. I think let's defer the fix for c++/60760 (i.e. the nullptr_p bits) until stage 1, when it can be combined with the POINTER_PLUS_EXPR fix, and put the rest of this patch in now. I can split up the patch into two and post the subset without the fix for c++/60760, though I don't expect to be done with it after I get back (next week). I'd like to understand your concern with the fix for c++/60760. Is it that it's incomplete (doesn't reject taking the address of the first member of a struct, as in &null->first_member), or are you worried that the changes may not be stable enough? Martin
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/21/2016 06:09 PM, Jeff Law wrote: On 03/21/2016 11:54 AM, Jason Merrill wrote: Both b0 and b1 are invalid and should be diagnosed, but only b1 is. b1 isn't because because by the time we see its initializer in constexpr.c it's been transformed into the equivalent of "b1 = (int*)ps" (though we don't see the cast which would also make it invalid). But if we can avoid these early simplifying transformations and retain a more faithful representation of the original source then doing the checking later will likely be simpler and result in detecting more problems with greater consistency and less effort. Do we know where the folding is happening for this case and is it something we can reasonably defer?ie, is this just a case we missed as part of the deferred folding work and hence should have its own distinct BZ to track? Yes, why is it already folded? Let's pull that out into a separate BZ and tackle it for gcc-7. I need to understand the issue before I agree to defer it. It turns out that the problem is with how cp_build_binary_op calls cp_pointer_int_sum and thus the c-common pointer_int_sum, which folds. The POINTER_PLUS_EXPRs thus created have been a source of many issues with constexpr evaluation, since it's impossible to reconstruct the original expression, especially because POINTER_PLUS_EXPR uses an unsigned second operand. Deferring lowering to POINTER_PLUS_EXPR would help a lot. But it would indeed be a significant risk at this point. I think let's defer the fix for c++/60760 (i.e. the nullptr_p bits) until stage 1, when it can be combined with the POINTER_PLUS_EXPR fix, and put the rest of this patch in now. Jason
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/21/2016 11:54 AM, Jason Merrill wrote: Both b0 and b1 are invalid and should be diagnosed, but only b1 is. b1 isn't because because by the time we see its initializer in constexpr.c it's been transformed into the equivalent of "b1 = (int*)ps" (though we don't see the cast which would also make it invalid). But if we can avoid these early simplifying transformations and retain a more faithful representation of the original source then doing the checking later will likely be simpler and result in detecting more problems with greater consistency and less effort. Do we know where the folding is happening for this case and is it something we can reasonably defer?ie, is this just a case we missed as part of the deferred folding work and hence should have its own distinct BZ to track? Yes, why is it already folded? Let's pull that out into a separate BZ and tackle it for gcc-7. jeff
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/18/2016 01:04 PM, Jeff Law wrote: On 03/17/2016 03:16 PM, Martin Sebor wrote: The difficulty I've run into with detecting these problems in later phases is that some invalid expressions have already been simplified by the front end. The example that applies here (even though this is still the front end) is this: Yea. I was hoping that the delayed folding work would be helping in getting a more faithful representation out of the front-ends. It should. constexpr int* p = 0; constexpr bool b0 = &p[0] == 0; // accepted constexpr bool b1 = &p[1] == 0; // rejected Both b0 and b1 are invalid and should be diagnosed, but only b1 is. b1 isn't because because by the time we see its initializer in constexpr.c it's been transformed into the equivalent of "b1 = (int*)ps" (though we don't see the cast which would also make it invalid). But if we can avoid these early simplifying transformations and retain a more faithful representation of the original source then doing the checking later will likely be simpler and result in detecting more problems with greater consistency and less effort. Do we know where the folding is happening for this case and is it something we can reasonably defer?ie, is this just a case we missed as part of the deferred folding work and hence should have its own distinct BZ to track? Yes, why is it already folded? Jason
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/14/2016 03:25 PM, Martin Sebor wrote: The attached patch fixes the outstanding cases mentioned in comment 10 on bug c++/67376. While testing the fix I uncovered a number of other related problems without which the test would have been incomplete. They include: PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/60760 - arithmetic on null pointers should not be allowed in constant expressions In addition, I include a fix for the issue below that I also came across while testing the patch and that makes root causing constexpr problems due to out-of-bounds array subscripts easier: PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript In a discussion of bug 70170 between those CC'd Marek posted a prototype patch for match.pd. While the patch seems to do the right thing as far as the bug goes, like my own first attempt at a fix in const-fold.c it caused a couple of regressions (in pr21294.c and in pr44555.c). Since I'm not yet familiar enough with match.pd, in the interest of time I solved those regressions in const-fold.c rather than in match.pd. Tested on x86_64. Martin gcc-67376.patch PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end of array fails inside constant expression PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/60760 - arithmetic on null pointers should not be allowed in constant expressions PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript gcc/testsuite/ChangeLog: 2016-03-14 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/60760 PR c++/70228 * g++.dg/cpp0x/constexpr-array-ptr10.C: New test. * g++.dg/cpp0x/constexpr-array-ptr11.C: New test. * g++.dg/cpp0x/constexpr-array-ptr9.C: New test. * g++.dg/cpp0x/constexpr-array5.C: Adjust text of expected diagnostic. * g++.dg/cpp0x/constexpr-string.C: Same. * g++.dg/cpp0x/constexpr-wstring2.C: Same. * g++.dg/cpp0x/pr65398.C: Same. * g++.dg/ext/constexpr-vla1.C: Same. * g++.dg/ext/constexpr-vla2.C: Same. * g++.dg/ext/constexpr-vla3.C: Same. * g++.dg/ubsan/pr63956.C: Same. gcc/cp/ChangeLog: 2016-03-14 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/60760 PR c++/70228 (cxx_eval_binary_expression): Add argument. (cxx_eval_component_reference): Same. (cxx_eval_constant_expression): Same. (cxx_eval_indirect_ref): Same. (cxx_eval_outermost_constant_expr): Same. (diag_array_subscript): New function. * constexpr.c (cxx_eval_call_expression): Adjust. (cxx_eval_conditional_expression): Same. (cxx_eval_array_reference): Detect null pointers. (cxx_eval_statement_list): Adjust. gcc/ChangeLog: 2016-03-14 Martin Sebor PR c++/67376 * fold-const.c (fold_comparison): Fold equality and relational expressions involving null pointers. diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 5f97c9d..5ec5034 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -918,7 +918,8 @@ struct constexpr_ctx { static GTY (()) hash_table *constexpr_call_table; static tree cxx_eval_constant_expression (const constexpr_ctx *, tree, - bool, bool *, bool *, tree * = NULL); + bool, bool *, bool *, bool * = NULL, + tree * = NULL); I didn't look deeply, but do you end up fixing all (most) of the callers of cxx_eval_constant_expression? If so, then you don't need the default initialization. At a high level, I'm curious your thoughts on emitting these errors out of the front-end rather than after analysis. I'm thinking in particular about constant propagation, unreachable code elimination and DCE. The former can expose cases that are difficult to catch in the front-end, while the latter two might remove an out-of-range comparison/reference. diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 696b4a6..376aa09 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -8639,6 +8639,37 @@ fold_comparison (location_t loc, enum tree_code code, tree type, base1 = build_fold_addr_expr_loc (loc, base1); return fold_build2_loc (loc, code, type, base0, base1); } + + /* Comparison between an ordinary (non-weak) symbol and a null +pointer can be eliminated since such sybols must have a non +null ad
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
static tree cxx_eval_constant_expression (const constexpr_ctx *, tree, - bool, bool *, bool *, tree * = NULL); + bool, bool *, bool *, bool * = NULL, + tree * = NULL); I didn't look deeply, but do you end up fixing all (most) of the callers of cxx_eval_constant_expression? If so, then you don't need the default initialization. Thanks for the comments. The patch only modifies about 10 out of the 70 or so calls to the function in the file and the default argument helps avoid making the rest of the changes. (I have some ideas for improving the APIs of these functions that I'd like to run by Jason when we're done with the 6.0 work.) At a high level, I'm curious your thoughts on emitting these errors out of the front-end rather than after analysis. I'm thinking in particular about constant propagation, unreachable code elimination and DCE. The former can expose cases that are difficult to catch in the front-end, while the latter two might remove an out-of-range comparison/reference. The difficulty I've run into with detecting these problems in later phases is that some invalid expressions have already been simplified by the front end. The example that applies here (even though this is still the front end) is this: constexpr int* p = 0; constexpr bool b0 = &p[0] == 0; // accepted constexpr bool b1 = &p[1] == 0; // rejected Both b0 and b1 are invalid and should be diagnosed, but only b1 is. b1 isn't because because by the time we see its initializer in constexpr.c it's been transformed into the equivalent of "b1 = (int*)ps" (though we don't see the cast which would also make it invalid). But if we can avoid these early simplifying transformations and retain a more faithful representation of the original source then doing the checking later will likely be simpler and result in detecting more problems with greater consistency and less effort. diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 696b4a6..376aa09 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -8639,6 +8639,37 @@ fold_comparison (location_t loc, enum tree_code code, tree type, base1 = build_fold_addr_expr_loc (loc, base1); return fold_build2_loc (loc, code, type, base0, base1); } + + /* Comparison between an ordinary (non-weak) symbol and a null + pointer can be eliminated since such sybols must have a non + null address. */ Hmm, I thought we already had code to do this somewhere. It looks like it's moved around quite a bit. I think you want to be using symtab_node::nonzero_address to determine if a given symbol must bind to a nonzero address. Thanks for the hint. I had looked for existing functions but couldn't find one that worked. decl_with_nonnull_addr_p() in c-common.c looked promising but it's not accessible here and it doesn't do the right thing when HAS_DECL_ASSEMBLER_NAME_P() is false (it ICEs). There are a few functions in the fold-const.c itself that make use of symtab_node::nonzero_address(), either directly or indirectly (tree_expr_nonzero_p and tree_expr_nonzero_warnv_p) but none is usable in this context (they don't handle VAR_DECL, and adding that handling appears more involved than the current approach). In the end, I added a new function, maybe_nonzero_address(), that calls symtab_node::nonzero_address(), and that I factored out of tree_single_nonzero_warnv_p() that I was then able to use in fold_comparison(). I've made a few other small adjustments to the patch to avoid one false positive, and a few test cases, and tweak the expected diagnostics now that Marek has fixed 70194. I've also wrote myself a small sed script to replace blocks of 8 spaces with tabs and ran the patch through it. I'll integrate it into my workflow so I hopefully don't have to worry about this ever again. Martin PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end of array fails inside constant expression PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/60760 - arithmetic on null pointers should not be allowed in constant expressions PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript gcc/testsuite/ChangeLog: 2016-03-17 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/60760 PR c++/70228 * g++.dg/cpp0x/constexpr-array-ptr10.C: New test. * g++.dg/cpp0x/constexpr-array-ptr9.C: New test. * g++.dg/cpp0x/constexpr-array5.C: Adjust text of expected diagnostic. * g++.dg/cpp0x/constexpr-nullptr.C: Add test cases. * g++.dg/cpp0x/constexpr-string.C: Same. * g++.dg/cpp0x/constexpr-wstring2.C: Same. * g++.dg/cpp0x/pr65398.C: Same. * g++.dg/ext/constexpr-vla1.C: Same. * g++.dg/ext/constexpr-vla2.C: Same. * g++.dg/ext/constexpr-vla3.
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/14/2016 04:13 PM, Jakub Jelinek wrote: On Mon, Mar 14, 2016 at 03:25:07PM -0600, Martin Sebor wrote: PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end of array fails inside constant expression PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/60760 - arithmetic on null pointers should not be allowed in constant expressions PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript Can you please check up the formatting in the patch? Seems e.g. you've replaced tons of tabs with 8 spaces etc. (check your editor setting, and check the patch with contrib/check-GNU-style.sh). There is some trailing whitespace too, spaces before [, etc. Jakub, do you have any comments on the substance of the patch? If so, it would help immensely if you could provide them so that Martin could address technical issues at the same time as he fixes up whitespace nits. jeff
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/17/2016 03:16 PM, Martin Sebor wrote: gcc-67376.patch PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end of array fails inside constant expression PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/60760 - arithmetic on null pointers should not be allowed in constant expressions PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript gcc/testsuite/ChangeLog: 2016-03-17 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/60760 PR c++/70228 * g++.dg/cpp0x/constexpr-array-ptr10.C: New test. * g++.dg/cpp0x/constexpr-array-ptr9.C: New test. * g++.dg/cpp0x/constexpr-array5.C: Adjust text of expected diagnostic. * g++.dg/cpp0x/constexpr-nullptr.C: Add test cases. * g++.dg/cpp0x/constexpr-string.C: Same. * g++.dg/cpp0x/constexpr-wstring2.C: Same. * g++.dg/cpp0x/pr65398.C: Same. * g++.dg/ext/constexpr-vla1.C: Same. * g++.dg/ext/constexpr-vla2.C: Same. * g++.dg/ext/constexpr-vla3.C: Same. * g++.dg/ubsan/pr63956.C: Same. gcc/cp/ChangeLog: 2016-03-17 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/60760 PR c++/70228 * constexpr.c (cxx_eval_binary_expression): Add argument. (cxx_eval_component_reference): Same. (cxx_eval_constant_expression): Same. (cxx_eval_indirect_ref): Same. (cxx_eval_outermost_constant_expr): Same. (diag_array_subscript): New function. (cxx_eval_call_expression): Adjust. (cxx_eval_conditional_expression): Same. (cxx_eval_array_reference): Detect null pointers. (cxx_eval_statement_list): Adjust. gcc/ChangeLog: 2016-03-17 Martin Sebor PR c++/67376 * fold-const.c (maybe_nonzero_address): New function. (fold_comparison): Call it. Fold equality and relational expressions involving null pointers. (tree_single_nonzero_warnv_p): Call maybe_nonzero_address. Index: gcc/cp/constexpr.c === --- gcc/cp/constexpr.c (revision 234306) +++ gcc/cp/constexpr.c (working copy) @@ -1839,11 +1874,26 @@ cxx_eval_array_reference (const constexp @@ -3300,10 +3357,21 @@ cxx_eval_constant_expression (const cons + + if (TREE_CODE (t) == INTEGER_CST + && TREE_CODE (TREE_TYPE (t)) == POINTER_TYPE + && !integer_zerop (t)) + { + if (!ctx->quiet) + error ("null pointer arithmetic in %qE", t); + if (nullptr_p) + *nullptr_p = true; + } Something looks odd here. You're testing !integer_zerop, so in T is going to be non-NULL, but you mentioned null pointer arithmetic in the error message. Am I missing something here? @@ -3738,15 +3812,32 @@ cxx_eval_constant_expression (const cons Index: gcc/fold-const.c @@ -8639,6 +8653,38 @@ fold_comparison (location_t loc, enum tr base1 = build_fold_addr_expr_loc (loc, base1); return fold_build2_loc (loc, code, type, base0, base1); } + + /* Comparison between an ordinary (non-weak) symbol and a null +pointer can be eliminated since such sybols must have a non +null address. */ + else if (DECL_P (base0) + && maybe_nonzero_address (base0) > 0 + // && (!HAS_DECL_ASSEMBLER_NAME_P (base0) || !DECL_WEAK (base0)) Please remove the commented out line. + /* Avoid folding references to struct members at offset 0 to + prevent tests like '&ptr->firstmember == 0' from getting + eliminated. When ptr is null, although the -> expression + is strictly speaking invalid, GCC retains it as a matter + of QoI. See PR c/44555. */ + && (TREE_CODE (op0) != ADDR_EXPR + || TREE_CODE (TREE_OPERAND (op0, 0)) != COMPONENT_REF + || compare_tree_int (DECL_FIELD_OFFSET ((TREE_OPERAND + (TREE_OPERAND (op0, 0), 1))), 0)) + && TREE_CODE (arg1) == INTEGER_CST + && compare_tree_int (arg1, 0) == 0) + { + switch (code) + { + case GE_EXPR: + case EQ_EXPR: + case LE_EXPR: + return boolean_false_node; + case GT_EXPR: + case LT_EXPR: + case NE_EXPR: + return boolean_true_node; + default: gcc_unreachable (); Can you put the gcc_unreachable on a new line? I know there's a few cases in the sources where it's on the default: line, but the vast majority have it on its own line.
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On 03/17/2016 03:16 PM, Martin Sebor wrote: static tree cxx_eval_constant_expression (const constexpr_ctx *, tree, - bool, bool *, bool *, tree * = NULL); + bool, bool *, bool *, bool * = NULL, + tree * = NULL); I didn't look deeply, but do you end up fixing all (most) of the callers of cxx_eval_constant_expression? If so, then you don't need the default initialization. Thanks for the comments. The patch only modifies about 10 out of the 70 or so calls to the function in the file and the default argument helps avoid making the rest of the changes. (I have some ideas for improving the APIs of these functions that I'd like to run by Jason when we're done with the 6.0 work.) OK. Then let's keep the default initialization. The difficulty I've run into with detecting these problems in later phases is that some invalid expressions have already been simplified by the front end. The example that applies here (even though this is still the front end) is this: Yea. I was hoping that the delayed folding work would be helping in getting a more faithful representation out of the front-ends. constexpr int* p = 0; constexpr bool b0 = &p[0] == 0; // accepted constexpr bool b1 = &p[1] == 0; // rejected Both b0 and b1 are invalid and should be diagnosed, but only b1 is. b1 isn't because because by the time we see its initializer in constexpr.c it's been transformed into the equivalent of "b1 = (int*)ps" (though we don't see the cast which would also make it invalid). But if we can avoid these early simplifying transformations and retain a more faithful representation of the original source then doing the checking later will likely be simpler and result in detecting more problems with greater consistency and less effort. Do we know where the folding is happening for this case and is it something we can reasonably defer?ie, is this just a case we missed as part of the deferred folding work and hence should have its own distinct BZ to track? Hmm, I thought we already had code to do this somewhere. It looks like it's moved around quite a bit. I think you want to be using symtab_node::nonzero_address to determine if a given symbol must bind to a nonzero address. Thanks for the hint. I had looked for existing functions but couldn't find one that worked. decl_with_nonnull_addr_p() in c-common.c looked promising but it's not accessible here and it doesn't do the right thing when HAS_DECL_ASSEMBLER_NAME_P() is false (it ICEs). Yea, I found the same mis-mash of bits that didn't look directly usable for the problem you're tackling. What's odd is I would have sworn that we had code to do exactly what you wanted, but I wasn't able to find it, either as a distinct routine or open-coded. In the end, I added a new function, maybe_nonzero_address(), that calls symtab_node::nonzero_address(), and that I factored out of tree_single_nonzero_warnv_p() that I was then able to use in fold_comparison(). Sounds good. I've made a few other small adjustments to the patch to avoid one false positive, and a few test cases, and tweak the expected diagnostics now that Marek has fixed 70194. I've also wrote myself a small sed script to replace blocks of 8 spaces with tabs and ran the patch through it. I'll integrate it into my workflow so I hopefully don't have to worry about this ever again. I'll try to take a look at the updated patch shortly. It may still hit too much of the C++ front-end for me to be comfortable reviewing -- we'll see. jeff
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On Wed, Mar 16, 2016 at 01:38:21PM -0600, Jeff Law wrote: > On 03/14/2016 04:13 PM, Jakub Jelinek wrote: > >On Mon, Mar 14, 2016 at 03:25:07PM -0600, Martin Sebor wrote: > >>PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end > >>of array fails inside constant expression > >>PR c++/70170 - [6 regression] bogus not a constant expression error > >>comparing > >>pointer to array to null > >>PR c++/70172 - incorrect reinterpret_cast from integer to pointer error > >>on invalid constexpr initialization > >>PR c++/60760 - arithmetic on null pointers should not be allowed in constant > >>expressions > >>PR c++/70228 - insufficient detail in diagnostics for a constexpr out of > >>bounds > >>array subscript > > > >Can you please check up the formatting in the patch? > >Seems e.g. you've replaced tons of tabs with 8 spaces etc. (check your > >editor setting, and check the patch with contrib/check-GNU-style.sh). > >There is some trailing whitespace too, spaces before [, etc. > Jakub, do you have any comments on the substance of the patch? If so, it > would help immensely if you could provide them so that Martin could address > technical issues at the same time as he fixes up whitespace nits. No, I'll defer technical comments to Jason. The formatting is just something that caught my eye during the 10 seconds or so spent on reading the patch flying by. Jakub
Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
On Mon, Mar 14, 2016 at 03:25:07PM -0600, Martin Sebor wrote: > PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end > of array fails inside constant expression > PR c++/70170 - [6 regression] bogus not a constant expression error comparing > pointer to array to null > PR c++/70172 - incorrect reinterpret_cast from integer to pointer error > on invalid constexpr initialization > PR c++/60760 - arithmetic on null pointers should not be allowed in constant > expressions > PR c++/70228 - insufficient detail in diagnostics for a constexpr out of > bounds > array subscript Can you please check up the formatting in the patch? Seems e.g. you've replaced tons of tabs with 8 spaces etc. (check your editor setting, and check the patch with contrib/check-GNU-style.sh). There is some trailing whitespace too, spaces before [, etc. Jakub
[PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression
The attached patch fixes the outstanding cases mentioned in comment 10 on bug c++/67376. While testing the fix I uncovered a number of other related problems without which the test would have been incomplete. They include: PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/60760 - arithmetic on null pointers should not be allowed in constant expressions In addition, I include a fix for the issue below that I also came across while testing the patch and that makes root causing constexpr problems due to out-of-bounds array subscripts easier: PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript In a discussion of bug 70170 between those CC'd Marek posted a prototype patch for match.pd. While the patch seems to do the right thing as far as the bug goes, like my own first attempt at a fix in const-fold.c it caused a couple of regressions (in pr21294.c and in pr44555.c). Since I'm not yet familiar enough with match.pd, in the interest of time I solved those regressions in const-fold.c rather than in match.pd. Tested on x86_64. Martin PR c++/67376 - [5/6 regression] Comparison with pointer to past-the-end of array fails inside constant expression PR c++/70170 - [6 regression] bogus not a constant expression error comparing pointer to array to null PR c++/70172 - incorrect reinterpret_cast from integer to pointer error on invalid constexpr initialization PR c++/60760 - arithmetic on null pointers should not be allowed in constant expressions PR c++/70228 - insufficient detail in diagnostics for a constexpr out of bounds array subscript gcc/testsuite/ChangeLog: 2016-03-14 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/60760 PR c++/70228 * g++.dg/cpp0x/constexpr-array-ptr10.C: New test. * g++.dg/cpp0x/constexpr-array-ptr11.C: New test. * g++.dg/cpp0x/constexpr-array-ptr9.C: New test. * g++.dg/cpp0x/constexpr-array5.C: Adjust text of expected diagnostic. * g++.dg/cpp0x/constexpr-string.C: Same. * g++.dg/cpp0x/constexpr-wstring2.C: Same. * g++.dg/cpp0x/pr65398.C: Same. * g++.dg/ext/constexpr-vla1.C: Same. * g++.dg/ext/constexpr-vla2.C: Same. * g++.dg/ext/constexpr-vla3.C: Same. * g++.dg/ubsan/pr63956.C: Same. gcc/cp/ChangeLog: 2016-03-14 Martin Sebor PR c++/67376 PR c++/70170 PR c++/70172 PR c++/60760 PR c++/70228 (cxx_eval_binary_expression): Add argument. (cxx_eval_component_reference): Same. (cxx_eval_constant_expression): Same. (cxx_eval_indirect_ref): Same. (cxx_eval_outermost_constant_expr): Same. (diag_array_subscript): New function. * constexpr.c (cxx_eval_call_expression): Adjust. (cxx_eval_conditional_expression): Same. (cxx_eval_array_reference): Detect null pointers. (cxx_eval_statement_list): Adjust. gcc/ChangeLog: 2016-03-14 Martin Sebor PR c++/67376 * fold-const.c (fold_comparison): Fold equality and relational expressions involving null pointers. diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 5f97c9d..5ec5034 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -918,7 +918,8 @@ struct constexpr_ctx { static GTY (()) hash_table *constexpr_call_table; static tree cxx_eval_constant_expression (const constexpr_ctx *, tree, - bool, bool *, bool *, tree * = NULL); + bool, bool *, bool *, bool * = NULL, + tree * = NULL); /* Compute a hash value for a constexpr call representation. */ @@ -1390,7 +1391,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, tree jump_target = NULL_TREE; cxx_eval_constant_expression (ctx, body, lval, non_constant_p, overflow_p, - &jump_target); +NULL, &jump_target); if (DECL_CONSTRUCTOR_P (fun)) /* This can be null for a subobject constructor call, in @@ -1607,20 +1608,21 @@ cxx_eval_unary_expression (const constexpr_ctx *ctx, tree t, static tree cxx_eval_binary_expression (const constexpr_ctx *ctx, tree t, bool /*lval*/, - bool *non_constant_p, bool *overflow_p) + bool *non_constant_p, bool *overflow_p, +bool *nullptr_p) { tree r = NULL_TREE; tree orig_lhs = TREE_OPERAND (t, 0); tree orig_rhs = TREE_OPERAND (t, 1); tree lhs, rhs; lhs = cxx_eval_constant_expression (ctx, orig_lhs, /*lval*/false, - non_constant_p, overflow_p); + non_constant_p, overflow_p, nullptr_p); /* Don't VERIFY_CONSTANT here, it's unnecessary and will break pointer subtraction. */ if (*non_constant_p) return t; rhs = cxx_eval_constant_expression (ctx, orig_rhs, /*lval*/false, - non_constant_p, overflow_p); + non_constant_p, overflow_p, nullptr_p); if (*non_constant_p) return t; @@ -1642,6 +1644,15 @@ cxx_eval_binary_expre