| Message ID | 20150313144052.GU26802@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 13, 2015 at 3:40 PM, Marek Polacek <polacek@redhat.com> wrote: > We started to reject this (IMHO valid) testcase with r214941 that did away with > try_move_mult_to_index -- meaning that we are no longer able to fold *(&s[0] + 1) > into s[1], while we are able to fold *(s + 1) into s[1]. > > I suppose cxx_fold_indirect_ref ought to be able to handle both cases, so I added > some code to that effect, it should handle now at least the simple cases... > Or should that be handled in the middle end? It's "correct" for constexpr folding but not correct to hand s[1] down to the middle-end IL (both cases). Well, in the particular case with in-array-bound constant and a non-pointer base it's good enough at least. Richard. > I have also tried: > > constexpr char s[] = "abc"; > constexpr int i = 0; > constexpr char c = *(&s[0] + i); > > and that is accepted even without this patch. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2015-03-13 Marek Polacek <polacek@redhat.com> > > PR c++/65398 > * constexpr.c (cxx_fold_indirect_ref): Transform *(&A[i] p+ j) into > A[i + j]. > > * g++.dg/cpp0x/pr65398.C: New test. > > diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c > index 1b5f50c..18f4d8c 100644 > --- gcc/cp/constexpr.c > +++ gcc/cp/constexpr.c > @@ -2427,6 +2427,17 @@ cxx_fold_indirect_ref (location_t loc, tree type, tree op0, bool *empty_base) > break; > } > } > + /* *(&A[i] p+ j) => A[i + j] */ > + else if (TREE_CODE (op00) == ARRAY_REF > + && TREE_CODE (TREE_OPERAND (op00, 1)) == INTEGER_CST > + && TREE_CODE (op01) == INTEGER_CST) > + { > + tree t = fold_convert_loc (loc, TREE_TYPE (op01), > + TREE_OPERAND (op00, 1)); > + t = size_binop_loc (loc, PLUS_EXPR, op01, t); > + return build4_loc (loc, ARRAY_REF, type, TREE_OPERAND (op00, 0), > + t, NULL_TREE, NULL_TREE); > + } > } > } > /* *(foo *)fooarrptr => (*fooarrptr)[0] */ > diff --git gcc/testsuite/g++.dg/cpp0x/pr65398.C gcc/testsuite/g++.dg/cpp0x/pr65398.C > index e69de29..dc22d27 100644 > --- gcc/testsuite/g++.dg/cpp0x/pr65398.C > +++ gcc/testsuite/g++.dg/cpp0x/pr65398.C > @@ -0,0 +1,19 @@ > +// PR c++/65398 > +// { dg-do compile { target c++11 } } > + > +#define SA(X) static_assert((X),#X) > + > +constexpr char s[] = "abc"; > +constexpr char c1 = *(&s[0] + 0); > +constexpr char c2 = *(&s[0] + 1); > +constexpr char c3 = *(&s[1] + 0); > +constexpr char c4 = *(&s[1] + 1); > +constexpr char c5 = *(&s[2] + 0); > +constexpr char c6 = *(&s[0] + 2); > + > +SA (c1 == 'a'); > +SA (c2 == 'b'); > +SA (c3 == 'b'); > +SA (c4 == 'c'); > +SA (c5 == 'c'); > +SA (c6 == 'c'); > > Marek
On Wed, Mar 18, 2015 at 11:08:15AM +0100, Richard Biener wrote: > On Fri, Mar 13, 2015 at 3:40 PM, Marek Polacek <polacek@redhat.com> wrote: > > We started to reject this (IMHO valid) testcase with r214941 that did away with > > try_move_mult_to_index -- meaning that we are no longer able to fold *(&s[0] + 1) > > into s[1], while we are able to fold *(s + 1) into s[1]. > > > > I suppose cxx_fold_indirect_ref ought to be able to handle both cases, so I added > > some code to that effect, it should handle now at least the simple cases... > > Or should that be handled in the middle end? > > It's "correct" for constexpr folding but not correct to hand s[1] down to > the middle-end IL (both cases). Well, in the particular case with > in-array-bound constant and a non-pointer base it's good enough at > least. I believe cxx_fold_indirect_ref result is not passed through to the middle-end, unless it can be folded into a constant. Though, a question is if we do (or, if we don't and should) reject say constexpr char s[] = "abc"; constexpr int j = 4; constexpr char c = *(&s[j] - 2); because there was out of bound access in there. Jakub
On Thu, Mar 19, 2015 at 07:05:36PM +0100, Jakub Jelinek wrote: > On Wed, Mar 18, 2015 at 11:08:15AM +0100, Richard Biener wrote: > > On Fri, Mar 13, 2015 at 3:40 PM, Marek Polacek <polacek@redhat.com> wrote: > > > We started to reject this (IMHO valid) testcase with r214941 that did away with > > > try_move_mult_to_index -- meaning that we are no longer able to fold *(&s[0] + 1) > > > into s[1], while we are able to fold *(s + 1) into s[1]. > > > > > > I suppose cxx_fold_indirect_ref ought to be able to handle both cases, so I added > > > some code to that effect, it should handle now at least the simple cases... > > > Or should that be handled in the middle end? > > > > It's "correct" for constexpr folding but not correct to hand s[1] down to > > the middle-end IL (both cases). Well, in the particular case with > > in-array-bound constant and a non-pointer base it's good enough at > > least. > > I believe cxx_fold_indirect_ref result is not passed through to the > middle-end, unless it can be folded into a constant. > > Though, a question is if we do (or, if we don't and should) reject say > constexpr char s[] = "abc"; > constexpr int j = 4; > constexpr char c = *(&s[j] - 2); > because there was out of bound access in there. That is rejected even with my patch with: error: overflow in constant expression [-fpermissive] and without the patch: error: ‘*((& s[4]) + 18446744073709551614u)’ is not a constant expression (a valid constexpr can't have UB). Marek
On Thu, Mar 19, 2015 at 07:13:47PM +0100, Marek Polacek wrote: > On Thu, Mar 19, 2015 at 07:05:36PM +0100, Jakub Jelinek wrote: > > I believe cxx_fold_indirect_ref result is not passed through to the > > middle-end, unless it can be folded into a constant. > > > > Though, a question is if we do (or, if we don't and should) reject say > > constexpr char s[] = "abc"; > > constexpr int j = 4; > > constexpr char c = *(&s[j] - 2); > > because there was out of bound access in there. > > That is rejected even with my patch with: > error: overflow in constant expression [-fpermissive] > and without the patch: > error: ‘*((& s[4]) + 18446744073709551614u)’ is not a constant expression > (a valid constexpr can't have UB). But s/j = 4/j = 3/ should be valid, don't we reject even that? I mean, isn't the rejection because we fold the - 2 early into sizetype (unsigned) + -2UL? Jakub
On Thu, Mar 19, 2015 at 07:17:23PM +0100, Jakub Jelinek wrote: > On Thu, Mar 19, 2015 at 07:13:47PM +0100, Marek Polacek wrote: > > On Thu, Mar 19, 2015 at 07:05:36PM +0100, Jakub Jelinek wrote: > > > I believe cxx_fold_indirect_ref result is not passed through to the > > > middle-end, unless it can be folded into a constant. > > > > > > Though, a question is if we do (or, if we don't and should) reject say > > > constexpr char s[] = "abc"; > > > constexpr int j = 4; > > > constexpr char c = *(&s[j] - 2); > > > because there was out of bound access in there. > > > > That is rejected even with my patch with: > > error: overflow in constant expression [-fpermissive] > > and without the patch: > > error: ‘*((& s[4]) + 18446744073709551614u)’ is not a constant expression > > (a valid constexpr can't have UB). > > But s/j = 4/j = 3/ should be valid, don't we reject even that? > I mean, isn't the rejection because we fold the - 2 early into sizetype > (unsigned) + -2UL? Unfortunately we reject even that (regardless the patch), and yeah, it's because of how POINTER_PLUS_EXPR uses sizetype as a type of the second operand. E.g. constexpr char s[] = "abc"; constexpr int j = 1; constexpr char c = *(&s[j] + 3); is correctly rejected with the patch: error: array subscript out of bound Marek
On 03/19/2015 02:05 PM, Jakub Jelinek wrote: > Though, a question is if we do (or, if we don't and should) reject say > constexpr char s[] = "abc"; > constexpr int j = 4; > constexpr char c = *(&s[j] - 2); > because there was out of bound access in there. I don't see an out-of-bound access in this example; taking the address of one-past-the-end is OK as long as you don't try to access through it. > Unfortunately we reject even that (regardless the patch), and yeah, it's > because of how POINTER_PLUS_EXPR uses sizetype as a type of the second operand. This seems like something to fix in this patch. > + tree t = fold_convert_loc (loc, TREE_TYPE (op01), > + TREE_OPERAND (op00, 1)); > + t = size_binop_loc (loc, PLUS_EXPR, op01, t); This seems to be assuming that the elements are size 1. Jason
On Fri, Mar 20, 2015 at 10:53:50AM -0400, Jason Merrill wrote: > On 03/19/2015 02:05 PM, Jakub Jelinek wrote: > >Though, a question is if we do (or, if we don't and should) reject say > >constexpr char s[] = "abc"; > >constexpr int j = 4; > >constexpr char c = *(&s[j] - 2); > >because there was out of bound access in there. > > I don't see an out-of-bound access in this example; taking the address of > one-past-the-end is OK as long as you don't try to access through it. It is taking address of two past the end though - &s[3] is fine, sure. But &s[4] is invalid already. > >Unfortunately we reject even that (regardless the patch), and yeah, it's > >because of how POINTER_PLUS_EXPR uses sizetype as a type of the second operand. > > This seems like something to fix in this patch. > > >+ tree t = fold_convert_loc (loc, TREE_TYPE (op01), > >+ TREE_OPERAND (op00, 1)); > >+ t = size_binop_loc (loc, PLUS_EXPR, op01, t); > > This seems to be assuming that the elements are size 1. Jakub
On 03/20/2015 10:59 AM, Jakub Jelinek wrote: > On Fri, Mar 20, 2015 at 10:53:50AM -0400, Jason Merrill wrote: >> On 03/19/2015 02:05 PM, Jakub Jelinek wrote: >>> Though, a question is if we do (or, if we don't and should) reject say >>> constexpr char s[] = "abc"; >>> constexpr int j = 4; >>> constexpr char c = *(&s[j] - 2); >>> because there was out of bound access in there. >> >> I don't see an out-of-bound access in this example; taking the address of >> one-past-the-end is OK as long as you don't try to access through it. > > It is taking address of two past the end though - &s[3] is fine, sure. > But &s[4] is invalid already. &s[3] is the address of the terminal \0. Jason
On Fri, Mar 20, 2015 at 11:02:59AM -0400, Jason Merrill wrote: > On 03/20/2015 10:59 AM, Jakub Jelinek wrote: > >On Fri, Mar 20, 2015 at 10:53:50AM -0400, Jason Merrill wrote: > >>On 03/19/2015 02:05 PM, Jakub Jelinek wrote: > >>>Though, a question is if we do (or, if we don't and should) reject say > >>>constexpr char s[] = "abc"; > >>>constexpr int j = 4; > >>>constexpr char c = *(&s[j] - 2); > >>>because there was out of bound access in there. > >> > >>I don't see an out-of-bound access in this example; taking the address of > >>one-past-the-end is OK as long as you don't try to access through it. > > > >It is taking address of two past the end though - &s[3] is fine, sure. > >But &s[4] is invalid already. > > &s[3] is the address of the terminal \0. Yeah, sure. But the above testcase does &s[4], which is out of bounds arithmetics, and then subtracts 2 to point it back into range. I'm not saying it is absolutely necessary to handle this for GCC 5, but we at least should not treat the POINTER_PLUS_EXPR offset in the constexpr handling as unsigned, but think of it as signed, otherwise we reject even valid code - say constexpr char d = *(&s[3] - 1). Jakub
On 03/20/2015 11:11 AM, Jakub Jelinek wrote: >> &s[3] is the address of the terminal \0. > > Yeah, sure. But the above testcase does &s[4], which is out of bounds > arithmetics But since &s[3] is the address of an element of the array (the NUL), &s[4] is one-past-the-end, which is fine. &s[5] would be out of bounds. > I'm not saying it is absolutely necessary to handle this for GCC 5, but > we at least should not treat the POINTER_PLUS_EXPR offset in the constexpr > handling as unsigned, but think of it as signed, otherwise we reject even > valid code - say constexpr char d = *(&s[3] - 1). Agreed. And I still think it makes sense to fix this as part of this patch. Jason
On Fri, Mar 20, 2015 at 11:15:30AM -0400, Jason Merrill wrote: > On 03/20/2015 11:11 AM, Jakub Jelinek wrote: > >>&s[3] is the address of the terminal \0. > > > >Yeah, sure. But the above testcase does &s[4], which is out of bounds > >arithmetics > > But since &s[3] is the address of an element of the array (the NUL), &s[4] > is one-past-the-end, which is fine. &s[5] would be out of bounds. Ashamed... Obviously I meant if we should diagnose *(&s[5] - 2). clang++ does. > Agreed. And I still think it makes sense to fix this as part of this patch. Yes. Jakub
diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c index 1b5f50c..18f4d8c 100644 --- gcc/cp/constexpr.c +++ gcc/cp/constexpr.c @@ -2427,6 +2427,17 @@ cxx_fold_indirect_ref (location_t loc, tree type, tree op0, bool *empty_base) break; } } + /* *(&A[i] p+ j) => A[i + j] */ + else if (TREE_CODE (op00) == ARRAY_REF + && TREE_CODE (TREE_OPERAND (op00, 1)) == INTEGER_CST + && TREE_CODE (op01) == INTEGER_CST) + { + tree t = fold_convert_loc (loc, TREE_TYPE (op01), + TREE_OPERAND (op00, 1)); + t = size_binop_loc (loc, PLUS_EXPR, op01, t); + return build4_loc (loc, ARRAY_REF, type, TREE_OPERAND (op00, 0), + t, NULL_TREE, NULL_TREE); + } } } /* *(foo *)fooarrptr => (*fooarrptr)[0] */ diff --git gcc/testsuite/g++.dg/cpp0x/pr65398.C gcc/testsuite/g++.dg/cpp0x/pr65398.C index e69de29..dc22d27 100644 --- gcc/testsuite/g++.dg/cpp0x/pr65398.C +++ gcc/testsuite/g++.dg/cpp0x/pr65398.C @@ -0,0 +1,19 @@ +// PR c++/65398 +// { dg-do compile { target c++11 } } + +#define SA(X) static_assert((X),#X) + +constexpr char s[] = "abc"; +constexpr char c1 = *(&s[0] + 0); +constexpr char c2 = *(&s[0] + 1); +constexpr char c3 = *(&s[1] + 0); +constexpr char c4 = *(&s[1] + 1); +constexpr char c5 = *(&s[2] + 0); +constexpr char c6 = *(&s[0] + 2); + +SA (c1 == 'a'); +SA (c2 == 'b'); +SA (c3 == 'b'); +SA (c4 == 'c'); +SA (c5 == 'c'); +SA (c6 == 'c');