| Message ID | 50c22816-dd24-4ceb-6620-04c168642427@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | reject scalar array initialization with nullptr [PR94510] | expand |
On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via Gcc-patches wrote: > Among the numerous regressions introduced by the change committed > to GCC 9 to allow string literals as template arguments is a failure > to recognize the C++ nullptr and GCC's __null constants as pointers. > For one, I didn't realize that nullptr, being a null pointer constant, > doesn't have a pointer type, and two, I didn't think of __null (which > is a special integer constant that NULL sometimes expands to). > > The attached patch adjusts the special handling of trailing zero > initializers in reshape_init_array_1 to recognize both kinds of > constants and avoid treating them as zeros of the array integer > element type. This restores the expected diagnostics when either > constant is used in the initializer list. > > Martin > PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array > > gcc/cp/ChangeLog: > > PR c++/94510 > * decl.c (reshape_init_array_1): Exclude mismatches with all kinds > of pointers. > > gcc/testsuite/ChangeLog: > > PR c++/94510 > * g++.dg/init/array57.C: New test. > * g++.dg/init/array58.C: New test. > > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c > index a127734af69..692c8ed73f4 100644 > --- a/gcc/cp/decl.c > +++ b/gcc/cp/decl.c > @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d, > TREE_CONSTANT (new_init) = false; > > /* Pointers initialized to strings must be treated as non-zero > - even if the string is empty. */ > + even if the string is empty. Handle all kinds of pointers, > + including std::nullptr and GCC's __nullptr, neither of which > + has a pointer type. */ > tree init_type = TREE_TYPE (elt_init); > - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) > + bool init_is_ptr = (POINTER_TYPE_P (init_type) > + || NULLPTR_TYPE_P (init_type) > + || null_node_p (elt_init)); > + if (POINTER_TYPE_P (elt_type) != init_is_ptr > || !type_initializer_zero_p (elt_type, elt_init)) > last_nonzero = index; It looks like this still won't handle e.g. pointers to member functions, e.g. struct S { }; int arr[3] = { (void (S::*) ()) 0, 0, 0 }; would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P instead of POINTER_TYPE_P to catch this case. Marek
On 4/7/20 1:50 PM, Marek Polacek wrote: > On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via Gcc-patches wrote: >> Among the numerous regressions introduced by the change committed >> to GCC 9 to allow string literals as template arguments is a failure >> to recognize the C++ nullptr and GCC's __null constants as pointers. >> For one, I didn't realize that nullptr, being a null pointer constant, >> doesn't have a pointer type, and two, I didn't think of __null (which >> is a special integer constant that NULL sometimes expands to). >> >> The attached patch adjusts the special handling of trailing zero >> initializers in reshape_init_array_1 to recognize both kinds of >> constants and avoid treating them as zeros of the array integer >> element type. This restores the expected diagnostics when either >> constant is used in the initializer list. >> >> Martin > >> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >> >> gcc/cp/ChangeLog: >> >> PR c++/94510 >> * decl.c (reshape_init_array_1): Exclude mismatches with all kinds >> of pointers. >> >> gcc/testsuite/ChangeLog: >> >> PR c++/94510 >> * g++.dg/init/array57.C: New test. >> * g++.dg/init/array58.C: New test. >> >> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >> index a127734af69..692c8ed73f4 100644 >> --- a/gcc/cp/decl.c >> +++ b/gcc/cp/decl.c >> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d, >> TREE_CONSTANT (new_init) = false; >> >> /* Pointers initialized to strings must be treated as non-zero >> - even if the string is empty. */ >> + even if the string is empty. Handle all kinds of pointers, >> + including std::nullptr and GCC's __nullptr, neither of which >> + has a pointer type. */ >> tree init_type = TREE_TYPE (elt_init); >> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >> + || NULLPTR_TYPE_P (init_type) >> + || null_node_p (elt_init)); >> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >> || !type_initializer_zero_p (elt_type, elt_init)) >> last_nonzero = index; > > It looks like this still won't handle e.g. pointers to member functions, > e.g. > > struct S { }; > int arr[3] = { (void (S::*) ()) 0, 0, 0 }; > > would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P instead of > POINTER_TYPE_P to catch this case. Good catch! That doesn't fail because unlike null data member pointers which are represented as -1, member function pointers are represented as a zero. I had looked for an API that would answer the question: "is this expression a pointer?" without having to think of all the different kinds of them but all I could find was null_node_p(). Is this a rare, isolated case that having an API like that wouldn't be worth having or should I add one like in the attached update? Martin
On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: > On 4/7/20 1:50 PM, Marek Polacek wrote: > > On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via Gcc-patches wrote: > > > Among the numerous regressions introduced by the change committed > > > to GCC 9 to allow string literals as template arguments is a failure > > > to recognize the C++ nullptr and GCC's __null constants as pointers. > > > For one, I didn't realize that nullptr, being a null pointer constant, > > > doesn't have a pointer type, and two, I didn't think of __null (which > > > is a special integer constant that NULL sometimes expands to). > > > > > > The attached patch adjusts the special handling of trailing zero > > > initializers in reshape_init_array_1 to recognize both kinds of > > > constants and avoid treating them as zeros of the array integer > > > element type. This restores the expected diagnostics when either > > > constant is used in the initializer list. > > > > > > Martin > > > > > PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array > > > > > > gcc/cp/ChangeLog: > > > > > > PR c++/94510 > > > * decl.c (reshape_init_array_1): Exclude mismatches with all kinds > > > of pointers. > > > > > > gcc/testsuite/ChangeLog: > > > > > > PR c++/94510 > > > * g++.dg/init/array57.C: New test. > > > * g++.dg/init/array58.C: New test. > > > > > > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c > > > index a127734af69..692c8ed73f4 100644 > > > --- a/gcc/cp/decl.c > > > +++ b/gcc/cp/decl.c > > > @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d, > > > TREE_CONSTANT (new_init) = false; > > > /* Pointers initialized to strings must be treated as non-zero > > > - even if the string is empty. */ > > > + even if the string is empty. Handle all kinds of pointers, > > > + including std::nullptr and GCC's __nullptr, neither of which > > > + has a pointer type. */ > > > tree init_type = TREE_TYPE (elt_init); > > > - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) > > > + bool init_is_ptr = (POINTER_TYPE_P (init_type) > > > + || NULLPTR_TYPE_P (init_type) > > > + || null_node_p (elt_init)); > > > + if (POINTER_TYPE_P (elt_type) != init_is_ptr > > > || !type_initializer_zero_p (elt_type, elt_init)) > > > last_nonzero = index; > > > > It looks like this still won't handle e.g. pointers to member functions, > > e.g. > > > > struct S { }; > > int arr[3] = { (void (S::*) ()) 0, 0, 0 }; > > > > would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P instead of > > POINTER_TYPE_P to catch this case. > > Good catch! That doesn't fail because unlike null data member pointers > which are represented as -1, member function pointers are represented > as a zero. > > I had looked for an API that would answer the question: "is this > expression a pointer?" without having to think of all the different > kinds of them but all I could find was null_node_p(). Is this a rare, > isolated case that having an API like that wouldn't be worth having > or should I add one like in the attached update? > > Martin > PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array > > gcc/cp/ChangeLog: > > PR c++/94510 > * decl.c (reshape_init_array_1): Exclude mismatches with all kinds > of pointers. > * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. (Drop the gcc/cp/.) > +/* Returns true if EXPR is a null pointer constant of any type. */ > + > +inline bool > +null_pointer_constant_p (tree expr) > +{ > + STRIP_ANY_LOCATION_WRAPPER (expr); > + if (expr == null_node) > + return true; > + tree type = TREE_TYPE (expr); > + if (NULLPTR_TYPE_P (type)) > + return true; > + if (POINTER_TYPE_P (type)) > + return integer_zerop (expr); > + return null_member_pointer_value_p (expr); > +} > + We already have a null_ptr_cst_p so it would be sort of confusing to have this as well. But are you really interested in whether it's a null pointer, not just a pointer? Marek
On 4/7/20 3:36 PM, Marek Polacek wrote: > On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >> On 4/7/20 1:50 PM, Marek Polacek wrote: >>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via Gcc-patches wrote: >>>> Among the numerous regressions introduced by the change committed >>>> to GCC 9 to allow string literals as template arguments is a failure >>>> to recognize the C++ nullptr and GCC's __null constants as pointers. >>>> For one, I didn't realize that nullptr, being a null pointer constant, >>>> doesn't have a pointer type, and two, I didn't think of __null (which >>>> is a special integer constant that NULL sometimes expands to). >>>> >>>> The attached patch adjusts the special handling of trailing zero >>>> initializers in reshape_init_array_1 to recognize both kinds of >>>> constants and avoid treating them as zeros of the array integer >>>> element type. This restores the expected diagnostics when either >>>> constant is used in the initializer list. >>>> >>>> Martin >>> >>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >>>> >>>> gcc/cp/ChangeLog: >>>> >>>> PR c++/94510 >>>> * decl.c (reshape_init_array_1): Exclude mismatches with all kinds >>>> of pointers. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> PR c++/94510 >>>> * g++.dg/init/array57.C: New test. >>>> * g++.dg/init/array58.C: New test. >>>> >>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>> index a127734af69..692c8ed73f4 100644 >>>> --- a/gcc/cp/decl.c >>>> +++ b/gcc/cp/decl.c >>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d, >>>> TREE_CONSTANT (new_init) = false; >>>> /* Pointers initialized to strings must be treated as non-zero >>>> - even if the string is empty. */ >>>> + even if the string is empty. Handle all kinds of pointers, >>>> + including std::nullptr and GCC's __nullptr, neither of which >>>> + has a pointer type. */ >>>> tree init_type = TREE_TYPE (elt_init); >>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>> + || NULLPTR_TYPE_P (init_type) >>>> + || null_node_p (elt_init)); >>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>> last_nonzero = index; >>> >>> It looks like this still won't handle e.g. pointers to member functions, >>> e.g. >>> >>> struct S { }; >>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>> >>> would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P instead of >>> POINTER_TYPE_P to catch this case. >> >> Good catch! That doesn't fail because unlike null data member pointers >> which are represented as -1, member function pointers are represented >> as a zero. >> >> I had looked for an API that would answer the question: "is this >> expression a pointer?" without having to think of all the different >> kinds of them but all I could find was null_node_p(). Is this a rare, >> isolated case that having an API like that wouldn't be worth having >> or should I add one like in the attached update? >> >> Martin > >> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >> >> gcc/cp/ChangeLog: >> >> PR c++/94510 >> * decl.c (reshape_init_array_1): Exclude mismatches with all kinds >> of pointers. >> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. > > (Drop the gcc/cp/.) > >> +/* Returns true if EXPR is a null pointer constant of any type. */ >> + >> +inline bool >> +null_pointer_constant_p (tree expr) >> +{ >> + STRIP_ANY_LOCATION_WRAPPER (expr); >> + if (expr == null_node) >> + return true; >> + tree type = TREE_TYPE (expr); >> + if (NULLPTR_TYPE_P (type)) >> + return true; >> + if (POINTER_TYPE_P (type)) >> + return integer_zerop (expr); >> + return null_member_pointer_value_p (expr); >> +} >> + > > We already have a null_ptr_cst_p so it would be sort of confusing to have > this as well. But are you really interested in whether it's a null pointer, > not just a pointer? The goal of the code is to detect a mismatch in "pointerness" between an initializer expression and the type of the initialized element, so it needs to know if the expression is a pointer (non-nulls pointers are detected in type_initializer_zero_p). That means testing a number of IMO unintuitive conditions: TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) || NULLPTR_TYPE_P (TREE_TYPE (expr)) || null_node_p (expr) I don't know if this type of a query is common in the C++ FE but unless this is an isolated use case then besides fixing the bug I thought it would be nice to make it easier to get the test above right, or at least come close to it. Since null_pointer_constant_p already exists (but isn't suitable here because it returns true for plain literal zeros) would a function like /* Returns true if EXPR is a pointer of any type, including nullptr and __null. */ inline bool pointer_p (tree expr) { STRIP_ANY_LOCATION_WRAPPER (expr); if (expr == null_node) return true; tree type = TREE_TYPE (expr); if (NULLPTR_TYPE_P (type)) return true; return TYPE_PTR_OR_PTRMEM_P (type); } be useful? If not, I can just open-code it using TYPE_PTR_OR_PTRMEM_P as you suggested. Martin
On 4/7/20 2:50 PM, Martin Sebor wrote: > Among the numerous regressions introduced by the change committed > to GCC 9 to allow string literals as template arguments is a failure > to recognize the C++ nullptr and GCC's __null constants as pointers. > For one, I didn't realize that nullptr, being a null pointer constant, > doesn't have a pointer type, and two, I didn't think of __null (which > is a special integer constant that NULL sometimes expands to). > > The attached patch adjusts the special handling of trailing zero > initializers in reshape_init_array_1 to recognize both kinds of > constants and avoid treating them as zeros of the array integer > element type. This restores the expected diagnostics when either > constant is used in the initializer list. This is another problem due to doing this checking too early, as with 90938. Let's look at your other patch from the 90938 discussion to move the zero pruning to process_init_constructor_array. Jason
On 4/8/20 12:43 PM, Jason Merrill wrote: > On 4/7/20 2:50 PM, Martin Sebor wrote: >> Among the numerous regressions introduced by the change committed >> to GCC 9 to allow string literals as template arguments is a failure >> to recognize the C++ nullptr and GCC's __null constants as pointers. >> For one, I didn't realize that nullptr, being a null pointer constant, >> doesn't have a pointer type, and two, I didn't think of __null (which >> is a special integer constant that NULL sometimes expands to). >> >> The attached patch adjusts the special handling of trailing zero >> initializers in reshape_init_array_1 to recognize both kinds of >> constants and avoid treating them as zeros of the array integer >> element type. This restores the expected diagnostics when either >> constant is used in the initializer list. > > This is another problem due to doing this checking too early, as with > 90938. Let's look at your other patch from the 90938 discussion to move > the zero pruning to process_init_constructor_array. The patch for pr90938 handles this correctly, and I'm planning to resubmit it in stage 1 (as I thought ultimately ended up being your expectation as well). A I mentioned in the review at the end of February I had reservations about making those changes then because it seemed late to me. I'm that much less comfortable with them now, barely a month away from the anticipated release date. For reference, the last revision of the patch is here: https://gcc.gnu.org/pipermail/gcc-patches/2020-February/540792.html Martin PS As a reminder, all these bugs (94510, 94124, 90947, and 90938) were introduced by making a similar change to code I wasn't familiar with at the very end of GCC 9.
On 4/8/20 1:23 PM, Martin Sebor wrote: > On 4/7/20 3:36 PM, Marek Polacek wrote: >> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>> Gcc-patches wrote: >>>>> Among the numerous regressions introduced by the change committed >>>>> to GCC 9 to allow string literals as template arguments is a failure >>>>> to recognize the C++ nullptr and GCC's __null constants as pointers. >>>>> For one, I didn't realize that nullptr, being a null pointer constant, >>>>> doesn't have a pointer type, and two, I didn't think of __null (which >>>>> is a special integer constant that NULL sometimes expands to). >>>>> >>>>> The attached patch adjusts the special handling of trailing zero >>>>> initializers in reshape_init_array_1 to recognize both kinds of >>>>> constants and avoid treating them as zeros of the array integer >>>>> element type. This restores the expected diagnostics when either >>>>> constant is used in the initializer list. >>>>> >>>>> Martin >>>> >>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> PR c++/94510 >>>>> * decl.c (reshape_init_array_1): Exclude mismatches with all kinds >>>>> of pointers. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> PR c++/94510 >>>>> * g++.dg/init/array57.C: New test. >>>>> * g++.dg/init/array58.C: New test. >>>>> >>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>> index a127734af69..692c8ed73f4 100644 >>>>> --- a/gcc/cp/decl.c >>>>> +++ b/gcc/cp/decl.c >>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree >>>>> max_index, reshape_iter *d, >>>>> TREE_CONSTANT (new_init) = false; >>>>> /* Pointers initialized to strings must be treated as >>>>> non-zero >>>>> - even if the string is empty. */ >>>>> + even if the string is empty. Handle all kinds of pointers, >>>>> + including std::nullptr and GCC's __nullptr, neither of which >>>>> + has a pointer type. */ >>>>> tree init_type = TREE_TYPE (elt_init); >>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>> + || NULLPTR_TYPE_P (init_type) >>>>> + || null_node_p (elt_init)); >>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>> last_nonzero = index; >>>> >>>> It looks like this still won't handle e.g. pointers to member >>>> functions, >>>> e.g. >>>> >>>> struct S { }; >>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>> >>>> would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P instead of >>>> POINTER_TYPE_P to catch this case. >>> >>> Good catch! That doesn't fail because unlike null data member pointers >>> which are represented as -1, member function pointers are represented >>> as a zero. >>> >>> I had looked for an API that would answer the question: "is this >>> expression a pointer?" without having to think of all the different >>> kinds of them but all I could find was null_node_p(). Is this a rare, >>> isolated case that having an API like that wouldn't be worth having >>> or should I add one like in the attached update? >>> >>> Martin >> >>> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >>> >>> gcc/cp/ChangeLog: >>> >>> PR c++/94510 >>> * decl.c (reshape_init_array_1): Exclude mismatches with all kinds >>> of pointers. >>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. >> >> (Drop the gcc/cp/.) >> >>> +/* Returns true if EXPR is a null pointer constant of any type. */ >>> + >>> +inline bool >>> +null_pointer_constant_p (tree expr) >>> +{ >>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>> + if (expr == null_node) >>> + return true; >>> + tree type = TREE_TYPE (expr); >>> + if (NULLPTR_TYPE_P (type)) >>> + return true; >>> + if (POINTER_TYPE_P (type)) >>> + return integer_zerop (expr); >>> + return null_member_pointer_value_p (expr); >>> +} >>> + >> >> We already have a null_ptr_cst_p so it would be sort of confusing to have >> this as well. But are you really interested in whether it's a null >> pointer, >> not just a pointer? > > The goal of the code is to detect a mismatch in "pointerness" between > an initializer expression and the type of the initialized element, so > it needs to know if the expression is a pointer (non-nulls pointers > are detected in type_initializer_zero_p). That means testing a number > of IMO unintuitive conditions: > > TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) > || NULLPTR_TYPE_P (TREE_TYPE (expr)) > || null_node_p (expr) > > I don't know if this type of a query is common in the C++ FE but unless > this is an isolated use case then besides fixing the bug I thought it > would be nice to make it easier to get the test above right, or at least > come close to it. > > Since null_pointer_constant_p already exists (but isn't suitable here > because it returns true for plain literal zeros) Why is that unsuitable? A literal zero is a perfectly good zero-initializer for a pointer. Jason
On 4/9/20 1:03 PM, Jason Merrill wrote: > On 4/8/20 1:23 PM, Martin Sebor wrote: >> On 4/7/20 3:36 PM, Marek Polacek wrote: >>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>>> Gcc-patches wrote: >>>>>> Among the numerous regressions introduced by the change committed >>>>>> to GCC 9 to allow string literals as template arguments is a failure >>>>>> to recognize the C++ nullptr and GCC's __null constants as pointers. >>>>>> For one, I didn't realize that nullptr, being a null pointer >>>>>> constant, >>>>>> doesn't have a pointer type, and two, I didn't think of __null (which >>>>>> is a special integer constant that NULL sometimes expands to). >>>>>> >>>>>> The attached patch adjusts the special handling of trailing zero >>>>>> initializers in reshape_init_array_1 to recognize both kinds of >>>>>> constants and avoid treating them as zeros of the array integer >>>>>> element type. This restores the expected diagnostics when either >>>>>> constant is used in the initializer list. >>>>>> >>>>>> Martin >>>>> >>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >>>>>> >>>>>> gcc/cp/ChangeLog: >>>>>> >>>>>> PR c++/94510 >>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with all >>>>>> kinds >>>>>> of pointers. >>>>>> >>>>>> gcc/testsuite/ChangeLog: >>>>>> >>>>>> PR c++/94510 >>>>>> * g++.dg/init/array57.C: New test. >>>>>> * g++.dg/init/array58.C: New test. >>>>>> >>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>> index a127734af69..692c8ed73f4 100644 >>>>>> --- a/gcc/cp/decl.c >>>>>> +++ b/gcc/cp/decl.c >>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree >>>>>> max_index, reshape_iter *d, >>>>>> TREE_CONSTANT (new_init) = false; >>>>>> /* Pointers initialized to strings must be treated as >>>>>> non-zero >>>>>> - even if the string is empty. */ >>>>>> + even if the string is empty. Handle all kinds of pointers, >>>>>> + including std::nullptr and GCC's __nullptr, neither of which >>>>>> + has a pointer type. */ >>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>> + || null_node_p (elt_init)); >>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>> last_nonzero = index; >>>>> >>>>> It looks like this still won't handle e.g. pointers to member >>>>> functions, >>>>> e.g. >>>>> >>>>> struct S { }; >>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>> >>>>> would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P >>>>> instead of >>>>> POINTER_TYPE_P to catch this case. >>>> >>>> Good catch! That doesn't fail because unlike null data member pointers >>>> which are represented as -1, member function pointers are represented >>>> as a zero. >>>> >>>> I had looked for an API that would answer the question: "is this >>>> expression a pointer?" without having to think of all the different >>>> kinds of them but all I could find was null_node_p(). Is this a rare, >>>> isolated case that having an API like that wouldn't be worth having >>>> or should I add one like in the attached update? >>>> >>>> Martin >>> >>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >>>> >>>> gcc/cp/ChangeLog: >>>> >>>> PR c++/94510 >>>> * decl.c (reshape_init_array_1): Exclude mismatches with all kinds >>>> of pointers. >>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. >>> >>> (Drop the gcc/cp/.) >>> >>>> +/* Returns true if EXPR is a null pointer constant of any type. */ >>>> + >>>> +inline bool >>>> +null_pointer_constant_p (tree expr) >>>> +{ >>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>> + if (expr == null_node) >>>> + return true; >>>> + tree type = TREE_TYPE (expr); >>>> + if (NULLPTR_TYPE_P (type)) >>>> + return true; >>>> + if (POINTER_TYPE_P (type)) >>>> + return integer_zerop (expr); >>>> + return null_member_pointer_value_p (expr); >>>> +} >>>> + >>> >>> We already have a null_ptr_cst_p so it would be sort of confusing to >>> have >>> this as well. But are you really interested in whether it's a null >>> pointer, >>> not just a pointer? >> >> The goal of the code is to detect a mismatch in "pointerness" between >> an initializer expression and the type of the initialized element, so >> it needs to know if the expression is a pointer (non-nulls pointers >> are detected in type_initializer_zero_p). That means testing a number >> of IMO unintuitive conditions: >> >> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >> || null_node_p (expr) >> >> I don't know if this type of a query is common in the C++ FE but unless >> this is an isolated use case then besides fixing the bug I thought it >> would be nice to make it easier to get the test above right, or at least >> come close to it. >> >> Since null_pointer_constant_p already exists (but isn't suitable here >> because it returns true for plain literal zeros) > > Why is that unsuitable? A literal zero is a perfectly good > zero-initializer for a pointer. Right, that's why it's not suitable here. Because a literal zero is also not a pointer. The question the code asks is: "is the initializer expression a pointer (of any kind)?" and I thought that might be common enough to justify adding a helper function for. If it isn't then leaving it open-coded as it is in the updated patch below is fine with me. --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d, TREE_CONSTANT (new_init) = false; /* Pointers initialized to strings must be treated as non-zero - even if the string is empty. */ + even if the string is empty. Handle all kinds of pointers, + including std::nullptr and GCC's __nullptr, neither of which + has a pointer type. */ tree init_type = TREE_TYPE (elt_init); - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) + bool init_is_ptr = (TYPE_PTR_OR_PTRMEM_P (init_type) + || NULLPTR_TYPE_P (init_type) + || null_node_p (elt_init)); + if (POINTER_TYPE_P (elt_type) != init_is_ptr || !type_initializer_zero_p (elt_type, elt_init)) last_nonzero = index; Martin
On 4/9/20 3:24 PM, Martin Sebor wrote: > On 4/9/20 1:03 PM, Jason Merrill wrote: >> On 4/8/20 1:23 PM, Martin Sebor wrote: >>> On 4/7/20 3:36 PM, Marek Polacek wrote: >>>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>>>> Gcc-patches wrote: >>>>>>> Among the numerous regressions introduced by the change committed >>>>>>> to GCC 9 to allow string literals as template arguments is a failure >>>>>>> to recognize the C++ nullptr and GCC's __null constants as pointers. >>>>>>> For one, I didn't realize that nullptr, being a null pointer >>>>>>> constant, >>>>>>> doesn't have a pointer type, and two, I didn't think of __null >>>>>>> (which >>>>>>> is a special integer constant that NULL sometimes expands to). >>>>>>> >>>>>>> The attached patch adjusts the special handling of trailing zero >>>>>>> initializers in reshape_init_array_1 to recognize both kinds of >>>>>>> constants and avoid treating them as zeros of the array integer >>>>>>> element type. This restores the expected diagnostics when either >>>>>>> constant is used in the initializer list. >>>>>>> >>>>>>> Martin >>>>>> >>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >>>>>>> >>>>>>> gcc/cp/ChangeLog: >>>>>>> >>>>>>> PR c++/94510 >>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with all >>>>>>> kinds >>>>>>> of pointers. >>>>>>> >>>>>>> gcc/testsuite/ChangeLog: >>>>>>> >>>>>>> PR c++/94510 >>>>>>> * g++.dg/init/array57.C: New test. >>>>>>> * g++.dg/init/array58.C: New test. >>>>>>> >>>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>>> index a127734af69..692c8ed73f4 100644 >>>>>>> --- a/gcc/cp/decl.c >>>>>>> +++ b/gcc/cp/decl.c >>>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree >>>>>>> max_index, reshape_iter *d, >>>>>>> TREE_CONSTANT (new_init) = false; >>>>>>> /* Pointers initialized to strings must be treated as >>>>>>> non-zero >>>>>>> - even if the string is empty. */ >>>>>>> + even if the string is empty. Handle all kinds of pointers, >>>>>>> + including std::nullptr and GCC's __nullptr, neither of which >>>>>>> + has a pointer type. */ >>>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>>> + || null_node_p (elt_init)); >>>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>>> last_nonzero = index; >>>>>> >>>>>> It looks like this still won't handle e.g. pointers to member >>>>>> functions, >>>>>> e.g. >>>>>> >>>>>> struct S { }; >>>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>>> >>>>>> would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P >>>>>> instead of >>>>>> POINTER_TYPE_P to catch this case. >>>>> >>>>> Good catch! That doesn't fail because unlike null data member >>>>> pointers >>>>> which are represented as -1, member function pointers are represented >>>>> as a zero. >>>>> >>>>> I had looked for an API that would answer the question: "is this >>>>> expression a pointer?" without having to think of all the different >>>>> kinds of them but all I could find was null_node_p(). Is this a rare, >>>>> isolated case that having an API like that wouldn't be worth having >>>>> or should I add one like in the attached update? >>>>> >>>>> Martin >>>> >>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> PR c++/94510 >>>>> * decl.c (reshape_init_array_1): Exclude mismatches with all kinds >>>>> of pointers. >>>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. >>>> >>>> (Drop the gcc/cp/.) >>>> >>>>> +/* Returns true if EXPR is a null pointer constant of any type. */ >>>>> + >>>>> +inline bool >>>>> +null_pointer_constant_p (tree expr) >>>>> +{ >>>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>>> + if (expr == null_node) >>>>> + return true; >>>>> + tree type = TREE_TYPE (expr); >>>>> + if (NULLPTR_TYPE_P (type)) >>>>> + return true; >>>>> + if (POINTER_TYPE_P (type)) >>>>> + return integer_zerop (expr); >>>>> + return null_member_pointer_value_p (expr); >>>>> +} >>>>> + >>>> >>>> We already have a null_ptr_cst_p so it would be sort of confusing to >>>> have >>>> this as well. But are you really interested in whether it's a null >>>> pointer, >>>> not just a pointer? >>> >>> The goal of the code is to detect a mismatch in "pointerness" between >>> an initializer expression and the type of the initialized element, so >>> it needs to know if the expression is a pointer (non-nulls pointers >>> are detected in type_initializer_zero_p). That means testing a number >>> of IMO unintuitive conditions: >>> >>> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >>> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >>> || null_node_p (expr) >>> >>> I don't know if this type of a query is common in the C++ FE but unless >>> this is an isolated use case then besides fixing the bug I thought it >>> would be nice to make it easier to get the test above right, or at least >>> come close to it. >>> >>> Since null_pointer_constant_p already exists (but isn't suitable here >>> because it returns true for plain literal zeros) >> >> Why is that unsuitable? A literal zero is a perfectly good >> zero-initializer for a pointer. > > Right, that's why it's not suitable here. Because a literal zero > is also not a pointer. > > The question the code asks is: "is the initializer expression > a pointer (of any kind)?" Why is that a question we want to ask? What we need here is to know whether the initializer expression is equivalent to implicit zero-initialization. For initializing a pointer, a literal 0 is equivalent, so we don't want to update last_nonzero. Also, why is the pointer check here rather than part of the POINTER_TYPE_P handling in type_initializer_zero_p? and I thought that might be common enough > to justify adding a helper function for. If it isn't then leaving > it open-coded as it is in the updated patch below is fine with me. > > --- a/gcc/cp/decl.c > +++ b/gcc/cp/decl.c > @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree > max_index, reshape_iter *d, > TREE_CONSTANT (new_init) = false; > > /* Pointers initialized to strings must be treated as non-zero > - even if the string is empty. */ > + even if the string is empty. Handle all kinds of pointers, > + including std::nullptr and GCC's __nullptr, neither of which > + has a pointer type. */ > tree init_type = TREE_TYPE (elt_init); > - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) > + bool init_is_ptr = (TYPE_PTR_OR_PTRMEM_P (init_type) > + || NULLPTR_TYPE_P (init_type) > + || null_node_p (elt_init)); > + if (POINTER_TYPE_P (elt_type) != init_is_ptr > || !type_initializer_zero_p (elt_type, elt_init)) > last_nonzero = index; > > Martin >
On 4/9/20 1:32 PM, Jason Merrill wrote: > On 4/9/20 3:24 PM, Martin Sebor wrote: >> On 4/9/20 1:03 PM, Jason Merrill wrote: >>> On 4/8/20 1:23 PM, Martin Sebor wrote: >>>> On 4/7/20 3:36 PM, Marek Polacek wrote: >>>>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>>>>> Gcc-patches wrote: >>>>>>>> Among the numerous regressions introduced by the change committed >>>>>>>> to GCC 9 to allow string literals as template arguments is a >>>>>>>> failure >>>>>>>> to recognize the C++ nullptr and GCC's __null constants as >>>>>>>> pointers. >>>>>>>> For one, I didn't realize that nullptr, being a null pointer >>>>>>>> constant, >>>>>>>> doesn't have a pointer type, and two, I didn't think of __null >>>>>>>> (which >>>>>>>> is a special integer constant that NULL sometimes expands to). >>>>>>>> >>>>>>>> The attached patch adjusts the special handling of trailing zero >>>>>>>> initializers in reshape_init_array_1 to recognize both kinds of >>>>>>>> constants and avoid treating them as zeros of the array integer >>>>>>>> element type. This restores the expected diagnostics when either >>>>>>>> constant is used in the initializer list. >>>>>>>> >>>>>>>> Martin >>>>>>> >>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>> std::array >>>>>>>> >>>>>>>> gcc/cp/ChangeLog: >>>>>>>> >>>>>>>> PR c++/94510 >>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with all >>>>>>>> kinds >>>>>>>> of pointers. >>>>>>>> >>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>> >>>>>>>> PR c++/94510 >>>>>>>> * g++.dg/init/array57.C: New test. >>>>>>>> * g++.dg/init/array58.C: New test. >>>>>>>> >>>>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>>>> index a127734af69..692c8ed73f4 100644 >>>>>>>> --- a/gcc/cp/decl.c >>>>>>>> +++ b/gcc/cp/decl.c >>>>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree >>>>>>>> max_index, reshape_iter *d, >>>>>>>> TREE_CONSTANT (new_init) = false; >>>>>>>> /* Pointers initialized to strings must be treated as >>>>>>>> non-zero >>>>>>>> - even if the string is empty. */ >>>>>>>> + even if the string is empty. Handle all kinds of pointers, >>>>>>>> + including std::nullptr and GCC's __nullptr, neither of which >>>>>>>> + has a pointer type. */ >>>>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>>>> + || null_node_p (elt_init)); >>>>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>>>> last_nonzero = index; >>>>>>> >>>>>>> It looks like this still won't handle e.g. pointers to member >>>>>>> functions, >>>>>>> e.g. >>>>>>> >>>>>>> struct S { }; >>>>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>>>> >>>>>>> would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P >>>>>>> instead of >>>>>>> POINTER_TYPE_P to catch this case. >>>>>> >>>>>> Good catch! That doesn't fail because unlike null data member >>>>>> pointers >>>>>> which are represented as -1, member function pointers are represented >>>>>> as a zero. >>>>>> >>>>>> I had looked for an API that would answer the question: "is this >>>>>> expression a pointer?" without having to think of all the different >>>>>> kinds of them but all I could find was null_node_p(). Is this a >>>>>> rare, >>>>>> isolated case that having an API like that wouldn't be worth having >>>>>> or should I add one like in the attached update? >>>>>> >>>>>> Martin >>>>> >>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >>>>>> >>>>>> gcc/cp/ChangeLog: >>>>>> >>>>>> PR c++/94510 >>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with all >>>>>> kinds >>>>>> of pointers. >>>>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. >>>>> >>>>> (Drop the gcc/cp/.) >>>>> >>>>>> +/* Returns true if EXPR is a null pointer constant of any type. */ >>>>>> + >>>>>> +inline bool >>>>>> +null_pointer_constant_p (tree expr) >>>>>> +{ >>>>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>>>> + if (expr == null_node) >>>>>> + return true; >>>>>> + tree type = TREE_TYPE (expr); >>>>>> + if (NULLPTR_TYPE_P (type)) >>>>>> + return true; >>>>>> + if (POINTER_TYPE_P (type)) >>>>>> + return integer_zerop (expr); >>>>>> + return null_member_pointer_value_p (expr); >>>>>> +} >>>>>> + >>>>> >>>>> We already have a null_ptr_cst_p so it would be sort of confusing >>>>> to have >>>>> this as well. But are you really interested in whether it's a null >>>>> pointer, >>>>> not just a pointer? >>>> >>>> The goal of the code is to detect a mismatch in "pointerness" between >>>> an initializer expression and the type of the initialized element, so >>>> it needs to know if the expression is a pointer (non-nulls pointers >>>> are detected in type_initializer_zero_p). That means testing a number >>>> of IMO unintuitive conditions: >>>> >>>> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >>>> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >>>> || null_node_p (expr) >>>> >>>> I don't know if this type of a query is common in the C++ FE but unless >>>> this is an isolated use case then besides fixing the bug I thought it >>>> would be nice to make it easier to get the test above right, or at >>>> least >>>> come close to it. >>>> >>>> Since null_pointer_constant_p already exists (but isn't suitable here >>>> because it returns true for plain literal zeros) >>> >>> Why is that unsuitable? A literal zero is a perfectly good >>> zero-initializer for a pointer. >> >> Right, that's why it's not suitable here. Because a literal zero >> is also not a pointer. >> >> The question the code asks is: "is the initializer expression >> a pointer (of any kind)?" > > Why is that a question we want to ask? What we need here is to know > whether the initializer expression is equivalent to implicit > zero-initialization. For initializing a pointer, a literal 0 is > equivalent, so we don't want to update last_nonzero. Yes, but that's not the bug we're fixing. The problem occurs with an integer array and a pointer initializer: int a[2] = { nullptr, 0 }; and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) the test POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) evaluates to false because neither type is a pointer type and type_initializer_zero_p (elt_type, elt_init) returns true because nullptr is zero, and so last_nonzero doesn't get set, the element gets trimmed, and the invalid initialization of int with nullptr isn't diagnosed. But I'm not sure if you're questioning the current code, the simple fix quoted above, or my assertion that null_pointer_constant_p would not be a suitable function to call to tell if an initializer is nullptr vs plain zero. > Also, why is the pointer check here rather than part of the > POINTER_TYPE_P handling in type_initializer_zero_p? type_initializer_zero_p is implemented in terms of initializer_zerop with the only difference that empty strings are considered to be zero only for char arrays and not char pointers. It could be changed to return false for incompatible initializers like pointers (or even __null) for non-pointer types, even if they are zero, but that's not what it's designed to do. Martin > and I thought that might be common enough >> to justify adding a helper function for. If it isn't then leaving >> it open-coded as it is in the updated patch below is fine with me. >> >> --- a/gcc/cp/decl.c >> +++ b/gcc/cp/decl.c >> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree >> max_index, reshape_iter *d, >> TREE_CONSTANT (new_init) = false; >> >> /* Pointers initialized to strings must be treated as non-zero >> - even if the string is empty. */ >> + even if the string is empty. Handle all kinds of pointers, >> + including std::nullptr and GCC's __nullptr, neither of which >> + has a pointer type. */ >> tree init_type = TREE_TYPE (elt_init); >> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >> + bool init_is_ptr = (TYPE_PTR_OR_PTRMEM_P (init_type) >> + || NULLPTR_TYPE_P (init_type) >> + || null_node_p (elt_init)); >> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >> || !type_initializer_zero_p (elt_type, elt_init)) >> last_nonzero = index; >> >> Martin >> >
On 4/9/20 4:23 PM, Martin Sebor wrote: > On 4/9/20 1:32 PM, Jason Merrill wrote: >> On 4/9/20 3:24 PM, Martin Sebor wrote: >>> On 4/9/20 1:03 PM, Jason Merrill wrote: >>>> On 4/8/20 1:23 PM, Martin Sebor wrote: >>>>> On 4/7/20 3:36 PM, Marek Polacek wrote: >>>>>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>>>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>>>>>> Gcc-patches wrote: >>>>>>>>> Among the numerous regressions introduced by the change committed >>>>>>>>> to GCC 9 to allow string literals as template arguments is a >>>>>>>>> failure >>>>>>>>> to recognize the C++ nullptr and GCC's __null constants as >>>>>>>>> pointers. >>>>>>>>> For one, I didn't realize that nullptr, being a null pointer >>>>>>>>> constant, >>>>>>>>> doesn't have a pointer type, and two, I didn't think of __null >>>>>>>>> (which >>>>>>>>> is a special integer constant that NULL sometimes expands to). >>>>>>>>> >>>>>>>>> The attached patch adjusts the special handling of trailing zero >>>>>>>>> initializers in reshape_init_array_1 to recognize both kinds of >>>>>>>>> constants and avoid treating them as zeros of the array integer >>>>>>>>> element type. This restores the expected diagnostics when either >>>>>>>>> constant is used in the initializer list. >>>>>>>>> >>>>>>>>> Martin >>>>>>>> >>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>> std::array >>>>>>>>> >>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>> >>>>>>>>> PR c++/94510 >>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with >>>>>>>>> all kinds >>>>>>>>> of pointers. >>>>>>>>> >>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>> >>>>>>>>> PR c++/94510 >>>>>>>>> * g++.dg/init/array57.C: New test. >>>>>>>>> * g++.dg/init/array58.C: New test. >>>>>>>>> >>>>>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>>>>> index a127734af69..692c8ed73f4 100644 >>>>>>>>> --- a/gcc/cp/decl.c >>>>>>>>> +++ b/gcc/cp/decl.c >>>>>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, >>>>>>>>> tree max_index, reshape_iter *d, >>>>>>>>> TREE_CONSTANT (new_init) = false; >>>>>>>>> /* Pointers initialized to strings must be treated as >>>>>>>>> non-zero >>>>>>>>> - even if the string is empty. */ >>>>>>>>> + even if the string is empty. Handle all kinds of pointers, >>>>>>>>> + including std::nullptr and GCC's __nullptr, neither of which >>>>>>>>> + has a pointer type. */ >>>>>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>>>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>>>>> + || null_node_p (elt_init)); >>>>>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>>>>> last_nonzero = index; >>>>>>>> >>>>>>>> It looks like this still won't handle e.g. pointers to member >>>>>>>> functions, >>>>>>>> e.g. >>>>>>>> >>>>>>>> struct S { }; >>>>>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>>>>> >>>>>>>> would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P >>>>>>>> instead of >>>>>>>> POINTER_TYPE_P to catch this case. >>>>>>> >>>>>>> Good catch! That doesn't fail because unlike null data member >>>>>>> pointers >>>>>>> which are represented as -1, member function pointers are >>>>>>> represented >>>>>>> as a zero. >>>>>>> >>>>>>> I had looked for an API that would answer the question: "is this >>>>>>> expression a pointer?" without having to think of all the different >>>>>>> kinds of them but all I could find was null_node_p(). Is this a >>>>>>> rare, >>>>>>> isolated case that having an API like that wouldn't be worth having >>>>>>> or should I add one like in the attached update? >>>>>>> >>>>>>> Martin >>>>>> >>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array >>>>>>> >>>>>>> gcc/cp/ChangeLog: >>>>>>> >>>>>>> PR c++/94510 >>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with all >>>>>>> kinds >>>>>>> of pointers. >>>>>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. >>>>>> >>>>>> (Drop the gcc/cp/.) >>>>>> >>>>>>> +/* Returns true if EXPR is a null pointer constant of any type. */ >>>>>>> + >>>>>>> +inline bool >>>>>>> +null_pointer_constant_p (tree expr) >>>>>>> +{ >>>>>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>>>>> + if (expr == null_node) >>>>>>> + return true; >>>>>>> + tree type = TREE_TYPE (expr); >>>>>>> + if (NULLPTR_TYPE_P (type)) >>>>>>> + return true; >>>>>>> + if (POINTER_TYPE_P (type)) >>>>>>> + return integer_zerop (expr); >>>>>>> + return null_member_pointer_value_p (expr); >>>>>>> +} >>>>>>> + >>>>>> >>>>>> We already have a null_ptr_cst_p so it would be sort of confusing >>>>>> to have >>>>>> this as well. But are you really interested in whether it's a >>>>>> null pointer, >>>>>> not just a pointer? >>>>> >>>>> The goal of the code is to detect a mismatch in "pointerness" between >>>>> an initializer expression and the type of the initialized element, so >>>>> it needs to know if the expression is a pointer (non-nulls pointers >>>>> are detected in type_initializer_zero_p). That means testing a number >>>>> of IMO unintuitive conditions: >>>>> >>>>> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >>>>> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >>>>> || null_node_p (expr) >>>>> >>>>> I don't know if this type of a query is common in the C++ FE but >>>>> unless >>>>> this is an isolated use case then besides fixing the bug I thought it >>>>> would be nice to make it easier to get the test above right, or at >>>>> least >>>>> come close to it. >>>>> >>>>> Since null_pointer_constant_p already exists (but isn't suitable here >>>>> because it returns true for plain literal zeros) >>>> >>>> Why is that unsuitable? A literal zero is a perfectly good >>>> zero-initializer for a pointer. >>> >>> Right, that's why it's not suitable here. Because a literal zero >>> is also not a pointer. >>> >>> The question the code asks is: "is the initializer expression >>> a pointer (of any kind)?" >> >> Why is that a question we want to ask? What we need here is to know >> whether the initializer expression is equivalent to implicit >> zero-initialization. For initializing a pointer, a literal 0 is >> equivalent, so we don't want to update last_nonzero. > > Yes, but that's not the bug we're fixing. The problem occurs with > an integer array and a pointer initializer: > > int a[2] = { nullptr, 0 }; Aha, you're fixing a different bug than the one I was seeing. > and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) > the test > > POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) > > evaluates to false because neither type is a pointer type and > > type_initializer_zero_p (elt_type, elt_init) > > returns true because nullptr is zero, and so last_nonzero doesn't > get set, the element gets trimmed, and the invalid initialization > of int with nullptr isn't diagnosed. > > But I'm not sure if you're questioning the current code, the simple > fix quoted above, or my assertion that null_pointer_constant_p would > not be a suitable function to call to tell if an initializer is > nullptr vs plain zero. > >> Also, why is the pointer check here rather than part of the >> POINTER_TYPE_P handling in type_initializer_zero_p? > > type_initializer_zero_p is implemented in terms of initializer_zerop > with the only difference that empty strings are considered to be zero > only for char arrays and not char pointers. Yeah, but that's the fundamental problem: We're assuming that any zero is suitable for initializing any type except for a few exceptions, and adding more exceptions when we find a new testcase that breaks. Handling this in process_init_constructor_array avoids all these problems by looking at the initializers after they've been converted to the desired type, at which point it's much clearer whether they are zero or not; then we don't need type_initializer_zero_p because the initializer already has the proper type and for zero_init_p types we can just use initializer_zero_p. We do probably want some function that tests whether a particular initializer is equivalent to zero-initialization, which is either initializer_zero_p for zero_init_p types, !expr for pointers to members, and recursing for aggregates. Maybe cp_initializer_zero_p or zero_init_expr_p? > It could be changed to return false for incompatible initializers > like pointers (or even __null) for non-pointer types, even if they > are zero, but that's not what it's designed to do. But that's exactly what we did for 90938. Now you're proposing another small exception, only putting it in the caller instead. I think we'll keep running into these problems until we fix the design issue. It would also be possible to improve things by doing the conversion in type_initializer_zero_p before considering its zeroness, but that would again be duplicating work that we're already doing elsewhere. Jason
On 4/10/20 8:52 AM, Jason Merrill wrote: > On 4/9/20 4:23 PM, Martin Sebor wrote: >> On 4/9/20 1:32 PM, Jason Merrill wrote: >>> On 4/9/20 3:24 PM, Martin Sebor wrote: >>>> On 4/9/20 1:03 PM, Jason Merrill wrote: >>>>> On 4/8/20 1:23 PM, Martin Sebor wrote: >>>>>> On 4/7/20 3:36 PM, Marek Polacek wrote: >>>>>>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>>>>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>>>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>>>>>>> Gcc-patches wrote: >>>>>>>>>> Among the numerous regressions introduced by the change committed >>>>>>>>>> to GCC 9 to allow string literals as template arguments is a >>>>>>>>>> failure >>>>>>>>>> to recognize the C++ nullptr and GCC's __null constants as >>>>>>>>>> pointers. >>>>>>>>>> For one, I didn't realize that nullptr, being a null pointer >>>>>>>>>> constant, >>>>>>>>>> doesn't have a pointer type, and two, I didn't think of __null >>>>>>>>>> (which >>>>>>>>>> is a special integer constant that NULL sometimes expands to). >>>>>>>>>> >>>>>>>>>> The attached patch adjusts the special handling of trailing zero >>>>>>>>>> initializers in reshape_init_array_1 to recognize both kinds of >>>>>>>>>> constants and avoid treating them as zeros of the array integer >>>>>>>>>> element type. This restores the expected diagnostics when either >>>>>>>>>> constant is used in the initializer list. >>>>>>>>>> >>>>>>>>>> Martin >>>>>>>>> >>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>> std::array >>>>>>>>>> >>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>> >>>>>>>>>> PR c++/94510 >>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with >>>>>>>>>> all kinds >>>>>>>>>> of pointers. >>>>>>>>>> >>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>> >>>>>>>>>> PR c++/94510 >>>>>>>>>> * g++.dg/init/array57.C: New test. >>>>>>>>>> * g++.dg/init/array58.C: New test. >>>>>>>>>> >>>>>>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>>>>>> index a127734af69..692c8ed73f4 100644 >>>>>>>>>> --- a/gcc/cp/decl.c >>>>>>>>>> +++ b/gcc/cp/decl.c >>>>>>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, >>>>>>>>>> tree max_index, reshape_iter *d, >>>>>>>>>> TREE_CONSTANT (new_init) = false; >>>>>>>>>> /* Pointers initialized to strings must be treated as >>>>>>>>>> non-zero >>>>>>>>>> - even if the string is empty. */ >>>>>>>>>> + even if the string is empty. Handle all kinds of pointers, >>>>>>>>>> + including std::nullptr and GCC's __nullptr, neither of >>>>>>>>>> which >>>>>>>>>> + has a pointer type. */ >>>>>>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P >>>>>>>>>> (init_type) >>>>>>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>>>>>> + || null_node_p (elt_init)); >>>>>>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>>>>>> last_nonzero = index; >>>>>>>>> >>>>>>>>> It looks like this still won't handle e.g. pointers to member >>>>>>>>> functions, >>>>>>>>> e.g. >>>>>>>>> >>>>>>>>> struct S { }; >>>>>>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>>>>>> >>>>>>>>> would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P >>>>>>>>> instead of >>>>>>>>> POINTER_TYPE_P to catch this case. >>>>>>>> >>>>>>>> Good catch! That doesn't fail because unlike null data member >>>>>>>> pointers >>>>>>>> which are represented as -1, member function pointers are >>>>>>>> represented >>>>>>>> as a zero. >>>>>>>> >>>>>>>> I had looked for an API that would answer the question: "is this >>>>>>>> expression a pointer?" without having to think of all the different >>>>>>>> kinds of them but all I could find was null_node_p(). Is this a >>>>>>>> rare, >>>>>>>> isolated case that having an API like that wouldn't be worth having >>>>>>>> or should I add one like in the attached update? >>>>>>>> >>>>>>>> Martin >>>>>>> >>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>> std::array >>>>>>>> >>>>>>>> gcc/cp/ChangeLog: >>>>>>>> >>>>>>>> PR c++/94510 >>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with all >>>>>>>> kinds >>>>>>>> of pointers. >>>>>>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. >>>>>>> >>>>>>> (Drop the gcc/cp/.) >>>>>>> >>>>>>>> +/* Returns true if EXPR is a null pointer constant of any >>>>>>>> type. */ >>>>>>>> + >>>>>>>> +inline bool >>>>>>>> +null_pointer_constant_p (tree expr) >>>>>>>> +{ >>>>>>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>>>>>> + if (expr == null_node) >>>>>>>> + return true; >>>>>>>> + tree type = TREE_TYPE (expr); >>>>>>>> + if (NULLPTR_TYPE_P (type)) >>>>>>>> + return true; >>>>>>>> + if (POINTER_TYPE_P (type)) >>>>>>>> + return integer_zerop (expr); >>>>>>>> + return null_member_pointer_value_p (expr); >>>>>>>> +} >>>>>>>> + >>>>>>> >>>>>>> We already have a null_ptr_cst_p so it would be sort of confusing >>>>>>> to have >>>>>>> this as well. But are you really interested in whether it's a >>>>>>> null pointer, >>>>>>> not just a pointer? >>>>>> >>>>>> The goal of the code is to detect a mismatch in "pointerness" between >>>>>> an initializer expression and the type of the initialized element, so >>>>>> it needs to know if the expression is a pointer (non-nulls pointers >>>>>> are detected in type_initializer_zero_p). That means testing a >>>>>> number >>>>>> of IMO unintuitive conditions: >>>>>> >>>>>> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >>>>>> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >>>>>> || null_node_p (expr) >>>>>> >>>>>> I don't know if this type of a query is common in the C++ FE but >>>>>> unless >>>>>> this is an isolated use case then besides fixing the bug I thought it >>>>>> would be nice to make it easier to get the test above right, or at >>>>>> least >>>>>> come close to it. >>>>>> >>>>>> Since null_pointer_constant_p already exists (but isn't suitable here >>>>>> because it returns true for plain literal zeros) >>>>> >>>>> Why is that unsuitable? A literal zero is a perfectly good >>>>> zero-initializer for a pointer. >>>> >>>> Right, that's why it's not suitable here. Because a literal zero >>>> is also not a pointer. >>>> >>>> The question the code asks is: "is the initializer expression >>>> a pointer (of any kind)?" >>> >>> Why is that a question we want to ask? What we need here is to know >>> whether the initializer expression is equivalent to implicit >>> zero-initialization. For initializing a pointer, a literal 0 is >>> equivalent, so we don't want to update last_nonzero. >> >> Yes, but that's not the bug we're fixing. The problem occurs with >> an integer array and a pointer initializer: >> >> int a[2] = { nullptr, 0 }; > > Aha, you're fixing a different bug than the one I was seeing. What is that one? (I'm not aware of any others in this area.) > >> and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) >> the test >> >> POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >> >> evaluates to false because neither type is a pointer type and >> >> type_initializer_zero_p (elt_type, elt_init) >> >> returns true because nullptr is zero, and so last_nonzero doesn't >> get set, the element gets trimmed, and the invalid initialization >> of int with nullptr isn't diagnosed. >> >> But I'm not sure if you're questioning the current code, the simple >> fix quoted above, or my assertion that null_pointer_constant_p would >> not be a suitable function to call to tell if an initializer is >> nullptr vs plain zero. >> >>> Also, why is the pointer check here rather than part of the >>> POINTER_TYPE_P handling in type_initializer_zero_p? >> >> type_initializer_zero_p is implemented in terms of initializer_zerop >> with the only difference that empty strings are considered to be zero >> only for char arrays and not char pointers. > > Yeah, but that's the fundamental problem: We're assuming that any zero > is suitable for initializing any type except for a few exceptions, and > adding more exceptions when we find a new testcase that breaks. > > Handling this in process_init_constructor_array avoids all these > problems by looking at the initializers after they've been converted to > the desired type, at which point it's much clearer whether they are zero > or not; then we don't need type_initializer_zero_p because the > initializer already has the proper type and for zero_init_p types we can > just use initializer_zero_p. I've already expressed my concerns with that change but if you are comfortable with it I won't insist on waiting until GCC 11. Your last request for that patch was to rework the second loop to avoid changing the counter of the previous loop. The attached update does that. I also added another C++ 2a test to exercise a few more cases with pointers to members. With it I ran into what looks like an unrelated bug in this area. I opened PR 94568 for it, CC'd you, and xfailed the problem case in the new test. > > We do probably want some function that tests whether a particular > initializer is equivalent to zero-initialization, which is either > initializer_zero_p for zero_init_p types, !expr for pointers to members, > and recursing for aggregates. Maybe cp_initializer_zero_p or > zero_init_expr_p? > >> It could be changed to return false for incompatible initializers >> like pointers (or even __null) for non-pointer types, even if they >> are zero, but that's not what it's designed to do. > > But that's exactly what we did for 90938. Now you're proposing another > small exception, only putting it in the caller instead. I think we'll > keep running into these problems until we fix the design issue. Somehow that felt different. But I don't have a problem with moving the pointer check there as well. It shouldn't be too much more intrusive than the original patch for this bug if you decide to go with it for now. > > It would also be possible to improve things by doing the conversion in > type_initializer_zero_p before considering its zeroness, but that would > again be duplicating work that we're already doing elsewhere. I agree that it's not worth the trouble given the long-term fix is in process_init_constructor_array. Attached is the updated patch with the process_init_constructor_array changes, retested on x86_64-linux. Martin
On 4/12/20 5:49 PM, Martin Sebor wrote: > On 4/10/20 8:52 AM, Jason Merrill wrote: >> On 4/9/20 4:23 PM, Martin Sebor wrote: >>> On 4/9/20 1:32 PM, Jason Merrill wrote: >>>> On 4/9/20 3:24 PM, Martin Sebor wrote: >>>>> On 4/9/20 1:03 PM, Jason Merrill wrote: >>>>>> On 4/8/20 1:23 PM, Martin Sebor wrote: >>>>>>> On 4/7/20 3:36 PM, Marek Polacek wrote: >>>>>>>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>>>>>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>>>>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>>>>>>>> Gcc-patches wrote: >>>>>>>>>>> Among the numerous regressions introduced by the change >>>>>>>>>>> committed >>>>>>>>>>> to GCC 9 to allow string literals as template arguments is a >>>>>>>>>>> failure >>>>>>>>>>> to recognize the C++ nullptr and GCC's __null constants as >>>>>>>>>>> pointers. >>>>>>>>>>> For one, I didn't realize that nullptr, being a null pointer >>>>>>>>>>> constant, >>>>>>>>>>> doesn't have a pointer type, and two, I didn't think of >>>>>>>>>>> __null (which >>>>>>>>>>> is a special integer constant that NULL sometimes expands to). >>>>>>>>>>> >>>>>>>>>>> The attached patch adjusts the special handling of trailing zero >>>>>>>>>>> initializers in reshape_init_array_1 to recognize both kinds of >>>>>>>>>>> constants and avoid treating them as zeros of the array integer >>>>>>>>>>> element type. This restores the expected diagnostics when >>>>>>>>>>> either >>>>>>>>>>> constant is used in the initializer list. >>>>>>>>>>> >>>>>>>>>>> Martin >>>>>>>>>> >>>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>>> std::array >>>>>>>>>>> >>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>> >>>>>>>>>>> PR c++/94510 >>>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with >>>>>>>>>>> all kinds >>>>>>>>>>> of pointers. >>>>>>>>>>> >>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>> >>>>>>>>>>> PR c++/94510 >>>>>>>>>>> * g++.dg/init/array57.C: New test. >>>>>>>>>>> * g++.dg/init/array58.C: New test. >>>>>>>>>>> >>>>>>>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>>>>>>> index a127734af69..692c8ed73f4 100644 >>>>>>>>>>> --- a/gcc/cp/decl.c >>>>>>>>>>> +++ b/gcc/cp/decl.c >>>>>>>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, >>>>>>>>>>> tree max_index, reshape_iter *d, >>>>>>>>>>> TREE_CONSTANT (new_init) = false; >>>>>>>>>>> /* Pointers initialized to strings must be treated >>>>>>>>>>> as non-zero >>>>>>>>>>> - even if the string is empty. */ >>>>>>>>>>> + even if the string is empty. Handle all kinds of >>>>>>>>>>> pointers, >>>>>>>>>>> + including std::nullptr and GCC's __nullptr, neither of >>>>>>>>>>> which >>>>>>>>>>> + has a pointer type. */ >>>>>>>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>>>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P >>>>>>>>>>> (init_type) >>>>>>>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>>>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>>>>>>> + || null_node_p (elt_init)); >>>>>>>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>>>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>>>>>>> last_nonzero = index; >>>>>>>>>> >>>>>>>>>> It looks like this still won't handle e.g. pointers to member >>>>>>>>>> functions, >>>>>>>>>> e.g. >>>>>>>>>> >>>>>>>>>> struct S { }; >>>>>>>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>>>>>>> >>>>>>>>>> would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P >>>>>>>>>> instead of >>>>>>>>>> POINTER_TYPE_P to catch this case. >>>>>>>>> >>>>>>>>> Good catch! That doesn't fail because unlike null data member >>>>>>>>> pointers >>>>>>>>> which are represented as -1, member function pointers are >>>>>>>>> represented >>>>>>>>> as a zero. >>>>>>>>> >>>>>>>>> I had looked for an API that would answer the question: "is this >>>>>>>>> expression a pointer?" without having to think of all the >>>>>>>>> different >>>>>>>>> kinds of them but all I could find was null_node_p(). Is this >>>>>>>>> a rare, >>>>>>>>> isolated case that having an API like that wouldn't be worth >>>>>>>>> having >>>>>>>>> or should I add one like in the attached update? >>>>>>>>> >>>>>>>>> Martin >>>>>>>> >>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>> std::array >>>>>>>>> >>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>> >>>>>>>>> PR c++/94510 >>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with >>>>>>>>> all kinds >>>>>>>>> of pointers. >>>>>>>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. >>>>>>>> >>>>>>>> (Drop the gcc/cp/.) >>>>>>>> >>>>>>>>> +/* Returns true if EXPR is a null pointer constant of any >>>>>>>>> type. */ >>>>>>>>> + >>>>>>>>> +inline bool >>>>>>>>> +null_pointer_constant_p (tree expr) >>>>>>>>> +{ >>>>>>>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>>>>>>> + if (expr == null_node) >>>>>>>>> + return true; >>>>>>>>> + tree type = TREE_TYPE (expr); >>>>>>>>> + if (NULLPTR_TYPE_P (type)) >>>>>>>>> + return true; >>>>>>>>> + if (POINTER_TYPE_P (type)) >>>>>>>>> + return integer_zerop (expr); >>>>>>>>> + return null_member_pointer_value_p (expr); >>>>>>>>> +} >>>>>>>>> + >>>>>>>> >>>>>>>> We already have a null_ptr_cst_p so it would be sort of >>>>>>>> confusing to have >>>>>>>> this as well. But are you really interested in whether it's a >>>>>>>> null pointer, >>>>>>>> not just a pointer? >>>>>>> >>>>>>> The goal of the code is to detect a mismatch in "pointerness" >>>>>>> between >>>>>>> an initializer expression and the type of the initialized >>>>>>> element, so >>>>>>> it needs to know if the expression is a pointer (non-nulls pointers >>>>>>> are detected in type_initializer_zero_p). That means testing a >>>>>>> number >>>>>>> of IMO unintuitive conditions: >>>>>>> >>>>>>> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >>>>>>> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >>>>>>> || null_node_p (expr) >>>>>>> >>>>>>> I don't know if this type of a query is common in the C++ FE but >>>>>>> unless >>>>>>> this is an isolated use case then besides fixing the bug I >>>>>>> thought it >>>>>>> would be nice to make it easier to get the test above right, or >>>>>>> at least >>>>>>> come close to it. >>>>>>> >>>>>>> Since null_pointer_constant_p already exists (but isn't suitable >>>>>>> here >>>>>>> because it returns true for plain literal zeros) >>>>>> >>>>>> Why is that unsuitable? A literal zero is a perfectly good >>>>>> zero-initializer for a pointer. >>>>> >>>>> Right, that's why it's not suitable here. Because a literal zero >>>>> is also not a pointer. >>>>> >>>>> The question the code asks is: "is the initializer expression >>>>> a pointer (of any kind)?" >>>> >>>> Why is that a question we want to ask? What we need here is to know >>>> whether the initializer expression is equivalent to implicit >>>> zero-initialization. For initializing a pointer, a literal 0 is >>>> equivalent, so we don't want to update last_nonzero. >>> >>> Yes, but that's not the bug we're fixing. The problem occurs with >>> an integer array and a pointer initializer: >>> >>> int a[2] = { nullptr, 0 }; >> >> Aha, you're fixing a different bug than the one I was seeing. > > What is that one? (I'm not aware of any others in this area.) > >> >>> and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) >>> the test >>> >>> POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>> >>> evaluates to false because neither type is a pointer type and >>> >>> type_initializer_zero_p (elt_type, elt_init) >>> >>> returns true because nullptr is zero, and so last_nonzero doesn't >>> get set, the element gets trimmed, and the invalid initialization >>> of int with nullptr isn't diagnosed. >>> >>> But I'm not sure if you're questioning the current code, the simple >>> fix quoted above, or my assertion that null_pointer_constant_p would >>> not be a suitable function to call to tell if an initializer is >>> nullptr vs plain zero. >>> >>>> Also, why is the pointer check here rather than part of the >>>> POINTER_TYPE_P handling in type_initializer_zero_p? >>> >>> type_initializer_zero_p is implemented in terms of initializer_zerop >>> with the only difference that empty strings are considered to be zero >>> only for char arrays and not char pointers. >> >> Yeah, but that's the fundamental problem: We're assuming that any zero >> is suitable for initializing any type except for a few exceptions, and >> adding more exceptions when we find a new testcase that breaks. >> >> Handling this in process_init_constructor_array avoids all these >> problems by looking at the initializers after they've been converted >> to the desired type, at which point it's much clearer whether they are >> zero or not; then we don't need type_initializer_zero_p because the >> initializer already has the proper type and for zero_init_p types we >> can just use initializer_zero_p. > > I've already expressed my concerns with that change but if you are > comfortable with it I won't insist on waiting until GCC 11. Your last > request for that patch was to rework the second loop to avoid changing > the counter of the previous loop. The attached update does that. > > I also added another C++ 2a test to exercise a few more cases with > pointers to members. With it I ran into what looks like an unrelated > bug in this area. I opened PR 94568 for it, CC'd you, and xfailed > the problem case in the new test. > >> >> We do probably want some function that tests whether a particular >> initializer is equivalent to zero-initialization, which is either >> initializer_zero_p for zero_init_p types, !expr for pointers to >> members, and recursing for aggregates. Maybe cp_initializer_zero_p or >> zero_init_expr_p? >> >>> It could be changed to return false for incompatible initializers >>> like pointers (or even __null) for non-pointer types, even if they >>> are zero, but that's not what it's designed to do. >> >> But that's exactly what we did for 90938. Now you're proposing >> another small exception, only putting it in the caller instead. I >> think we'll keep running into these problems until we fix the design >> issue. > > Somehow that felt different. But I don't have a problem with moving > the pointer check there as well. It shouldn't be too much more > intrusive than the original patch for this bug if you decide to > go with it for now. > >> >> It would also be possible to improve things by doing the conversion in >> type_initializer_zero_p before considering its zeroness, but that >> would again be duplicating work that we're already doing elsewhere. > > I agree that it's not worth the trouble given the long-term fix is > in process_init_constructor_array. > > Attached is the updated patch with the process_init_constructor_array > changes, retested on x86_64-linux. > + if (!trunc_zero || !type_initializer_zero_p (eltype, ce->value)) > + last_nonzero = i; I think we can remove type_initializer_zero_p as well, and use initializer_zerop here. > + if (last_nonzero < i - 1) > + { > + vec_safe_truncate (v, last_nonzero + 1); This looks like you will never truncate to length 0, which seems like a problem with last_nonzero being both unsigned and an index; perhaps it should be something like num_to_keep? > + len = i = vec_safe_length (v); > + } Nitpick: It seems you don't need to update len or i since you're about to return. > - else if (TREE_CODE (next) == CONSTRUCTOR > - && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next)) > - { > - /* As above. */ > - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0; > - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; > - } This is from the recent fix for 90996, we want to keep it. > /* Set to the index of the last initializer element whose value > (either as a single expression or as a repeating range) to > append to the CONSTRUCTOR. */ > unsigned HOST_WIDE_INT last_to_append = i; OK, I was wrong, let's go back to modifying i instead of introducing this variable. > if (next) > { > picflags |= picflag_from_initializer (next); > if (initializer_constant_valid_p (next, TREE_TYPE (next)) > == null_pointer_node) > { > /* If the last distinct explicit initializer value is > the same as the implicit initializer NEXT built above, > include the former in the range built below. */ > if (i && next == (*v)[last_distinct].value) > last_to_append = last_distinct; > > break; > } > > CONSTRUCTOR_APPEND_ELT (v, size_int (i), next); > last_distinct = i; > } > else > { > /* Don't bother checking all the other elements and avoid > appending to the initializer list below. */ > last_distinct = i; > last_to_append = i + 1; > break; > } > if (last_distinct < last_to_append - 1) Could this be if (len > i+1 && next) instead, and never look at last_distinct again? If next is the same as the last_distinct value, we will have adjusted i to be last_distinct; otherwise, we still want to start the range with i. Jason
On 4/13/20 8:43 PM, Jason Merrill wrote: > On 4/12/20 5:49 PM, Martin Sebor wrote: >> On 4/10/20 8:52 AM, Jason Merrill wrote: >>> On 4/9/20 4:23 PM, Martin Sebor wrote: >>>> On 4/9/20 1:32 PM, Jason Merrill wrote: >>>>> On 4/9/20 3:24 PM, Martin Sebor wrote: >>>>>> On 4/9/20 1:03 PM, Jason Merrill wrote: >>>>>>> On 4/8/20 1:23 PM, Martin Sebor wrote: >>>>>>>> On 4/7/20 3:36 PM, Marek Polacek wrote: >>>>>>>>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>>>>>>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>>>>>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>>>>>>>>> Gcc-patches wrote: >>>>>>>>>>>> Among the numerous regressions introduced by the change >>>>>>>>>>>> committed >>>>>>>>>>>> to GCC 9 to allow string literals as template arguments is a >>>>>>>>>>>> failure >>>>>>>>>>>> to recognize the C++ nullptr and GCC's __null constants as >>>>>>>>>>>> pointers. >>>>>>>>>>>> For one, I didn't realize that nullptr, being a null pointer >>>>>>>>>>>> constant, >>>>>>>>>>>> doesn't have a pointer type, and two, I didn't think of >>>>>>>>>>>> __null (which >>>>>>>>>>>> is a special integer constant that NULL sometimes expands to). >>>>>>>>>>>> >>>>>>>>>>>> The attached patch adjusts the special handling of trailing >>>>>>>>>>>> zero >>>>>>>>>>>> initializers in reshape_init_array_1 to recognize both kinds of >>>>>>>>>>>> constants and avoid treating them as zeros of the array integer >>>>>>>>>>>> element type. This restores the expected diagnostics when >>>>>>>>>>>> either >>>>>>>>>>>> constant is used in the initializer list. >>>>>>>>>>>> >>>>>>>>>>>> Martin >>>>>>>>>>> >>>>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>>>> std::array >>>>>>>>>>>> >>>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>>> >>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with >>>>>>>>>>>> all kinds >>>>>>>>>>>> of pointers. >>>>>>>>>>>> >>>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>>> >>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>> * g++.dg/init/array57.C: New test. >>>>>>>>>>>> * g++.dg/init/array58.C: New test. >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>>>>>>>> index a127734af69..692c8ed73f4 100644 >>>>>>>>>>>> --- a/gcc/cp/decl.c >>>>>>>>>>>> +++ b/gcc/cp/decl.c >>>>>>>>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, >>>>>>>>>>>> tree max_index, reshape_iter *d, >>>>>>>>>>>> TREE_CONSTANT (new_init) = false; >>>>>>>>>>>> /* Pointers initialized to strings must be treated >>>>>>>>>>>> as non-zero >>>>>>>>>>>> - even if the string is empty. */ >>>>>>>>>>>> + even if the string is empty. Handle all kinds of >>>>>>>>>>>> pointers, >>>>>>>>>>>> + including std::nullptr and GCC's __nullptr, neither of >>>>>>>>>>>> which >>>>>>>>>>>> + has a pointer type. */ >>>>>>>>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>>>>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P >>>>>>>>>>>> (init_type) >>>>>>>>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>>>>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>>>>>>>> + || null_node_p (elt_init)); >>>>>>>>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>>>>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>>>>>>>> last_nonzero = index; >>>>>>>>>>> >>>>>>>>>>> It looks like this still won't handle e.g. pointers to member >>>>>>>>>>> functions, >>>>>>>>>>> e.g. >>>>>>>>>>> >>>>>>>>>>> struct S { }; >>>>>>>>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>>>>>>>> >>>>>>>>>>> would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P >>>>>>>>>>> instead of >>>>>>>>>>> POINTER_TYPE_P to catch this case. >>>>>>>>>> >>>>>>>>>> Good catch! That doesn't fail because unlike null data member >>>>>>>>>> pointers >>>>>>>>>> which are represented as -1, member function pointers are >>>>>>>>>> represented >>>>>>>>>> as a zero. >>>>>>>>>> >>>>>>>>>> I had looked for an API that would answer the question: "is this >>>>>>>>>> expression a pointer?" without having to think of all the >>>>>>>>>> different >>>>>>>>>> kinds of them but all I could find was null_node_p(). Is this >>>>>>>>>> a rare, >>>>>>>>>> isolated case that having an API like that wouldn't be worth >>>>>>>>>> having >>>>>>>>>> or should I add one like in the attached update? >>>>>>>>>> >>>>>>>>>> Martin >>>>>>>>> >>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>> std::array >>>>>>>>>> >>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>> >>>>>>>>>> PR c++/94510 >>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with >>>>>>>>>> all kinds >>>>>>>>>> of pointers. >>>>>>>>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. >>>>>>>>> >>>>>>>>> (Drop the gcc/cp/.) >>>>>>>>> >>>>>>>>>> +/* Returns true if EXPR is a null pointer constant of any >>>>>>>>>> type. */ >>>>>>>>>> + >>>>>>>>>> +inline bool >>>>>>>>>> +null_pointer_constant_p (tree expr) >>>>>>>>>> +{ >>>>>>>>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>>>>>>>> + if (expr == null_node) >>>>>>>>>> + return true; >>>>>>>>>> + tree type = TREE_TYPE (expr); >>>>>>>>>> + if (NULLPTR_TYPE_P (type)) >>>>>>>>>> + return true; >>>>>>>>>> + if (POINTER_TYPE_P (type)) >>>>>>>>>> + return integer_zerop (expr); >>>>>>>>>> + return null_member_pointer_value_p (expr); >>>>>>>>>> +} >>>>>>>>>> + >>>>>>>>> >>>>>>>>> We already have a null_ptr_cst_p so it would be sort of >>>>>>>>> confusing to have >>>>>>>>> this as well. But are you really interested in whether it's a >>>>>>>>> null pointer, >>>>>>>>> not just a pointer? >>>>>>>> >>>>>>>> The goal of the code is to detect a mismatch in "pointerness" >>>>>>>> between >>>>>>>> an initializer expression and the type of the initialized >>>>>>>> element, so >>>>>>>> it needs to know if the expression is a pointer (non-nulls pointers >>>>>>>> are detected in type_initializer_zero_p). That means testing a >>>>>>>> number >>>>>>>> of IMO unintuitive conditions: >>>>>>>> >>>>>>>> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >>>>>>>> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >>>>>>>> || null_node_p (expr) >>>>>>>> >>>>>>>> I don't know if this type of a query is common in the C++ FE but >>>>>>>> unless >>>>>>>> this is an isolated use case then besides fixing the bug I >>>>>>>> thought it >>>>>>>> would be nice to make it easier to get the test above right, or >>>>>>>> at least >>>>>>>> come close to it. >>>>>>>> >>>>>>>> Since null_pointer_constant_p already exists (but isn't suitable >>>>>>>> here >>>>>>>> because it returns true for plain literal zeros) >>>>>>> >>>>>>> Why is that unsuitable? A literal zero is a perfectly good >>>>>>> zero-initializer for a pointer. >>>>>> >>>>>> Right, that's why it's not suitable here. Because a literal zero >>>>>> is also not a pointer. >>>>>> >>>>>> The question the code asks is: "is the initializer expression >>>>>> a pointer (of any kind)?" >>>>> >>>>> Why is that a question we want to ask? What we need here is to >>>>> know whether the initializer expression is equivalent to implicit >>>>> zero-initialization. For initializing a pointer, a literal 0 is >>>>> equivalent, so we don't want to update last_nonzero. >>>> >>>> Yes, but that's not the bug we're fixing. The problem occurs with >>>> an integer array and a pointer initializer: >>>> >>>> int a[2] = { nullptr, 0 }; >>> >>> Aha, you're fixing a different bug than the one I was seeing. >> >> What is that one? (I'm not aware of any others in this area.) >> >>> >>>> and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) >>>> the test >>>> >>>> POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>> >>>> evaluates to false because neither type is a pointer type and >>>> >>>> type_initializer_zero_p (elt_type, elt_init) >>>> >>>> returns true because nullptr is zero, and so last_nonzero doesn't >>>> get set, the element gets trimmed, and the invalid initialization >>>> of int with nullptr isn't diagnosed. >>>> >>>> But I'm not sure if you're questioning the current code, the simple >>>> fix quoted above, or my assertion that null_pointer_constant_p would >>>> not be a suitable function to call to tell if an initializer is >>>> nullptr vs plain zero. >>>> >>>>> Also, why is the pointer check here rather than part of the >>>>> POINTER_TYPE_P handling in type_initializer_zero_p? >>>> >>>> type_initializer_zero_p is implemented in terms of initializer_zerop >>>> with the only difference that empty strings are considered to be zero >>>> only for char arrays and not char pointers. >>> >>> Yeah, but that's the fundamental problem: We're assuming that any >>> zero is suitable for initializing any type except for a few >>> exceptions, and adding more exceptions when we find a new testcase >>> that breaks. >>> >>> Handling this in process_init_constructor_array avoids all these >>> problems by looking at the initializers after they've been converted >>> to the desired type, at which point it's much clearer whether they >>> are zero or not; then we don't need type_initializer_zero_p because >>> the initializer already has the proper type and for zero_init_p types >>> we can just use initializer_zero_p. >> >> I've already expressed my concerns with that change but if you are >> comfortable with it I won't insist on waiting until GCC 11. Your last >> request for that patch was to rework the second loop to avoid changing >> the counter of the previous loop. The attached update does that. >> >> I also added another C++ 2a test to exercise a few more cases with >> pointers to members. With it I ran into what looks like an unrelated >> bug in this area. I opened PR 94568 for it, CC'd you, and xfailed >> the problem case in the new test. >> >>> >>> We do probably want some function that tests whether a particular >>> initializer is equivalent to zero-initialization, which is either >>> initializer_zero_p for zero_init_p types, !expr for pointers to >>> members, and recursing for aggregates. Maybe cp_initializer_zero_p >>> or zero_init_expr_p? >>> >>>> It could be changed to return false for incompatible initializers >>>> like pointers (or even __null) for non-pointer types, even if they >>>> are zero, but that's not what it's designed to do. >>> >>> But that's exactly what we did for 90938. Now you're proposing >>> another small exception, only putting it in the caller instead. I >>> think we'll keep running into these problems until we fix the design >>> issue. >> >> Somehow that felt different. But I don't have a problem with moving >> the pointer check there as well. It shouldn't be too much more >> intrusive than the original patch for this bug if you decide to >> go with it for now. >> >>> >>> It would also be possible to improve things by doing the conversion >>> in type_initializer_zero_p before considering its zeroness, but that >>> would again be duplicating work that we're already doing elsewhere. >> >> I agree that it's not worth the trouble given the long-term fix is >> in process_init_constructor_array. >> >> Attached is the updated patch with the process_init_constructor_array >> changes, retested on x86_64-linux. > >> + if (!trunc_zero || !type_initializer_zero_p (eltype, ce->value)) >> + last_nonzero = i; > > I think we can remove type_initializer_zero_p as well, and use > initializer_zerop here. > >> + if (last_nonzero < i - 1) >> + { >> + vec_safe_truncate (v, last_nonzero + 1); > > This looks like you will never truncate to length 0, which seems like a > problem with last_nonzero being both unsigned and an index; perhaps it > should be something like num_to_keep? This whole block appears to serve no real purpose. It trims trailing zeros only from arithmetic types, but the trimming only matters for pointers to members and that's done later. I've removed it. > >> + len = i = vec_safe_length (v); >> + } > > Nitpick: It seems you don't need to update len or i since you're about > to return. > >> - else if (TREE_CODE (next) == CONSTRUCTOR >> - && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next)) >> - { >> - /* As above. */ >> - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0; >> - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; >> - } > > This is from the recent fix for 90996, we want to keep it. Whoops. But no test failed with this change, not even pr90996.C (with make check-c++-all). I'm not sure how to write one that does fail. > >> /* Set to the index of the last initializer element whose value >> (either as a single expression or as a repeating range) to append >> to the CONSTRUCTOR. */ >> unsigned HOST_WIDE_INT last_to_append = i; > > OK, I was wrong, let's go back to modifying i instead of introducing > this variable. > >> if (next) >> { >> picflags |= picflag_from_initializer (next); >> if (initializer_constant_valid_p (next, TREE_TYPE (next)) >> == null_pointer_node) >> { >> /* If the last distinct explicit initializer value is >> the same as the implicit initializer NEXT built >> above, include the former in the range built below. */ >> if (i && next == (*v)[last_distinct].value) >> last_to_append = last_distinct; >> >> break; >> } >> >> CONSTRUCTOR_APPEND_ELT (v, size_int (i), next); >> last_distinct = i; >> } >> else >> { >> /* Don't bother checking all the other elements and avoid >> appending to the initializer list below. */ >> last_distinct = i; >> last_to_append = i + 1; >> break; >> } > >> if (last_distinct < last_to_append - 1) > > Could this be > > if (len > i+1 && next) > > instead, and never look at last_distinct again? If next is the same as > the last_distinct value, we will have adjusted i to be last_distinct; > otherwise, we still want to start the range with i. It's not the same and breaks tests. But even if it could be, it makes no difference in readability. That said, I do find the second loop with the subsequent test awkward to work with. I've adjusted it a little to make it easier for me to follow (but YMMV). I also added more tests. Together this let me uncover a subtle bug in the code (not setting the right null pointer range after non-empty non-null initializers). Attached is another revision of this patch, retested on x85_64-linux. Martin
On Wed, 15 Apr 2020, Martin Sebor via Gcc-patches wrote: > On 4/13/20 8:43 PM, Jason Merrill wrote: > > On 4/12/20 5:49 PM, Martin Sebor wrote: > > > On 4/10/20 8:52 AM, Jason Merrill wrote: > > > > On 4/9/20 4:23 PM, Martin Sebor wrote: > > > > > On 4/9/20 1:32 PM, Jason Merrill wrote: > > > > > > On 4/9/20 3:24 PM, Martin Sebor wrote: > > > > > > > On 4/9/20 1:03 PM, Jason Merrill wrote: > > > > > > > > On 4/8/20 1:23 PM, Martin Sebor wrote: > > > > > > > > > On 4/7/20 3:36 PM, Marek Polacek wrote: > > > > > > > > > > On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor > > > > > > > > > > wrote: > > > > > > > > > > > On 4/7/20 1:50 PM, Marek Polacek wrote: > > > > > > > > > > > > On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor > > > > > > > > > > > > via Gcc-patches wrote: > > > > > > > > > > > > > Among the numerous regressions introduced by the > > > > > > > > > > > > > change committed > > > > > > > > > > > > > to GCC 9 to allow string literals as template > > > > > > > > > > > > > arguments is a failure > > > > > > > > > > > > > to recognize the C++ nullptr and GCC's __null > > > > > > > > > > > > > constants as pointers. > > > > > > > > > > > > > For one, I didn't realize that nullptr, being a null > > > > > > > > > > > > > pointer constant, > > > > > > > > > > > > > doesn't have a pointer type, and two, I didn't think > > > > > > > > > > > > > of __null (which > > > > > > > > > > > > > is a special integer constant that NULL sometimes > > > > > > > > > > > > > expands to). > > > > > > > > > > > > > > > > > > > > > > > > > > The attached patch adjusts the special handling of > > > > > > > > > > > > > trailing zero > > > > > > > > > > > > > initializers in reshape_init_array_1 to recognize both > > > > > > > > > > > > > kinds of > > > > > > > > > > > > > constants and avoid treating them as zeros of the > > > > > > > > > > > > > array integer > > > > > > > > > > > > > element type. This restores the expected diagnostics > > > > > > > > > > > > > when either > > > > > > > > > > > > > constant is used in the initializer list. > > > > > > > > > > > > > > > > > > > > > > > > > > Martin > > > > > > > > > > > > > > > > > > > > > > > > > PR c++/94510 - nullptr_t implicitly cast to zero twice > > > > > > > > > > > > > in std::array > > > > > > > > > > > > > > > > > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > > > > > > > > > > > > > > > > > PR c++/94510 > > > > > > > > > > > > > * decl.c (reshape_init_array_1): Exclude > > > > > > > > > > > > > mismatches with all kinds > > > > > > > > > > > > > of pointers. > > > > > > > > > > > > > > > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > > > > > > > > > > > > > > > > > PR c++/94510 > > > > > > > > > > > > > * g++.dg/init/array57.C: New test. > > > > > > > > > > > > > * g++.dg/init/array58.C: New test. > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c > > > > > > > > > > > > > index a127734af69..692c8ed73f4 100644 > > > > > > > > > > > > > --- a/gcc/cp/decl.c > > > > > > > > > > > > > +++ b/gcc/cp/decl.c > > > > > > > > > > > > > @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree > > > > > > > > > > > > > elt_type, tree max_index, reshape_iter *d, > > > > > > > > > > > > > TREE_CONSTANT (new_init) = false; > > > > > > > > > > > > > /* Pointers initialized to strings must be > > > > > > > > > > > > > treated as non-zero > > > > > > > > > > > > > - even if the string is empty. */ > > > > > > > > > > > > > + even if the string is empty. Handle all kinds > > > > > > > > > > > > > of pointers, > > > > > > > > > > > > > + including std::nullptr and GCC's __nullptr, > > > > > > > > > > > > > neither of which > > > > > > > > > > > > > + has a pointer type. */ > > > > > > > > > > > > > tree init_type = TREE_TYPE (elt_init); > > > > > > > > > > > > > - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P > > > > > > > > > > > > > (init_type) > > > > > > > > > > > > > + bool init_is_ptr = (POINTER_TYPE_P (init_type) > > > > > > > > > > > > > + || NULLPTR_TYPE_P (init_type) > > > > > > > > > > > > > + || null_node_p (elt_init)); > > > > > > > > > > > > > + if (POINTER_TYPE_P (elt_type) != init_is_ptr > > > > > > > > > > > > > || !type_initializer_zero_p (elt_type, > > > > > > > > > > > > > elt_init)) > > > > > > > > > > > > > last_nonzero = index; > > > > > > > > > > > > > > > > > > > > > > > > It looks like this still won't handle e.g. pointers to > > > > > > > > > > > > member functions, > > > > > > > > > > > > e.g. > > > > > > > > > > > > > > > > > > > > > > > > struct S { }; > > > > > > > > > > > > int arr[3] = { (void (S::*) ()) 0, 0, 0 }; > > > > > > > > > > > > > > > > > > > > > > > > would still be accepted. You could use > > > > > > > > > > > > TYPE_PTR_OR_PTRMEM_P instead of > > > > > > > > > > > > POINTER_TYPE_P to catch this case. > > > > > > > > > > > > > > > > > > > > > > Good catch! That doesn't fail because unlike null data > > > > > > > > > > > member pointers > > > > > > > > > > > which are represented as -1, member function pointers are > > > > > > > > > > > represented > > > > > > > > > > > as a zero. > > > > > > > > > > > > > > > > > > > > > > I had looked for an API that would answer the question: > > > > > > > > > > > "is this > > > > > > > > > > > expression a pointer?" without having to think of all the > > > > > > > > > > > different > > > > > > > > > > > kinds of them but all I could find was null_node_p(). Is > > > > > > > > > > > this a rare, > > > > > > > > > > > isolated case that having an API like that wouldn't be > > > > > > > > > > > worth having > > > > > > > > > > > or should I add one like in the attached update? > > > > > > > > > > > > > > > > > > > > > > Martin > > > > > > > > > > > > > > > > > > > > > PR c++/94510 - nullptr_t implicitly cast to zero twice in > > > > > > > > > > > std::array > > > > > > > > > > > > > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > > > > > > > > > > > > > PR c++/94510 > > > > > > > > > > > * decl.c (reshape_init_array_1): Exclude mismatches > > > > > > > > > > > with all kinds > > > > > > > > > > > of pointers. > > > > > > > > > > > * gcc/cp/cp-tree.h (null_pointer_constant_p): New > > > > > > > > > > > function. > > > > > > > > > > > > > > > > > > > > (Drop the gcc/cp/.) > > > > > > > > > > > > > > > > > > > > > +/* Returns true if EXPR is a null pointer constant of any > > > > > > > > > > > type. */ > > > > > > > > > > > + > > > > > > > > > > > +inline bool > > > > > > > > > > > +null_pointer_constant_p (tree expr) > > > > > > > > > > > +{ > > > > > > > > > > > + STRIP_ANY_LOCATION_WRAPPER (expr); > > > > > > > > > > > + if (expr == null_node) > > > > > > > > > > > + return true; > > > > > > > > > > > + tree type = TREE_TYPE (expr); > > > > > > > > > > > + if (NULLPTR_TYPE_P (type)) > > > > > > > > > > > + return true; > > > > > > > > > > > + if (POINTER_TYPE_P (type)) > > > > > > > > > > > + return integer_zerop (expr); > > > > > > > > > > > + return null_member_pointer_value_p (expr); > > > > > > > > > > > +} > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > We already have a null_ptr_cst_p so it would be sort of > > > > > > > > > > confusing to have > > > > > > > > > > this as well. But are you really interested in whether it's > > > > > > > > > > a null pointer, > > > > > > > > > > not just a pointer? > > > > > > > > > > > > > > > > > > The goal of the code is to detect a mismatch in "pointerness" > > > > > > > > > between > > > > > > > > > an initializer expression and the type of the initialized > > > > > > > > > element, so > > > > > > > > > it needs to know if the expression is a pointer (non-nulls > > > > > > > > > pointers > > > > > > > > > are detected in type_initializer_zero_p). That means testing > > > > > > > > > a number > > > > > > > > > of IMO unintuitive conditions: > > > > > > > > > > > > > > > > > > TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) > > > > > > > > > || NULLPTR_TYPE_P (TREE_TYPE (expr)) > > > > > > > > > || null_node_p (expr) > > > > > > > > > > > > > > > > > > I don't know if this type of a query is common in the C++ FE > > > > > > > > > but unless > > > > > > > > > this is an isolated use case then besides fixing the bug I > > > > > > > > > thought it > > > > > > > > > would be nice to make it easier to get the test above right, > > > > > > > > > or at least > > > > > > > > > come close to it. > > > > > > > > > > > > > > > > > > Since null_pointer_constant_p already exists (but isn't > > > > > > > > > suitable here > > > > > > > > > because it returns true for plain literal zeros) > > > > > > > > > > > > > > > > Why is that unsuitable? A literal zero is a perfectly good > > > > > > > > zero-initializer for a pointer. > > > > > > > > > > > > > > Right, that's why it's not suitable here. Because a literal zero > > > > > > > is also not a pointer. > > > > > > > > > > > > > > The question the code asks is: "is the initializer expression > > > > > > > a pointer (of any kind)?" > > > > > > > > > > > > Why is that a question we want to ask? What we need here is to know > > > > > > whether the initializer expression is equivalent to implicit > > > > > > zero-initialization. For initializing a pointer, a literal 0 is > > > > > > equivalent, so we don't want to update last_nonzero. > > > > > > > > > > Yes, but that's not the bug we're fixing. The problem occurs with > > > > > an integer array and a pointer initializer: > > > > > > > > > > int a[2] = { nullptr, 0 }; > > > > > > > > Aha, you're fixing a different bug than the one I was seeing. > > > > > > What is that one? (I'm not aware of any others in this area.) > > > > > > > > > > > > and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) > > > > > the test > > > > > > > > > > POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) > > > > > > > > > > evaluates to false because neither type is a pointer type and > > > > > > > > > > type_initializer_zero_p (elt_type, elt_init) > > > > > > > > > > returns true because nullptr is zero, and so last_nonzero doesn't > > > > > get set, the element gets trimmed, and the invalid initialization > > > > > of int with nullptr isn't diagnosed. > > > > > > > > > > But I'm not sure if you're questioning the current code, the simple > > > > > fix quoted above, or my assertion that null_pointer_constant_p would > > > > > not be a suitable function to call to tell if an initializer is > > > > > nullptr vs plain zero. > > > > > > > > > > > Also, why is the pointer check here rather than part of the > > > > > > POINTER_TYPE_P handling in type_initializer_zero_p? > > > > > > > > > > type_initializer_zero_p is implemented in terms of initializer_zerop > > > > > with the only difference that empty strings are considered to be zero > > > > > only for char arrays and not char pointers. > > > > > > > > Yeah, but that's the fundamental problem: We're assuming that any zero > > > > is suitable for initializing any type except for a few exceptions, and > > > > adding more exceptions when we find a new testcase that breaks. > > > > > > > > Handling this in process_init_constructor_array avoids all these > > > > problems by looking at the initializers after they've been converted to > > > > the desired type, at which point it's much clearer whether they are zero > > > > or not; then we don't need type_initializer_zero_p because the > > > > initializer already has the proper type and for zero_init_p types we can > > > > just use initializer_zero_p. > > > > > > I've already expressed my concerns with that change but if you are > > > comfortable with it I won't insist on waiting until GCC 11. Your last > > > request for that patch was to rework the second loop to avoid changing > > > the counter of the previous loop. The attached update does that. > > > > > > I also added another C++ 2a test to exercise a few more cases with > > > pointers to members. With it I ran into what looks like an unrelated > > > bug in this area. I opened PR 94568 for it, CC'd you, and xfailed > > > the problem case in the new test. > > > > > > > > > > > We do probably want some function that tests whether a particular > > > > initializer is equivalent to zero-initialization, which is either > > > > initializer_zero_p for zero_init_p types, !expr for pointers to members, > > > > and recursing for aggregates. Maybe cp_initializer_zero_p or > > > > zero_init_expr_p? > > > > > > > > > It could be changed to return false for incompatible initializers > > > > > like pointers (or even __null) for non-pointer types, even if they > > > > > are zero, but that's not what it's designed to do. > > > > > > > > But that's exactly what we did for 90938. Now you're proposing another > > > > small exception, only putting it in the caller instead. I think we'll > > > > keep running into these problems until we fix the design issue. > > > > > > Somehow that felt different. But I don't have a problem with moving > > > the pointer check there as well. It shouldn't be too much more > > > intrusive than the original patch for this bug if you decide to > > > go with it for now. > > > > > > > > > > > It would also be possible to improve things by doing the conversion in > > > > type_initializer_zero_p before considering its zeroness, but that would > > > > again be duplicating work that we're already doing elsewhere. > > > > > > I agree that it's not worth the trouble given the long-term fix is > > > in process_init_constructor_array. > > > > > > Attached is the updated patch with the process_init_constructor_array > > > changes, retested on x86_64-linux. > > > > > + if (!trunc_zero || !type_initializer_zero_p (eltype, ce->value)) > > > + last_nonzero = i; > > > > I think we can remove type_initializer_zero_p as well, and use > > initializer_zerop here. > > > > > + if (last_nonzero < i - 1) > > > + { > > > + vec_safe_truncate (v, last_nonzero + 1); > > > > This looks like you will never truncate to length 0, which seems like a > > problem with last_nonzero being both unsigned and an index; perhaps it > > should be something like num_to_keep? > > This whole block appears to serve no real purpose. It trims trailing > zeros only from arithmetic types, but the trimming only matters for > pointers to members and that's done later. I've removed it. > > > > > > + len = i = vec_safe_length (v); > > > + } > > > > Nitpick: It seems you don't need to update len or i since you're about to > > return. > > > > > - else if (TREE_CODE (next) == CONSTRUCTOR > > > - && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next)) > > > - { > > > - /* As above. */ > > > - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0; > > > - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; > > > - } > > > > This is from the recent fix for 90996, we want to keep it. > > Whoops. But no test failed with this change, not even pr90996.C (with > make check-c++-all). I'm not sure how to write one that does fail. Hmm... it looks like the following hunk @@ -3247,7 +3247,7 @@ replace_placeholders (tree exp, tree obj, bool *seen_p /*= NULL*/) /* If the object isn't a (member of a) class, do nothing. */ tree op0 = obj; - while (TREE_CODE (op0) == COMPONENT_REF) + while (handled_component_p (op0)) op0 = TREE_OPERAND (op0, 0); if (!CLASS_TYPE_P (strip_array_types (TREE_TYPE (op0)))) return exp; which I added as an afterthought to my 90996 patch to also handle the initialization 'T d{};' in pr90996.C (and for symmetry with lookup_placeholder) is actually sufficient by itself to compile the whole of pr90996.C and to fix PR90996. With that hunk, the call to replace_placeholders from cp_gimplify_init_expr ends up doing the right thing when passed exp = *(&<PLACEHOLDER_EXPR struct S>)->a obj = c[i][j].b[0] for 0 <= i,j <= 1 because the hunk lets replace_placeholders strip outer ARRAY_REF from 'obj' to resolve each PLACEHOLDER_EXPR to c[i][j]. So if there is no observable advantage to replacing PLACEHOLDER_EXPRs sooner in store_init_value versus rather than later in cp_gimplify_init_expr, then the two hunks in process_init_constructor_array are neither necessary nor sufficient. Sorry I didn't catch this when writing the patch. Shall I commit the following after bootstrap/regtesting? -- >8 -- Subject: [PATCH] c++: Revert unnecessary parts of fix for [PR90996] gcc/cp/ChangeLog: Revert: 2020-04-07 Patrick Palka <ppalka@redhat.com> PR c++/90996 * typeck2.c (process_init_constructor_array): Propagate CONSTRUCTOR_PLACEHOLDER_BOUNDARY up from each element initializer to the array initializer. gcc/testsuite/ChangeLog: PR c++/90996 * g++.dg/cpp1y/pr90996.C: Turn into execution test to verify that each PLACEHOLDER_EXPR gets correctly resolved. --- gcc/cp/typeck2.c | 18 ------------------ gcc/testsuite/g++.dg/cpp1y/pr90996.C | 19 ++++++++++++++++++- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c index 56fd9bafa7e..cf1cb5ace66 100644 --- a/gcc/cp/typeck2.c +++ b/gcc/cp/typeck2.c @@ -1488,17 +1488,6 @@ process_init_constructor_array (tree type, tree init, int nested, int flags, = massage_init_elt (TREE_TYPE (type), ce->value, nested, flags, complain); - if (TREE_CODE (ce->value) == CONSTRUCTOR - && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value)) - { - /* Shift CONSTRUCTOR_PLACEHOLDER_BOUNDARY from the element initializer - up to the array initializer, so that the call to - replace_placeholders from store_init_value can resolve any - PLACEHOLDER_EXPRs inside this element initializer. */ - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value) = 0; - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; - } - gcc_checking_assert (ce->value == error_mark_node || (same_type_ignoring_top_level_qualifiers_p @@ -1527,13 +1516,6 @@ process_init_constructor_array (tree type, tree init, int nested, int flags, /* The default zero-initialization is fine for us; don't add anything to the CONSTRUCTOR. */ next = NULL_TREE; - else if (TREE_CODE (next) == CONSTRUCTOR - && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next)) - { - /* As above. */ - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0; - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; - } } else if (!zero_init_p (TREE_TYPE (type))) next = build_zero_init (TREE_TYPE (type), diff --git a/gcc/testsuite/g++.dg/cpp1y/pr90996.C b/gcc/testsuite/g++.dg/cpp1y/pr90996.C index 780cbb4e3ac..eff5b62db28 100644 --- a/gcc/testsuite/g++.dg/cpp1y/pr90996.C +++ b/gcc/testsuite/g++.dg/cpp1y/pr90996.C @@ -1,5 +1,5 @@ // PR c++/90996 -// { dg-do compile { target c++14 } } +// { dg-do run { target c++14 } } struct S { @@ -15,3 +15,20 @@ struct T }; T d {}; + +int +main() +{ + if (++c[0][0].b[0] != 6 + || ++c[0][1].b[0] != 3 + || ++c[1][0].b[0] != 3 + || ++c[1][1].b[0] != 3) + __builtin_abort(); + + auto& e = d.c; + if (++e[0][0].b[0] != 8 + || ++e[0][1].b[0] != 3 + || ++e[1][0].b[0] != 3 + || ++e[1][1].b[0] != 3) + __builtin_abort(); +}
On 4/15/20 1:30 PM, Martin Sebor wrote: > On 4/13/20 8:43 PM, Jason Merrill wrote: >> On 4/12/20 5:49 PM, Martin Sebor wrote: >>> On 4/10/20 8:52 AM, Jason Merrill wrote: >>>> On 4/9/20 4:23 PM, Martin Sebor wrote: >>>>> On 4/9/20 1:32 PM, Jason Merrill wrote: >>>>>> On 4/9/20 3:24 PM, Martin Sebor wrote: >>>>>>> On 4/9/20 1:03 PM, Jason Merrill wrote: >>>>>>>> On 4/8/20 1:23 PM, Martin Sebor wrote: >>>>>>>>> On 4/7/20 3:36 PM, Marek Polacek wrote: >>>>>>>>>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>>>>>>>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>>>>>>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>>>>>>>>>> Gcc-patches wrote: >>>>>>>>>>>>> Among the numerous regressions introduced by the change >>>>>>>>>>>>> committed >>>>>>>>>>>>> to GCC 9 to allow string literals as template arguments is >>>>>>>>>>>>> a failure >>>>>>>>>>>>> to recognize the C++ nullptr and GCC's __null constants as >>>>>>>>>>>>> pointers. >>>>>>>>>>>>> For one, I didn't realize that nullptr, being a null >>>>>>>>>>>>> pointer constant, >>>>>>>>>>>>> doesn't have a pointer type, and two, I didn't think of >>>>>>>>>>>>> __null (which >>>>>>>>>>>>> is a special integer constant that NULL sometimes expands to). >>>>>>>>>>>>> >>>>>>>>>>>>> The attached patch adjusts the special handling of trailing >>>>>>>>>>>>> zero >>>>>>>>>>>>> initializers in reshape_init_array_1 to recognize both >>>>>>>>>>>>> kinds of >>>>>>>>>>>>> constants and avoid treating them as zeros of the array >>>>>>>>>>>>> integer >>>>>>>>>>>>> element type. This restores the expected diagnostics when >>>>>>>>>>>>> either >>>>>>>>>>>>> constant is used in the initializer list. >>>>>>>>>>>>> >>>>>>>>>>>>> Martin >>>>>>>>>>>> >>>>>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>>>>> std::array >>>>>>>>>>>>> >>>>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>>>> >>>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches >>>>>>>>>>>>> with all kinds >>>>>>>>>>>>> of pointers. >>>>>>>>>>>>> >>>>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>>>> >>>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>>> * g++.dg/init/array57.C: New test. >>>>>>>>>>>>> * g++.dg/init/array58.C: New test. >>>>>>>>>>>>> >>>>>>>>>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>>>>>>>>> index a127734af69..692c8ed73f4 100644 >>>>>>>>>>>>> --- a/gcc/cp/decl.c >>>>>>>>>>>>> +++ b/gcc/cp/decl.c >>>>>>>>>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, >>>>>>>>>>>>> tree max_index, reshape_iter *d, >>>>>>>>>>>>> TREE_CONSTANT (new_init) = false; >>>>>>>>>>>>> /* Pointers initialized to strings must be treated >>>>>>>>>>>>> as non-zero >>>>>>>>>>>>> - even if the string is empty. */ >>>>>>>>>>>>> + even if the string is empty. Handle all kinds of >>>>>>>>>>>>> pointers, >>>>>>>>>>>>> + including std::nullptr and GCC's __nullptr, neither >>>>>>>>>>>>> of which >>>>>>>>>>>>> + has a pointer type. */ >>>>>>>>>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>>>>>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P >>>>>>>>>>>>> (init_type) >>>>>>>>>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>>>>>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>>>>>>>>> + || null_node_p (elt_init)); >>>>>>>>>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>>>>>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>>>>>>>>> last_nonzero = index; >>>>>>>>>>>> >>>>>>>>>>>> It looks like this still won't handle e.g. pointers to >>>>>>>>>>>> member functions, >>>>>>>>>>>> e.g. >>>>>>>>>>>> >>>>>>>>>>>> struct S { }; >>>>>>>>>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>>>>>>>>> >>>>>>>>>>>> would still be accepted. You could use TYPE_PTR_OR_PTRMEM_P >>>>>>>>>>>> instead of >>>>>>>>>>>> POINTER_TYPE_P to catch this case. >>>>>>>>>>> >>>>>>>>>>> Good catch! That doesn't fail because unlike null data >>>>>>>>>>> member pointers >>>>>>>>>>> which are represented as -1, member function pointers are >>>>>>>>>>> represented >>>>>>>>>>> as a zero. >>>>>>>>>>> >>>>>>>>>>> I had looked for an API that would answer the question: "is this >>>>>>>>>>> expression a pointer?" without having to think of all the >>>>>>>>>>> different >>>>>>>>>>> kinds of them but all I could find was null_node_p(). Is >>>>>>>>>>> this a rare, >>>>>>>>>>> isolated case that having an API like that wouldn't be worth >>>>>>>>>>> having >>>>>>>>>>> or should I add one like in the attached update? >>>>>>>>>>> >>>>>>>>>>> Martin >>>>>>>>>> >>>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>>> std::array >>>>>>>>>>> >>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>> >>>>>>>>>>> PR c++/94510 >>>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with >>>>>>>>>>> all kinds >>>>>>>>>>> of pointers. >>>>>>>>>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. >>>>>>>>>> >>>>>>>>>> (Drop the gcc/cp/.) >>>>>>>>>> >>>>>>>>>>> +/* Returns true if EXPR is a null pointer constant of any >>>>>>>>>>> type. */ >>>>>>>>>>> + >>>>>>>>>>> +inline bool >>>>>>>>>>> +null_pointer_constant_p (tree expr) >>>>>>>>>>> +{ >>>>>>>>>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>>>>>>>>> + if (expr == null_node) >>>>>>>>>>> + return true; >>>>>>>>>>> + tree type = TREE_TYPE (expr); >>>>>>>>>>> + if (NULLPTR_TYPE_P (type)) >>>>>>>>>>> + return true; >>>>>>>>>>> + if (POINTER_TYPE_P (type)) >>>>>>>>>>> + return integer_zerop (expr); >>>>>>>>>>> + return null_member_pointer_value_p (expr); >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>> >>>>>>>>>> We already have a null_ptr_cst_p so it would be sort of >>>>>>>>>> confusing to have >>>>>>>>>> this as well. But are you really interested in whether it's a >>>>>>>>>> null pointer, >>>>>>>>>> not just a pointer? >>>>>>>>> >>>>>>>>> The goal of the code is to detect a mismatch in "pointerness" >>>>>>>>> between >>>>>>>>> an initializer expression and the type of the initialized >>>>>>>>> element, so >>>>>>>>> it needs to know if the expression is a pointer (non-nulls >>>>>>>>> pointers >>>>>>>>> are detected in type_initializer_zero_p). That means testing a >>>>>>>>> number >>>>>>>>> of IMO unintuitive conditions: >>>>>>>>> >>>>>>>>> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >>>>>>>>> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >>>>>>>>> || null_node_p (expr) >>>>>>>>> >>>>>>>>> I don't know if this type of a query is common in the C++ FE >>>>>>>>> but unless >>>>>>>>> this is an isolated use case then besides fixing the bug I >>>>>>>>> thought it >>>>>>>>> would be nice to make it easier to get the test above right, or >>>>>>>>> at least >>>>>>>>> come close to it. >>>>>>>>> >>>>>>>>> Since null_pointer_constant_p already exists (but isn't >>>>>>>>> suitable here >>>>>>>>> because it returns true for plain literal zeros) >>>>>>>> >>>>>>>> Why is that unsuitable? A literal zero is a perfectly good >>>>>>>> zero-initializer for a pointer. >>>>>>> >>>>>>> Right, that's why it's not suitable here. Because a literal zero >>>>>>> is also not a pointer. >>>>>>> >>>>>>> The question the code asks is: "is the initializer expression >>>>>>> a pointer (of any kind)?" >>>>>> >>>>>> Why is that a question we want to ask? What we need here is to >>>>>> know whether the initializer expression is equivalent to implicit >>>>>> zero-initialization. For initializing a pointer, a literal 0 is >>>>>> equivalent, so we don't want to update last_nonzero. >>>>> >>>>> Yes, but that's not the bug we're fixing. The problem occurs with >>>>> an integer array and a pointer initializer: >>>>> >>>>> int a[2] = { nullptr, 0 }; >>>> >>>> Aha, you're fixing a different bug than the one I was seeing. >>> >>> What is that one? (I'm not aware of any others in this area.) >>> >>>> >>>>> and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) >>>>> the test >>>>> >>>>> POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>>> >>>>> evaluates to false because neither type is a pointer type and >>>>> >>>>> type_initializer_zero_p (elt_type, elt_init) >>>>> >>>>> returns true because nullptr is zero, and so last_nonzero doesn't >>>>> get set, the element gets trimmed, and the invalid initialization >>>>> of int with nullptr isn't diagnosed. >>>>> >>>>> But I'm not sure if you're questioning the current code, the simple >>>>> fix quoted above, or my assertion that null_pointer_constant_p would >>>>> not be a suitable function to call to tell if an initializer is >>>>> nullptr vs plain zero. >>>>> >>>>>> Also, why is the pointer check here rather than part of the >>>>>> POINTER_TYPE_P handling in type_initializer_zero_p? >>>>> >>>>> type_initializer_zero_p is implemented in terms of initializer_zerop >>>>> with the only difference that empty strings are considered to be zero >>>>> only for char arrays and not char pointers. >>>> >>>> Yeah, but that's the fundamental problem: We're assuming that any >>>> zero is suitable for initializing any type except for a few >>>> exceptions, and adding more exceptions when we find a new testcase >>>> that breaks. >>>> >>>> Handling this in process_init_constructor_array avoids all these >>>> problems by looking at the initializers after they've been converted >>>> to the desired type, at which point it's much clearer whether they >>>> are zero or not; then we don't need type_initializer_zero_p because >>>> the initializer already has the proper type and for zero_init_p >>>> types we can just use initializer_zero_p. >>> >>> I've already expressed my concerns with that change but if you are >>> comfortable with it I won't insist on waiting until GCC 11. Your last >>> request for that patch was to rework the second loop to avoid changing >>> the counter of the previous loop. The attached update does that. >>> >>> I also added another C++ 2a test to exercise a few more cases with >>> pointers to members. With it I ran into what looks like an unrelated >>> bug in this area. I opened PR 94568 for it, CC'd you, and xfailed >>> the problem case in the new test. >>> >>>> >>>> We do probably want some function that tests whether a particular >>>> initializer is equivalent to zero-initialization, which is either >>>> initializer_zero_p for zero_init_p types, !expr for pointers to >>>> members, and recursing for aggregates. Maybe cp_initializer_zero_p >>>> or zero_init_expr_p? >>>> >>>>> It could be changed to return false for incompatible initializers >>>>> like pointers (or even __null) for non-pointer types, even if they >>>>> are zero, but that's not what it's designed to do. >>>> >>>> But that's exactly what we did for 90938. Now you're proposing >>>> another small exception, only putting it in the caller instead. I >>>> think we'll keep running into these problems until we fix the design >>>> issue. >>> >>> Somehow that felt different. But I don't have a problem with moving >>> the pointer check there as well. It shouldn't be too much more >>> intrusive than the original patch for this bug if you decide to >>> go with it for now. >>> >>>> >>>> It would also be possible to improve things by doing the conversion >>>> in type_initializer_zero_p before considering its zeroness, but that >>>> would again be duplicating work that we're already doing elsewhere. >>> >>> I agree that it's not worth the trouble given the long-term fix is >>> in process_init_constructor_array. >>> >>> Attached is the updated patch with the process_init_constructor_array >>> changes, retested on x86_64-linux. >> >>> + if (!trunc_zero || !type_initializer_zero_p (eltype, ce->value)) >>> + last_nonzero = i; >> >> I think we can remove type_initializer_zero_p as well, and use >> initializer_zerop here. >> >>> + if (last_nonzero < i - 1) >>> + { >>> + vec_safe_truncate (v, last_nonzero + 1); >> >> This looks like you will never truncate to length 0, which seems like >> a problem with last_nonzero being both unsigned and an index; perhaps >> it should be something like num_to_keep? > > This whole block appears to serve no real purpose. It trims trailing > zeros only from arithmetic types, but the trimming only matters for > pointers to members and that's done later. I've removed it. Why doesn't it matter for arithmetic types? Because mangling omits trailing initializer_zerop elements, so the mangling is the same either way, and comparing class-type template arguments is based on mangling? In that case, why don't we do the same for pointers to members?
On 4/17/20 12:19 AM, Jason Merrill wrote: > On 4/15/20 1:30 PM, Martin Sebor wrote: >> On 4/13/20 8:43 PM, Jason Merrill wrote: >>> On 4/12/20 5:49 PM, Martin Sebor wrote: >>>> On 4/10/20 8:52 AM, Jason Merrill wrote: >>>>> On 4/9/20 4:23 PM, Martin Sebor wrote: >>>>>> On 4/9/20 1:32 PM, Jason Merrill wrote: >>>>>>> On 4/9/20 3:24 PM, Martin Sebor wrote: >>>>>>>> On 4/9/20 1:03 PM, Jason Merrill wrote: >>>>>>>>> On 4/8/20 1:23 PM, Martin Sebor wrote: >>>>>>>>>> On 4/7/20 3:36 PM, Marek Polacek wrote: >>>>>>>>>>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>>>>>>>>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>>>>>>>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>>>>>>>>>>> Gcc-patches wrote: >>>>>>>>>>>>>> Among the numerous regressions introduced by the change >>>>>>>>>>>>>> committed >>>>>>>>>>>>>> to GCC 9 to allow string literals as template arguments is >>>>>>>>>>>>>> a failure >>>>>>>>>>>>>> to recognize the C++ nullptr and GCC's __null constants as >>>>>>>>>>>>>> pointers. >>>>>>>>>>>>>> For one, I didn't realize that nullptr, being a null >>>>>>>>>>>>>> pointer constant, >>>>>>>>>>>>>> doesn't have a pointer type, and two, I didn't think of >>>>>>>>>>>>>> __null (which >>>>>>>>>>>>>> is a special integer constant that NULL sometimes expands >>>>>>>>>>>>>> to). >>>>>>>>>>>>>> >>>>>>>>>>>>>> The attached patch adjusts the special handling of >>>>>>>>>>>>>> trailing zero >>>>>>>>>>>>>> initializers in reshape_init_array_1 to recognize both >>>>>>>>>>>>>> kinds of >>>>>>>>>>>>>> constants and avoid treating them as zeros of the array >>>>>>>>>>>>>> integer >>>>>>>>>>>>>> element type. This restores the expected diagnostics when >>>>>>>>>>>>>> either >>>>>>>>>>>>>> constant is used in the initializer list. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Martin >>>>>>>>>>>>> >>>>>>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>>>>>> std::array >>>>>>>>>>>>>> >>>>>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>>>>> >>>>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches >>>>>>>>>>>>>> with all kinds >>>>>>>>>>>>>> of pointers. >>>>>>>>>>>>>> >>>>>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>>>>> >>>>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>>>> * g++.dg/init/array57.C: New test. >>>>>>>>>>>>>> * g++.dg/init/array58.C: New test. >>>>>>>>>>>>>> >>>>>>>>>>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>>>>>>>>>> index a127734af69..692c8ed73f4 100644 >>>>>>>>>>>>>> --- a/gcc/cp/decl.c >>>>>>>>>>>>>> +++ b/gcc/cp/decl.c >>>>>>>>>>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree >>>>>>>>>>>>>> elt_type, tree max_index, reshape_iter *d, >>>>>>>>>>>>>> TREE_CONSTANT (new_init) = false; >>>>>>>>>>>>>> /* Pointers initialized to strings must be >>>>>>>>>>>>>> treated as non-zero >>>>>>>>>>>>>> - even if the string is empty. */ >>>>>>>>>>>>>> + even if the string is empty. Handle all kinds of >>>>>>>>>>>>>> pointers, >>>>>>>>>>>>>> + including std::nullptr and GCC's __nullptr, neither >>>>>>>>>>>>>> of which >>>>>>>>>>>>>> + has a pointer type. */ >>>>>>>>>>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>>>>>>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P >>>>>>>>>>>>>> (init_type) >>>>>>>>>>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>>>>>>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>>>>>>>>>> + || null_node_p (elt_init)); >>>>>>>>>>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>>>>>>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>>>>>>>>>> last_nonzero = index; >>>>>>>>>>>>> >>>>>>>>>>>>> It looks like this still won't handle e.g. pointers to >>>>>>>>>>>>> member functions, >>>>>>>>>>>>> e.g. >>>>>>>>>>>>> >>>>>>>>>>>>> struct S { }; >>>>>>>>>>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>>>>>>>>>> >>>>>>>>>>>>> would still be accepted. You could use >>>>>>>>>>>>> TYPE_PTR_OR_PTRMEM_P instead of >>>>>>>>>>>>> POINTER_TYPE_P to catch this case. >>>>>>>>>>>> >>>>>>>>>>>> Good catch! That doesn't fail because unlike null data >>>>>>>>>>>> member pointers >>>>>>>>>>>> which are represented as -1, member function pointers are >>>>>>>>>>>> represented >>>>>>>>>>>> as a zero. >>>>>>>>>>>> >>>>>>>>>>>> I had looked for an API that would answer the question: "is >>>>>>>>>>>> this >>>>>>>>>>>> expression a pointer?" without having to think of all the >>>>>>>>>>>> different >>>>>>>>>>>> kinds of them but all I could find was null_node_p(). Is >>>>>>>>>>>> this a rare, >>>>>>>>>>>> isolated case that having an API like that wouldn't be worth >>>>>>>>>>>> having >>>>>>>>>>>> or should I add one like in the attached update? >>>>>>>>>>>> >>>>>>>>>>>> Martin >>>>>>>>>>> >>>>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>>>> std::array >>>>>>>>>>>> >>>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>>> >>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches with >>>>>>>>>>>> all kinds >>>>>>>>>>>> of pointers. >>>>>>>>>>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New function. >>>>>>>>>>> >>>>>>>>>>> (Drop the gcc/cp/.) >>>>>>>>>>> >>>>>>>>>>>> +/* Returns true if EXPR is a null pointer constant of any >>>>>>>>>>>> type. */ >>>>>>>>>>>> + >>>>>>>>>>>> +inline bool >>>>>>>>>>>> +null_pointer_constant_p (tree expr) >>>>>>>>>>>> +{ >>>>>>>>>>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>>>>>>>>>> + if (expr == null_node) >>>>>>>>>>>> + return true; >>>>>>>>>>>> + tree type = TREE_TYPE (expr); >>>>>>>>>>>> + if (NULLPTR_TYPE_P (type)) >>>>>>>>>>>> + return true; >>>>>>>>>>>> + if (POINTER_TYPE_P (type)) >>>>>>>>>>>> + return integer_zerop (expr); >>>>>>>>>>>> + return null_member_pointer_value_p (expr); >>>>>>>>>>>> +} >>>>>>>>>>>> + >>>>>>>>>>> >>>>>>>>>>> We already have a null_ptr_cst_p so it would be sort of >>>>>>>>>>> confusing to have >>>>>>>>>>> this as well. But are you really interested in whether it's >>>>>>>>>>> a null pointer, >>>>>>>>>>> not just a pointer? >>>>>>>>>> >>>>>>>>>> The goal of the code is to detect a mismatch in "pointerness" >>>>>>>>>> between >>>>>>>>>> an initializer expression and the type of the initialized >>>>>>>>>> element, so >>>>>>>>>> it needs to know if the expression is a pointer (non-nulls >>>>>>>>>> pointers >>>>>>>>>> are detected in type_initializer_zero_p). That means testing >>>>>>>>>> a number >>>>>>>>>> of IMO unintuitive conditions: >>>>>>>>>> >>>>>>>>>> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >>>>>>>>>> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >>>>>>>>>> || null_node_p (expr) >>>>>>>>>> >>>>>>>>>> I don't know if this type of a query is common in the C++ FE >>>>>>>>>> but unless >>>>>>>>>> this is an isolated use case then besides fixing the bug I >>>>>>>>>> thought it >>>>>>>>>> would be nice to make it easier to get the test above right, >>>>>>>>>> or at least >>>>>>>>>> come close to it. >>>>>>>>>> >>>>>>>>>> Since null_pointer_constant_p already exists (but isn't >>>>>>>>>> suitable here >>>>>>>>>> because it returns true for plain literal zeros) >>>>>>>>> >>>>>>>>> Why is that unsuitable? A literal zero is a perfectly good >>>>>>>>> zero-initializer for a pointer. >>>>>>>> >>>>>>>> Right, that's why it's not suitable here. Because a literal zero >>>>>>>> is also not a pointer. >>>>>>>> >>>>>>>> The question the code asks is: "is the initializer expression >>>>>>>> a pointer (of any kind)?" >>>>>>> >>>>>>> Why is that a question we want to ask? What we need here is to >>>>>>> know whether the initializer expression is equivalent to implicit >>>>>>> zero-initialization. For initializing a pointer, a literal 0 is >>>>>>> equivalent, so we don't want to update last_nonzero. >>>>>> >>>>>> Yes, but that's not the bug we're fixing. The problem occurs with >>>>>> an integer array and a pointer initializer: >>>>>> >>>>>> int a[2] = { nullptr, 0 }; >>>>> >>>>> Aha, you're fixing a different bug than the one I was seeing. >>>> >>>> What is that one? (I'm not aware of any others in this area.) >>>> >>>>> >>>>>> and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) >>>>>> the test >>>>>> >>>>>> POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>>>> >>>>>> evaluates to false because neither type is a pointer type and >>>>>> >>>>>> type_initializer_zero_p (elt_type, elt_init) >>>>>> >>>>>> returns true because nullptr is zero, and so last_nonzero doesn't >>>>>> get set, the element gets trimmed, and the invalid initialization >>>>>> of int with nullptr isn't diagnosed. >>>>>> >>>>>> But I'm not sure if you're questioning the current code, the simple >>>>>> fix quoted above, or my assertion that null_pointer_constant_p would >>>>>> not be a suitable function to call to tell if an initializer is >>>>>> nullptr vs plain zero. >>>>>> >>>>>>> Also, why is the pointer check here rather than part of the >>>>>>> POINTER_TYPE_P handling in type_initializer_zero_p? >>>>>> >>>>>> type_initializer_zero_p is implemented in terms of initializer_zerop >>>>>> with the only difference that empty strings are considered to be zero >>>>>> only for char arrays and not char pointers. >>>>> >>>>> Yeah, but that's the fundamental problem: We're assuming that any >>>>> zero is suitable for initializing any type except for a few >>>>> exceptions, and adding more exceptions when we find a new testcase >>>>> that breaks. >>>>> >>>>> Handling this in process_init_constructor_array avoids all these >>>>> problems by looking at the initializers after they've been >>>>> converted to the desired type, at which point it's much clearer >>>>> whether they are zero or not; then we don't need >>>>> type_initializer_zero_p because the initializer already has the >>>>> proper type and for zero_init_p types we can just use >>>>> initializer_zero_p. >>>> >>>> I've already expressed my concerns with that change but if you are >>>> comfortable with it I won't insist on waiting until GCC 11. Your last >>>> request for that patch was to rework the second loop to avoid changing >>>> the counter of the previous loop. The attached update does that. >>>> >>>> I also added another C++ 2a test to exercise a few more cases with >>>> pointers to members. With it I ran into what looks like an unrelated >>>> bug in this area. I opened PR 94568 for it, CC'd you, and xfailed >>>> the problem case in the new test. >>>> >>>>> >>>>> We do probably want some function that tests whether a particular >>>>> initializer is equivalent to zero-initialization, which is either >>>>> initializer_zero_p for zero_init_p types, !expr for pointers to >>>>> members, and recursing for aggregates. Maybe cp_initializer_zero_p >>>>> or zero_init_expr_p? >>>>> >>>>>> It could be changed to return false for incompatible initializers >>>>>> like pointers (or even __null) for non-pointer types, even if they >>>>>> are zero, but that's not what it's designed to do. >>>>> >>>>> But that's exactly what we did for 90938. Now you're proposing >>>>> another small exception, only putting it in the caller instead. I >>>>> think we'll keep running into these problems until we fix the >>>>> design issue. >>>> >>>> Somehow that felt different. But I don't have a problem with moving >>>> the pointer check there as well. It shouldn't be too much more >>>> intrusive than the original patch for this bug if you decide to >>>> go with it for now. >>>> >>>>> >>>>> It would also be possible to improve things by doing the conversion >>>>> in type_initializer_zero_p before considering its zeroness, but >>>>> that would again be duplicating work that we're already doing >>>>> elsewhere. >>>> >>>> I agree that it's not worth the trouble given the long-term fix is >>>> in process_init_constructor_array. >>>> >>>> Attached is the updated patch with the process_init_constructor_array >>>> changes, retested on x86_64-linux. >>> >>>> + if (!trunc_zero || !type_initializer_zero_p (eltype, ce->value)) >>>> + last_nonzero = i; >>> >>> I think we can remove type_initializer_zero_p as well, and use >>> initializer_zerop here. >>> >>>> + if (last_nonzero < i - 1) >>>> + { >>>> + vec_safe_truncate (v, last_nonzero + 1); >>> >>> This looks like you will never truncate to length 0, which seems like >>> a problem with last_nonzero being both unsigned and an index; perhaps >>> it should be something like num_to_keep? >> >> This whole block appears to serve no real purpose. It trims trailing >> zeros only from arithmetic types, but the trimming only matters for >> pointers to members and that's done later. I've removed it. > > Why doesn't it matter for arithmetic types? Because mangling omits > trailing initializer_zerop elements, so the mangling is the same either > way, and comparing class-type template arguments is based on mangling? > > In that case, why don't we do the same for pointers to members? Your little patch seems to work for the problems we're fixing (and even fixes a subset of pr94568 that I mentioned), which is great. But it fails two tests: ! FAIL: g++.dg/abi/mangle71.C (3: +3) ! FAIL: g++.dg/abi/mangle72.C (10: +10) I don't think the mangling of these things is specified yet so maybe that's okay (I mostly guessed when I wrote those tests, and could have very well guessed wrong). Here's one difference in mangle71.C: void f0__ (X<B{{ 0 }}>) { } mangles as _Z4f0__1XIXtl1BtlA3_1AtlS1_Lc0EEtlS1_Lc1EEEEEE by trunk but as _Z4f0__1XIXtl1BtlA3_1AtlS1_EtlS1_Lc1EEEEEE with your patch. I'd say the former is better but I'm not sure. The changes in mangle72.C, for example for void g__ (Y<B{{ }}>) { } void g0_ (Y<B{{ 0 }}>) { } void g00 (Y<B{{ 0, 0 }}>) { } from _Z3g__1YIXtl1BtlA2_M1AA2_iLS3_0EEEEE _Z3g0_1YIXtl1BtlA2_M1AA2_iLS3_0EEEEE _Z3g001YIXtl1BtlA2_M1AA2_iLS3_0EEEEE to _Z3g__1YIXtl1BEEE _Z3g0_1YIXtl1BEEE _Z3g001YIXtl1BEEE look like improvements, and the one for void g0x (Y<B{{ 0, &A::a }}>) { } from _Z3g0x1YIXtl1BtlA2_M1AA2_iLS3_0EEEEE to _Z3g0x1YIXtl1BtlA2_M1AA2_iLS3_0ELS3_0EEEEE looks like it might actually fix a bug (does it?). There are a few others for member pointers that are similar to the above. If these mangling changes look okay to you I'm much more comfortable with a simple, targeted fix like this than with messing around with all of array initialization. Assuming the fix is correct, what bothers me is that it implies that all these problems have been caused by forgetting to consider pointers to members in the fix for pr89833 in r270155, and that all this time since then I've been barking up the wrong tree by patching up the wrong functions. I.e., that none of the stripping of the trailing initializers outside of mangle.c is or ever was necessary. Why did neither of us realize this a year ago? Martin PS I successfully bootstrapped and regtested the patch with just one minor tweak (using UINT_MAX instead of -1 in mangle.c) and with just the two failing tests above. The version I tested is attached.
On 17 April 2020 23:18:05 CEST, Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: + zero-initialialization for its type, taking pointers to members s/initialialization/initialization/
On 4/17/20 5:18 PM, Martin Sebor wrote: > On 4/17/20 12:19 AM, Jason Merrill wrote: >> On 4/15/20 1:30 PM, Martin Sebor wrote: >>> On 4/13/20 8:43 PM, Jason Merrill wrote: >>>> On 4/12/20 5:49 PM, Martin Sebor wrote: >>>>> On 4/10/20 8:52 AM, Jason Merrill wrote: >>>>>> On 4/9/20 4:23 PM, Martin Sebor wrote: >>>>>>> On 4/9/20 1:32 PM, Jason Merrill wrote: >>>>>>>> On 4/9/20 3:24 PM, Martin Sebor wrote: >>>>>>>>> On 4/9/20 1:03 PM, Jason Merrill wrote: >>>>>>>>>> On 4/8/20 1:23 PM, Martin Sebor wrote: >>>>>>>>>>> On 4/7/20 3:36 PM, Marek Polacek wrote: >>>>>>>>>>>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>>>>>>>>>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>>>>>>>>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor via >>>>>>>>>>>>>> Gcc-patches wrote: >>>>>>>>>>>>>>> Among the numerous regressions introduced by the change >>>>>>>>>>>>>>> committed >>>>>>>>>>>>>>> to GCC 9 to allow string literals as template arguments >>>>>>>>>>>>>>> is a failure >>>>>>>>>>>>>>> to recognize the C++ nullptr and GCC's __null constants >>>>>>>>>>>>>>> as pointers. >>>>>>>>>>>>>>> For one, I didn't realize that nullptr, being a null >>>>>>>>>>>>>>> pointer constant, >>>>>>>>>>>>>>> doesn't have a pointer type, and two, I didn't think of >>>>>>>>>>>>>>> __null (which >>>>>>>>>>>>>>> is a special integer constant that NULL sometimes expands >>>>>>>>>>>>>>> to). >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> The attached patch adjusts the special handling of >>>>>>>>>>>>>>> trailing zero >>>>>>>>>>>>>>> initializers in reshape_init_array_1 to recognize both >>>>>>>>>>>>>>> kinds of >>>>>>>>>>>>>>> constants and avoid treating them as zeros of the array >>>>>>>>>>>>>>> integer >>>>>>>>>>>>>>> element type. This restores the expected diagnostics >>>>>>>>>>>>>>> when either >>>>>>>>>>>>>>> constant is used in the initializer list. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Martin >>>>>>>>>>>>>> >>>>>>>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>>>>>>> std::array >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches >>>>>>>>>>>>>>> with all kinds >>>>>>>>>>>>>>> of pointers. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>>>>> * g++.dg/init/array57.C: New test. >>>>>>>>>>>>>>> * g++.dg/init/array58.C: New test. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>>>>>>>>>>> index a127734af69..692c8ed73f4 100644 >>>>>>>>>>>>>>> --- a/gcc/cp/decl.c >>>>>>>>>>>>>>> +++ b/gcc/cp/decl.c >>>>>>>>>>>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree >>>>>>>>>>>>>>> elt_type, tree max_index, reshape_iter *d, >>>>>>>>>>>>>>> TREE_CONSTANT (new_init) = false; >>>>>>>>>>>>>>> /* Pointers initialized to strings must be >>>>>>>>>>>>>>> treated as non-zero >>>>>>>>>>>>>>> - even if the string is empty. */ >>>>>>>>>>>>>>> + even if the string is empty. Handle all kinds of >>>>>>>>>>>>>>> pointers, >>>>>>>>>>>>>>> + including std::nullptr and GCC's __nullptr, neither >>>>>>>>>>>>>>> of which >>>>>>>>>>>>>>> + has a pointer type. */ >>>>>>>>>>>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>>>>>>>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P >>>>>>>>>>>>>>> (init_type) >>>>>>>>>>>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>>>>>>>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>>>>>>>>>>> + || null_node_p (elt_init)); >>>>>>>>>>>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>>>>>>>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>>>>>>>>>>> last_nonzero = index; >>>>>>>>>>>>>> >>>>>>>>>>>>>> It looks like this still won't handle e.g. pointers to >>>>>>>>>>>>>> member functions, >>>>>>>>>>>>>> e.g. >>>>>>>>>>>>>> >>>>>>>>>>>>>> struct S { }; >>>>>>>>>>>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>>>>>>>>>>> >>>>>>>>>>>>>> would still be accepted. You could use >>>>>>>>>>>>>> TYPE_PTR_OR_PTRMEM_P instead of >>>>>>>>>>>>>> POINTER_TYPE_P to catch this case. >>>>>>>>>>>>> >>>>>>>>>>>>> Good catch! That doesn't fail because unlike null data >>>>>>>>>>>>> member pointers >>>>>>>>>>>>> which are represented as -1, member function pointers are >>>>>>>>>>>>> represented >>>>>>>>>>>>> as a zero. >>>>>>>>>>>>> >>>>>>>>>>>>> I had looked for an API that would answer the question: "is >>>>>>>>>>>>> this >>>>>>>>>>>>> expression a pointer?" without having to think of all the >>>>>>>>>>>>> different >>>>>>>>>>>>> kinds of them but all I could find was null_node_p(). Is >>>>>>>>>>>>> this a rare, >>>>>>>>>>>>> isolated case that having an API like that wouldn't be >>>>>>>>>>>>> worth having >>>>>>>>>>>>> or should I add one like in the attached update? >>>>>>>>>>>>> >>>>>>>>>>>>> Martin >>>>>>>>>>>> >>>>>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>>>>> std::array >>>>>>>>>>>>> >>>>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>>>> >>>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches >>>>>>>>>>>>> with all kinds >>>>>>>>>>>>> of pointers. >>>>>>>>>>>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New >>>>>>>>>>>>> function. >>>>>>>>>>>> >>>>>>>>>>>> (Drop the gcc/cp/.) >>>>>>>>>>>> >>>>>>>>>>>>> +/* Returns true if EXPR is a null pointer constant of any >>>>>>>>>>>>> type. */ >>>>>>>>>>>>> + >>>>>>>>>>>>> +inline bool >>>>>>>>>>>>> +null_pointer_constant_p (tree expr) >>>>>>>>>>>>> +{ >>>>>>>>>>>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>>>>>>>>>>> + if (expr == null_node) >>>>>>>>>>>>> + return true; >>>>>>>>>>>>> + tree type = TREE_TYPE (expr); >>>>>>>>>>>>> + if (NULLPTR_TYPE_P (type)) >>>>>>>>>>>>> + return true; >>>>>>>>>>>>> + if (POINTER_TYPE_P (type)) >>>>>>>>>>>>> + return integer_zerop (expr); >>>>>>>>>>>>> + return null_member_pointer_value_p (expr); >>>>>>>>>>>>> +} >>>>>>>>>>>>> + >>>>>>>>>>>> >>>>>>>>>>>> We already have a null_ptr_cst_p so it would be sort of >>>>>>>>>>>> confusing to have >>>>>>>>>>>> this as well. But are you really interested in whether it's >>>>>>>>>>>> a null pointer, >>>>>>>>>>>> not just a pointer? >>>>>>>>>>> >>>>>>>>>>> The goal of the code is to detect a mismatch in "pointerness" >>>>>>>>>>> between >>>>>>>>>>> an initializer expression and the type of the initialized >>>>>>>>>>> element, so >>>>>>>>>>> it needs to know if the expression is a pointer (non-nulls >>>>>>>>>>> pointers >>>>>>>>>>> are detected in type_initializer_zero_p). That means testing >>>>>>>>>>> a number >>>>>>>>>>> of IMO unintuitive conditions: >>>>>>>>>>> >>>>>>>>>>> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >>>>>>>>>>> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >>>>>>>>>>> || null_node_p (expr) >>>>>>>>>>> >>>>>>>>>>> I don't know if this type of a query is common in the C++ FE >>>>>>>>>>> but unless >>>>>>>>>>> this is an isolated use case then besides fixing the bug I >>>>>>>>>>> thought it >>>>>>>>>>> would be nice to make it easier to get the test above right, >>>>>>>>>>> or at least >>>>>>>>>>> come close to it. >>>>>>>>>>> >>>>>>>>>>> Since null_pointer_constant_p already exists (but isn't >>>>>>>>>>> suitable here >>>>>>>>>>> because it returns true for plain literal zeros) >>>>>>>>>> >>>>>>>>>> Why is that unsuitable? A literal zero is a perfectly good >>>>>>>>>> zero-initializer for a pointer. >>>>>>>>> >>>>>>>>> Right, that's why it's not suitable here. Because a literal zero >>>>>>>>> is also not a pointer. >>>>>>>>> >>>>>>>>> The question the code asks is: "is the initializer expression >>>>>>>>> a pointer (of any kind)?" >>>>>>>> >>>>>>>> Why is that a question we want to ask? What we need here is to >>>>>>>> know whether the initializer expression is equivalent to >>>>>>>> implicit zero-initialization. For initializing a pointer, a >>>>>>>> literal 0 is equivalent, so we don't want to update last_nonzero. >>>>>>> >>>>>>> Yes, but that's not the bug we're fixing. The problem occurs with >>>>>>> an integer array and a pointer initializer: >>>>>>> >>>>>>> int a[2] = { nullptr, 0 }; >>>>>> >>>>>> Aha, you're fixing a different bug than the one I was seeing. >>>>> >>>>> What is that one? (I'm not aware of any others in this area.) >>>>> >>>>>> >>>>>>> and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) >>>>>>> the test >>>>>>> >>>>>>> POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>>>>> >>>>>>> evaluates to false because neither type is a pointer type and >>>>>>> >>>>>>> type_initializer_zero_p (elt_type, elt_init) >>>>>>> >>>>>>> returns true because nullptr is zero, and so last_nonzero doesn't >>>>>>> get set, the element gets trimmed, and the invalid initialization >>>>>>> of int with nullptr isn't diagnosed. >>>>>>> >>>>>>> But I'm not sure if you're questioning the current code, the simple >>>>>>> fix quoted above, or my assertion that null_pointer_constant_p would >>>>>>> not be a suitable function to call to tell if an initializer is >>>>>>> nullptr vs plain zero. >>>>>>> >>>>>>>> Also, why is the pointer check here rather than part of the >>>>>>>> POINTER_TYPE_P handling in type_initializer_zero_p? >>>>>>> >>>>>>> type_initializer_zero_p is implemented in terms of initializer_zerop >>>>>>> with the only difference that empty strings are considered to be >>>>>>> zero >>>>>>> only for char arrays and not char pointers. >>>>>> >>>>>> Yeah, but that's the fundamental problem: We're assuming that any >>>>>> zero is suitable for initializing any type except for a few >>>>>> exceptions, and adding more exceptions when we find a new testcase >>>>>> that breaks. >>>>>> >>>>>> Handling this in process_init_constructor_array avoids all these >>>>>> problems by looking at the initializers after they've been >>>>>> converted to the desired type, at which point it's much clearer >>>>>> whether they are zero or not; then we don't need >>>>>> type_initializer_zero_p because the initializer already has the >>>>>> proper type and for zero_init_p types we can just use >>>>>> initializer_zero_p. >>>>> >>>>> I've already expressed my concerns with that change but if you are >>>>> comfortable with it I won't insist on waiting until GCC 11. Your last >>>>> request for that patch was to rework the second loop to avoid changing >>>>> the counter of the previous loop. The attached update does that. >>>>> >>>>> I also added another C++ 2a test to exercise a few more cases with >>>>> pointers to members. With it I ran into what looks like an unrelated >>>>> bug in this area. I opened PR 94568 for it, CC'd you, and xfailed >>>>> the problem case in the new test. >>>>> >>>>>> >>>>>> We do probably want some function that tests whether a particular >>>>>> initializer is equivalent to zero-initialization, which is either >>>>>> initializer_zero_p for zero_init_p types, !expr for pointers to >>>>>> members, and recursing for aggregates. Maybe >>>>>> cp_initializer_zero_p or zero_init_expr_p? >>>>>> >>>>>>> It could be changed to return false for incompatible initializers >>>>>>> like pointers (or even __null) for non-pointer types, even if they >>>>>>> are zero, but that's not what it's designed to do. >>>>>> >>>>>> But that's exactly what we did for 90938. Now you're proposing >>>>>> another small exception, only putting it in the caller instead. I >>>>>> think we'll keep running into these problems until we fix the >>>>>> design issue. >>>>> >>>>> Somehow that felt different. But I don't have a problem with moving >>>>> the pointer check there as well. It shouldn't be too much more >>>>> intrusive than the original patch for this bug if you decide to >>>>> go with it for now. >>>>> >>>>>> >>>>>> It would also be possible to improve things by doing the >>>>>> conversion in type_initializer_zero_p before considering its >>>>>> zeroness, but that would again be duplicating work that we're >>>>>> already doing elsewhere. >>>>> >>>>> I agree that it's not worth the trouble given the long-term fix is >>>>> in process_init_constructor_array. >>>>> >>>>> Attached is the updated patch with the process_init_constructor_array >>>>> changes, retested on x86_64-linux. >>>> >>>>> + if (!trunc_zero || !type_initializer_zero_p (eltype, >>>>> ce->value)) >>>>> + last_nonzero = i; >>>> >>>> I think we can remove type_initializer_zero_p as well, and use >>>> initializer_zerop here. >>>> >>>>> + if (last_nonzero < i - 1) >>>>> + { >>>>> + vec_safe_truncate (v, last_nonzero + 1); >>>> >>>> This looks like you will never truncate to length 0, which seems >>>> like a problem with last_nonzero being both unsigned and an index; >>>> perhaps it should be something like num_to_keep? >>> >>> This whole block appears to serve no real purpose. It trims trailing >>> zeros only from arithmetic types, but the trimming only matters for >>> pointers to members and that's done later. I've removed it. >> >> Why doesn't it matter for arithmetic types? Because mangling omits >> trailing initializer_zerop elements, so the mangling is the same >> either way, and comparing class-type template arguments is based on >> mangling? >> >> In that case, why don't we do the same for pointers to members? > > Your little patch seems to work for the problems we're fixing (and > even fixes a subset of pr94568 that I mentioned), which is great. > But it fails two tests: > > ! FAIL: g++.dg/abi/mangle71.C (3: +3) > ! FAIL: g++.dg/abi/mangle72.C (10: +10) > > I don't think the mangling of these things is specified yet so maybe > that's okay (I mostly guessed when I wrote those tests, and could > have very well guessed wrong). Here's one difference in mangle71.C: > > void f0__ (X<B{{ 0 }}>) { } > > mangles as > > _Z4f0__1XIXtl1BtlA3_1AtlS1_Lc0EEtlS1_Lc1EEEEEE > > by trunk but as > > _Z4f0__1XIXtl1BtlA3_1AtlS1_EtlS1_Lc1EEEEEE > > with your patch. I'd say the former is better but I'm not sure. I agree. > The changes in mangle72.C, for example for > > void g__ (Y<B{{ }}>) { } > void g0_ (Y<B{{ 0 }}>) { } > void g00 (Y<B{{ 0, 0 }}>) { } > > from > > _Z3g__1YIXtl1BtlA2_M1AA2_iLS3_0EEEEE > _Z3g0_1YIXtl1BtlA2_M1AA2_iLS3_0EEEEE > _Z3g001YIXtl1BtlA2_M1AA2_iLS3_0EEEEE > > to > > _Z3g__1YIXtl1BEEE > _Z3g0_1YIXtl1BEEE > _Z3g001YIXtl1BEEE > > look like improvements, and the one for > > void g0x (Y<B{{ 0, &A::a }}>) { } > > from > > _Z3g0x1YIXtl1BtlA2_M1AA2_iLS3_0EEEEE > > to > > _Z3g0x1YIXtl1BtlA2_M1AA2_iLS3_0ELS3_0EEEEE > > looks like it might actually fix a bug (does it?). There are > a few others for member pointers that are similar to the above. Yes. > If these mangling changes look okay to you I'm much more comfortable > with a simple, targeted fix like this than with messing around with > all of array initialization. Agreed. > Assuming the fix is correct, what bothers me is that it implies > that all these problems have been caused by forgetting to consider > pointers to members in the fix for pr89833 in r270155, and that all > this time since then I've been barking up the wrong tree by patching > up the wrong functions. I.e., that none of the stripping of > the trailing initializers outside of mangle.c is or ever was > necessary. Why did neither of us realize this a year ago? Yeah, that is frustrating. I fixed the mangle71.C issue by not doing any pruning for a non-trivial type. I also fixed another bug whereby the first function with a particular pointer to member in its signature (i.e. g0x) got mangled properly, but the next did not. And realized that our mangling of class non-type template args still needs a lot of work, but that will wait. Any comments before I check this in?
On 4/21/20 2:33 PM, Jason Merrill wrote: > On 4/17/20 5:18 PM, Martin Sebor wrote: >> On 4/17/20 12:19 AM, Jason Merrill wrote: >>> On 4/15/20 1:30 PM, Martin Sebor wrote: >>>> On 4/13/20 8:43 PM, Jason Merrill wrote: >>>>> On 4/12/20 5:49 PM, Martin Sebor wrote: >>>>>> On 4/10/20 8:52 AM, Jason Merrill wrote: >>>>>>> On 4/9/20 4:23 PM, Martin Sebor wrote: >>>>>>>> On 4/9/20 1:32 PM, Jason Merrill wrote: >>>>>>>>> On 4/9/20 3:24 PM, Martin Sebor wrote: >>>>>>>>>> On 4/9/20 1:03 PM, Jason Merrill wrote: >>>>>>>>>>> On 4/8/20 1:23 PM, Martin Sebor wrote: >>>>>>>>>>>> On 4/7/20 3:36 PM, Marek Polacek wrote: >>>>>>>>>>>>> On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor wrote: >>>>>>>>>>>>>> On 4/7/20 1:50 PM, Marek Polacek wrote: >>>>>>>>>>>>>>> On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor >>>>>>>>>>>>>>> via Gcc-patches wrote: >>>>>>>>>>>>>>>> Among the numerous regressions introduced by the change >>>>>>>>>>>>>>>> committed >>>>>>>>>>>>>>>> to GCC 9 to allow string literals as template arguments >>>>>>>>>>>>>>>> is a failure >>>>>>>>>>>>>>>> to recognize the C++ nullptr and GCC's __null constants >>>>>>>>>>>>>>>> as pointers. >>>>>>>>>>>>>>>> For one, I didn't realize that nullptr, being a null >>>>>>>>>>>>>>>> pointer constant, >>>>>>>>>>>>>>>> doesn't have a pointer type, and two, I didn't think of >>>>>>>>>>>>>>>> __null (which >>>>>>>>>>>>>>>> is a special integer constant that NULL sometimes >>>>>>>>>>>>>>>> expands to). >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> The attached patch adjusts the special handling of >>>>>>>>>>>>>>>> trailing zero >>>>>>>>>>>>>>>> initializers in reshape_init_array_1 to recognize both >>>>>>>>>>>>>>>> kinds of >>>>>>>>>>>>>>>> constants and avoid treating them as zeros of the array >>>>>>>>>>>>>>>> integer >>>>>>>>>>>>>>>> element type. This restores the expected diagnostics >>>>>>>>>>>>>>>> when either >>>>>>>>>>>>>>>> constant is used in the initializer list. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Martin >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice >>>>>>>>>>>>>>>> in std::array >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches >>>>>>>>>>>>>>>> with all kinds >>>>>>>>>>>>>>>> of pointers. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>>>>>> * g++.dg/init/array57.C: New test. >>>>>>>>>>>>>>>> * g++.dg/init/array58.C: New test. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c >>>>>>>>>>>>>>>> index a127734af69..692c8ed73f4 100644 >>>>>>>>>>>>>>>> --- a/gcc/cp/decl.c >>>>>>>>>>>>>>>> +++ b/gcc/cp/decl.c >>>>>>>>>>>>>>>> @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree >>>>>>>>>>>>>>>> elt_type, tree max_index, reshape_iter *d, >>>>>>>>>>>>>>>> TREE_CONSTANT (new_init) = false; >>>>>>>>>>>>>>>> /* Pointers initialized to strings must be >>>>>>>>>>>>>>>> treated as non-zero >>>>>>>>>>>>>>>> - even if the string is empty. */ >>>>>>>>>>>>>>>> + even if the string is empty. Handle all kinds of >>>>>>>>>>>>>>>> pointers, >>>>>>>>>>>>>>>> + including std::nullptr and GCC's __nullptr, >>>>>>>>>>>>>>>> neither of which >>>>>>>>>>>>>>>> + has a pointer type. */ >>>>>>>>>>>>>>>> tree init_type = TREE_TYPE (elt_init); >>>>>>>>>>>>>>>> - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P >>>>>>>>>>>>>>>> (init_type) >>>>>>>>>>>>>>>> + bool init_is_ptr = (POINTER_TYPE_P (init_type) >>>>>>>>>>>>>>>> + || NULLPTR_TYPE_P (init_type) >>>>>>>>>>>>>>>> + || null_node_p (elt_init)); >>>>>>>>>>>>>>>> + if (POINTER_TYPE_P (elt_type) != init_is_ptr >>>>>>>>>>>>>>>> || !type_initializer_zero_p (elt_type, elt_init)) >>>>>>>>>>>>>>>> last_nonzero = index; >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> It looks like this still won't handle e.g. pointers to >>>>>>>>>>>>>>> member functions, >>>>>>>>>>>>>>> e.g. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> struct S { }; >>>>>>>>>>>>>>> int arr[3] = { (void (S::*) ()) 0, 0, 0 }; >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> would still be accepted. You could use >>>>>>>>>>>>>>> TYPE_PTR_OR_PTRMEM_P instead of >>>>>>>>>>>>>>> POINTER_TYPE_P to catch this case. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Good catch! That doesn't fail because unlike null data >>>>>>>>>>>>>> member pointers >>>>>>>>>>>>>> which are represented as -1, member function pointers are >>>>>>>>>>>>>> represented >>>>>>>>>>>>>> as a zero. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I had looked for an API that would answer the question: >>>>>>>>>>>>>> "is this >>>>>>>>>>>>>> expression a pointer?" without having to think of all the >>>>>>>>>>>>>> different >>>>>>>>>>>>>> kinds of them but all I could find was null_node_p(). Is >>>>>>>>>>>>>> this a rare, >>>>>>>>>>>>>> isolated case that having an API like that wouldn't be >>>>>>>>>>>>>> worth having >>>>>>>>>>>>>> or should I add one like in the attached update? >>>>>>>>>>>>>> >>>>>>>>>>>>>> Martin >>>>>>>>>>>>> >>>>>>>>>>>>>> PR c++/94510 - nullptr_t implicitly cast to zero twice in >>>>>>>>>>>>>> std::array >>>>>>>>>>>>>> >>>>>>>>>>>>>> gcc/cp/ChangeLog: >>>>>>>>>>>>>> >>>>>>>>>>>>>> PR c++/94510 >>>>>>>>>>>>>> * decl.c (reshape_init_array_1): Exclude mismatches >>>>>>>>>>>>>> with all kinds >>>>>>>>>>>>>> of pointers. >>>>>>>>>>>>>> * gcc/cp/cp-tree.h (null_pointer_constant_p): New >>>>>>>>>>>>>> function. >>>>>>>>>>>>> >>>>>>>>>>>>> (Drop the gcc/cp/.) >>>>>>>>>>>>> >>>>>>>>>>>>>> +/* Returns true if EXPR is a null pointer constant of any >>>>>>>>>>>>>> type. */ >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +inline bool >>>>>>>>>>>>>> +null_pointer_constant_p (tree expr) >>>>>>>>>>>>>> +{ >>>>>>>>>>>>>> + STRIP_ANY_LOCATION_WRAPPER (expr); >>>>>>>>>>>>>> + if (expr == null_node) >>>>>>>>>>>>>> + return true; >>>>>>>>>>>>>> + tree type = TREE_TYPE (expr); >>>>>>>>>>>>>> + if (NULLPTR_TYPE_P (type)) >>>>>>>>>>>>>> + return true; >>>>>>>>>>>>>> + if (POINTER_TYPE_P (type)) >>>>>>>>>>>>>> + return integer_zerop (expr); >>>>>>>>>>>>>> + return null_member_pointer_value_p (expr); >>>>>>>>>>>>>> +} >>>>>>>>>>>>>> + >>>>>>>>>>>>> >>>>>>>>>>>>> We already have a null_ptr_cst_p so it would be sort of >>>>>>>>>>>>> confusing to have >>>>>>>>>>>>> this as well. But are you really interested in whether >>>>>>>>>>>>> it's a null pointer, >>>>>>>>>>>>> not just a pointer? >>>>>>>>>>>> >>>>>>>>>>>> The goal of the code is to detect a mismatch in >>>>>>>>>>>> "pointerness" between >>>>>>>>>>>> an initializer expression and the type of the initialized >>>>>>>>>>>> element, so >>>>>>>>>>>> it needs to know if the expression is a pointer (non-nulls >>>>>>>>>>>> pointers >>>>>>>>>>>> are detected in type_initializer_zero_p). That means >>>>>>>>>>>> testing a number >>>>>>>>>>>> of IMO unintuitive conditions: >>>>>>>>>>>> >>>>>>>>>>>> TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) >>>>>>>>>>>> || NULLPTR_TYPE_P (TREE_TYPE (expr)) >>>>>>>>>>>> || null_node_p (expr) >>>>>>>>>>>> >>>>>>>>>>>> I don't know if this type of a query is common in the C++ FE >>>>>>>>>>>> but unless >>>>>>>>>>>> this is an isolated use case then besides fixing the bug I >>>>>>>>>>>> thought it >>>>>>>>>>>> would be nice to make it easier to get the test above right, >>>>>>>>>>>> or at least >>>>>>>>>>>> come close to it. >>>>>>>>>>>> >>>>>>>>>>>> Since null_pointer_constant_p already exists (but isn't >>>>>>>>>>>> suitable here >>>>>>>>>>>> because it returns true for plain literal zeros) >>>>>>>>>>> >>>>>>>>>>> Why is that unsuitable? A literal zero is a perfectly good >>>>>>>>>>> zero-initializer for a pointer. >>>>>>>>>> >>>>>>>>>> Right, that's why it's not suitable here. Because a literal zero >>>>>>>>>> is also not a pointer. >>>>>>>>>> >>>>>>>>>> The question the code asks is: "is the initializer expression >>>>>>>>>> a pointer (of any kind)?" >>>>>>>>> >>>>>>>>> Why is that a question we want to ask? What we need here is to >>>>>>>>> know whether the initializer expression is equivalent to >>>>>>>>> implicit zero-initialization. For initializing a pointer, a >>>>>>>>> literal 0 is equivalent, so we don't want to update last_nonzero. >>>>>>>> >>>>>>>> Yes, but that's not the bug we're fixing. The problem occurs with >>>>>>>> an integer array and a pointer initializer: >>>>>>>> >>>>>>>> int a[2] = { nullptr, 0 }; >>>>>>> >>>>>>> Aha, you're fixing a different bug than the one I was seeing. >>>>>> >>>>>> What is that one? (I'm not aware of any others in this area.) >>>>>> >>>>>>> >>>>>>>> and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) >>>>>>>> the test >>>>>>>> >>>>>>>> POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) >>>>>>>> >>>>>>>> evaluates to false because neither type is a pointer type and >>>>>>>> >>>>>>>> type_initializer_zero_p (elt_type, elt_init) >>>>>>>> >>>>>>>> returns true because nullptr is zero, and so last_nonzero doesn't >>>>>>>> get set, the element gets trimmed, and the invalid initialization >>>>>>>> of int with nullptr isn't diagnosed. >>>>>>>> >>>>>>>> But I'm not sure if you're questioning the current code, the simple >>>>>>>> fix quoted above, or my assertion that null_pointer_constant_p >>>>>>>> would >>>>>>>> not be a suitable function to call to tell if an initializer is >>>>>>>> nullptr vs plain zero. >>>>>>>> >>>>>>>>> Also, why is the pointer check here rather than part of the >>>>>>>>> POINTER_TYPE_P handling in type_initializer_zero_p? >>>>>>>> >>>>>>>> type_initializer_zero_p is implemented in terms of >>>>>>>> initializer_zerop >>>>>>>> with the only difference that empty strings are considered to be >>>>>>>> zero >>>>>>>> only for char arrays and not char pointers. >>>>>>> >>>>>>> Yeah, but that's the fundamental problem: We're assuming that any >>>>>>> zero is suitable for initializing any type except for a few >>>>>>> exceptions, and adding more exceptions when we find a new >>>>>>> testcase that breaks. >>>>>>> >>>>>>> Handling this in process_init_constructor_array avoids all these >>>>>>> problems by looking at the initializers after they've been >>>>>>> converted to the desired type, at which point it's much clearer >>>>>>> whether they are zero or not; then we don't need >>>>>>> type_initializer_zero_p because the initializer already has the >>>>>>> proper type and for zero_init_p types we can just use >>>>>>> initializer_zero_p. >>>>>> >>>>>> I've already expressed my concerns with that change but if you are >>>>>> comfortable with it I won't insist on waiting until GCC 11. Your >>>>>> last >>>>>> request for that patch was to rework the second loop to avoid >>>>>> changing >>>>>> the counter of the previous loop. The attached update does that. >>>>>> >>>>>> I also added another C++ 2a test to exercise a few more cases with >>>>>> pointers to members. With it I ran into what looks like an unrelated >>>>>> bug in this area. I opened PR 94568 for it, CC'd you, and xfailed >>>>>> the problem case in the new test. >>>>>> >>>>>>> >>>>>>> We do probably want some function that tests whether a particular >>>>>>> initializer is equivalent to zero-initialization, which is either >>>>>>> initializer_zero_p for zero_init_p types, !expr for pointers to >>>>>>> members, and recursing for aggregates. Maybe >>>>>>> cp_initializer_zero_p or zero_init_expr_p? >>>>>>> >>>>>>>> It could be changed to return false for incompatible initializers >>>>>>>> like pointers (or even __null) for non-pointer types, even if they >>>>>>>> are zero, but that's not what it's designed to do. >>>>>>> >>>>>>> But that's exactly what we did for 90938. Now you're proposing >>>>>>> another small exception, only putting it in the caller instead. >>>>>>> I think we'll keep running into these problems until we fix the >>>>>>> design issue. >>>>>> >>>>>> Somehow that felt different. But I don't have a problem with moving >>>>>> the pointer check there as well. It shouldn't be too much more >>>>>> intrusive than the original patch for this bug if you decide to >>>>>> go with it for now. >>>>>> >>>>>>> >>>>>>> It would also be possible to improve things by doing the >>>>>>> conversion in type_initializer_zero_p before considering its >>>>>>> zeroness, but that would again be duplicating work that we're >>>>>>> already doing elsewhere. >>>>>> >>>>>> I agree that it's not worth the trouble given the long-term fix is >>>>>> in process_init_constructor_array. >>>>>> >>>>>> Attached is the updated patch with the process_init_constructor_array >>>>>> changes, retested on x86_64-linux. >>>>> >>>>>> + if (!trunc_zero || !type_initializer_zero_p (eltype, >>>>>> ce->value)) >>>>>> + last_nonzero = i; >>>>> >>>>> I think we can remove type_initializer_zero_p as well, and use >>>>> initializer_zerop here. >>>>> >>>>>> + if (last_nonzero < i - 1) >>>>>> + { >>>>>> + vec_safe_truncate (v, last_nonzero + 1); >>>>> >>>>> This looks like you will never truncate to length 0, which seems >>>>> like a problem with last_nonzero being both unsigned and an index; >>>>> perhaps it should be something like num_to_keep? >>>> >>>> This whole block appears to serve no real purpose. It trims trailing >>>> zeros only from arithmetic types, but the trimming only matters for >>>> pointers to members and that's done later. I've removed it. >>> >>> Why doesn't it matter for arithmetic types? Because mangling omits >>> trailing initializer_zerop elements, so the mangling is the same >>> either way, and comparing class-type template arguments is based on >>> mangling? >>> >>> In that case, why don't we do the same for pointers to members? >> >> Your little patch seems to work for the problems we're fixing (and >> even fixes a subset of pr94568 that I mentioned), which is great. >> But it fails two tests: >> >> ! FAIL: g++.dg/abi/mangle71.C (3: +3) >> ! FAIL: g++.dg/abi/mangle72.C (10: +10) >> >> I don't think the mangling of these things is specified yet so maybe >> that's okay (I mostly guessed when I wrote those tests, and could >> have very well guessed wrong). Here's one difference in mangle71.C: >> >> void f0__ (X<B{{ 0 }}>) { } >> >> mangles as >> >> _Z4f0__1XIXtl1BtlA3_1AtlS1_Lc0EEtlS1_Lc1EEEEEE >> >> by trunk but as >> >> _Z4f0__1XIXtl1BtlA3_1AtlS1_EtlS1_Lc1EEEEEE >> >> with your patch. I'd say the former is better but I'm not sure. > > I agree. > >> The changes in mangle72.C, for example for >> >> void g__ (Y<B{{ }}>) { } >> void g0_ (Y<B{{ 0 }}>) { } >> void g00 (Y<B{{ 0, 0 }}>) { } >> >> from >> >> _Z3g__1YIXtl1BtlA2_M1AA2_iLS3_0EEEEE >> _Z3g0_1YIXtl1BtlA2_M1AA2_iLS3_0EEEEE >> _Z3g001YIXtl1BtlA2_M1AA2_iLS3_0EEEEE >> >> to >> >> _Z3g__1YIXtl1BEEE >> _Z3g0_1YIXtl1BEEE >> _Z3g001YIXtl1BEEE >> >> look like improvements, and the one for >> >> void g0x (Y<B{{ 0, &A::a }}>) { } >> >> from >> >> _Z3g0x1YIXtl1BtlA2_M1AA2_iLS3_0EEEEE >> >> to >> >> _Z3g0x1YIXtl1BtlA2_M1AA2_iLS3_0ELS3_0EEEEE >> >> looks like it might actually fix a bug (does it?). There are >> a few others for member pointers that are similar to the above. > > Yes. > >> If these mangling changes look okay to you I'm much more comfortable >> with a simple, targeted fix like this than with messing around with >> all of array initialization. > > Agreed. > >> Assuming the fix is correct, what bothers me is that it implies >> that all these problems have been caused by forgetting to consider >> pointers to members in the fix for pr89833 in r270155, and that all >> this time since then I've been barking up the wrong tree by patching >> up the wrong functions. I.e., that none of the stripping of >> the trailing initializers outside of mangle.c is or ever was >> necessary. Why did neither of us realize this a year ago? > > Yeah, that is frustrating. > > I fixed the mangle71.C issue by not doing any pruning for a non-trivial > type. I also fixed another bug whereby the first function with a > particular pointer to member in its signature (i.e. g0x) got mangled > properly, but the next did not. And realized that our mangling of class > non-type template args still needs a lot of work, but that will wait. > > Any comments before I check this in? No comments from me. Martin
On Wed, 15 Apr 2020, Patrick Palka wrote: > On Wed, 15 Apr 2020, Martin Sebor via Gcc-patches wrote: > > On 4/13/20 8:43 PM, Jason Merrill wrote: > > > On 4/12/20 5:49 PM, Martin Sebor wrote: > > > > On 4/10/20 8:52 AM, Jason Merrill wrote: > > > > > On 4/9/20 4:23 PM, Martin Sebor wrote: > > > > > > On 4/9/20 1:32 PM, Jason Merrill wrote: > > > > > > > On 4/9/20 3:24 PM, Martin Sebor wrote: > > > > > > > > On 4/9/20 1:03 PM, Jason Merrill wrote: > > > > > > > > > On 4/8/20 1:23 PM, Martin Sebor wrote: > > > > > > > > > > On 4/7/20 3:36 PM, Marek Polacek wrote: > > > > > > > > > > > On Tue, Apr 07, 2020 at 02:46:52PM -0600, Martin Sebor > > > > > > > > > > > wrote: > > > > > > > > > > > > On 4/7/20 1:50 PM, Marek Polacek wrote: > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 12:50:48PM -0600, Martin Sebor > > > > > > > > > > > > > via Gcc-patches wrote: > > > > > > > > > > > > > > Among the numerous regressions introduced by the > > > > > > > > > > > > > > change committed > > > > > > > > > > > > > > to GCC 9 to allow string literals as template > > > > > > > > > > > > > > arguments is a failure > > > > > > > > > > > > > > to recognize the C++ nullptr and GCC's __null > > > > > > > > > > > > > > constants as pointers. > > > > > > > > > > > > > > For one, I didn't realize that nullptr, being a null > > > > > > > > > > > > > > pointer constant, > > > > > > > > > > > > > > doesn't have a pointer type, and two, I didn't think > > > > > > > > > > > > > > of __null (which > > > > > > > > > > > > > > is a special integer constant that NULL sometimes > > > > > > > > > > > > > > expands to). > > > > > > > > > > > > > > > > > > > > > > > > > > > > The attached patch adjusts the special handling of > > > > > > > > > > > > > > trailing zero > > > > > > > > > > > > > > initializers in reshape_init_array_1 to recognize both > > > > > > > > > > > > > > kinds of > > > > > > > > > > > > > > constants and avoid treating them as zeros of the > > > > > > > > > > > > > > array integer > > > > > > > > > > > > > > element type. This restores the expected diagnostics > > > > > > > > > > > > > > when either > > > > > > > > > > > > > > constant is used in the initializer list. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Martin > > > > > > > > > > > > > > > > > > > > > > > > > > > PR c++/94510 - nullptr_t implicitly cast to zero twice > > > > > > > > > > > > > > in std::array > > > > > > > > > > > > > > > > > > > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > > > > > > > > > > > > > > > > > > > PR c++/94510 > > > > > > > > > > > > > > * decl.c (reshape_init_array_1): Exclude > > > > > > > > > > > > > > mismatches with all kinds > > > > > > > > > > > > > > of pointers. > > > > > > > > > > > > > > > > > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > > > > > > > > > > > > > > > > > > > PR c++/94510 > > > > > > > > > > > > > > * g++.dg/init/array57.C: New test. > > > > > > > > > > > > > > * g++.dg/init/array58.C: New test. > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c > > > > > > > > > > > > > > index a127734af69..692c8ed73f4 100644 > > > > > > > > > > > > > > --- a/gcc/cp/decl.c > > > > > > > > > > > > > > +++ b/gcc/cp/decl.c > > > > > > > > > > > > > > @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree > > > > > > > > > > > > > > elt_type, tree max_index, reshape_iter *d, > > > > > > > > > > > > > > TREE_CONSTANT (new_init) = false; > > > > > > > > > > > > > > /* Pointers initialized to strings must be > > > > > > > > > > > > > > treated as non-zero > > > > > > > > > > > > > > - even if the string is empty. */ > > > > > > > > > > > > > > + even if the string is empty. Handle all kinds > > > > > > > > > > > > > > of pointers, > > > > > > > > > > > > > > + including std::nullptr and GCC's __nullptr, > > > > > > > > > > > > > > neither of which > > > > > > > > > > > > > > + has a pointer type. */ > > > > > > > > > > > > > > tree init_type = TREE_TYPE (elt_init); > > > > > > > > > > > > > > - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P > > > > > > > > > > > > > > (init_type) > > > > > > > > > > > > > > + bool init_is_ptr = (POINTER_TYPE_P (init_type) > > > > > > > > > > > > > > + || NULLPTR_TYPE_P (init_type) > > > > > > > > > > > > > > + || null_node_p (elt_init)); > > > > > > > > > > > > > > + if (POINTER_TYPE_P (elt_type) != init_is_ptr > > > > > > > > > > > > > > || !type_initializer_zero_p (elt_type, > > > > > > > > > > > > > > elt_init)) > > > > > > > > > > > > > > last_nonzero = index; > > > > > > > > > > > > > > > > > > > > > > > > > > It looks like this still won't handle e.g. pointers to > > > > > > > > > > > > > member functions, > > > > > > > > > > > > > e.g. > > > > > > > > > > > > > > > > > > > > > > > > > > struct S { }; > > > > > > > > > > > > > int arr[3] = { (void (S::*) ()) 0, 0, 0 }; > > > > > > > > > > > > > > > > > > > > > > > > > > would still be accepted. You could use > > > > > > > > > > > > > TYPE_PTR_OR_PTRMEM_P instead of > > > > > > > > > > > > > POINTER_TYPE_P to catch this case. > > > > > > > > > > > > > > > > > > > > > > > > Good catch! That doesn't fail because unlike null data > > > > > > > > > > > > member pointers > > > > > > > > > > > > which are represented as -1, member function pointers are > > > > > > > > > > > > represented > > > > > > > > > > > > as a zero. > > > > > > > > > > > > > > > > > > > > > > > > I had looked for an API that would answer the question: > > > > > > > > > > > > "is this > > > > > > > > > > > > expression a pointer?" without having to think of all the > > > > > > > > > > > > different > > > > > > > > > > > > kinds of them but all I could find was null_node_p(). Is > > > > > > > > > > > > this a rare, > > > > > > > > > > > > isolated case that having an API like that wouldn't be > > > > > > > > > > > > worth having > > > > > > > > > > > > or should I add one like in the attached update? > > > > > > > > > > > > > > > > > > > > > > > > Martin > > > > > > > > > > > > > > > > > > > > > > > PR c++/94510 - nullptr_t implicitly cast to zero twice in > > > > > > > > > > > > std::array > > > > > > > > > > > > > > > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > > > > > > > > > > > > > > > PR c++/94510 > > > > > > > > > > > > * decl.c (reshape_init_array_1): Exclude mismatches > > > > > > > > > > > > with all kinds > > > > > > > > > > > > of pointers. > > > > > > > > > > > > * gcc/cp/cp-tree.h (null_pointer_constant_p): New > > > > > > > > > > > > function. > > > > > > > > > > > > > > > > > > > > > > (Drop the gcc/cp/.) > > > > > > > > > > > > > > > > > > > > > > > +/* Returns true if EXPR is a null pointer constant of any > > > > > > > > > > > > type. */ > > > > > > > > > > > > + > > > > > > > > > > > > +inline bool > > > > > > > > > > > > +null_pointer_constant_p (tree expr) > > > > > > > > > > > > +{ > > > > > > > > > > > > + STRIP_ANY_LOCATION_WRAPPER (expr); > > > > > > > > > > > > + if (expr == null_node) > > > > > > > > > > > > + return true; > > > > > > > > > > > > + tree type = TREE_TYPE (expr); > > > > > > > > > > > > + if (NULLPTR_TYPE_P (type)) > > > > > > > > > > > > + return true; > > > > > > > > > > > > + if (POINTER_TYPE_P (type)) > > > > > > > > > > > > + return integer_zerop (expr); > > > > > > > > > > > > + return null_member_pointer_value_p (expr); > > > > > > > > > > > > +} > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > > We already have a null_ptr_cst_p so it would be sort of > > > > > > > > > > > confusing to have > > > > > > > > > > > this as well. But are you really interested in whether it's > > > > > > > > > > > a null pointer, > > > > > > > > > > > not just a pointer? > > > > > > > > > > > > > > > > > > > > The goal of the code is to detect a mismatch in "pointerness" > > > > > > > > > > between > > > > > > > > > > an initializer expression and the type of the initialized > > > > > > > > > > element, so > > > > > > > > > > it needs to know if the expression is a pointer (non-nulls > > > > > > > > > > pointers > > > > > > > > > > are detected in type_initializer_zero_p). That means testing > > > > > > > > > > a number > > > > > > > > > > of IMO unintuitive conditions: > > > > > > > > > > > > > > > > > > > > TYPE_PTR_OR_PTRMEM_P (TREE_TYPE (expr)) > > > > > > > > > > || NULLPTR_TYPE_P (TREE_TYPE (expr)) > > > > > > > > > > || null_node_p (expr) > > > > > > > > > > > > > > > > > > > > I don't know if this type of a query is common in the C++ FE > > > > > > > > > > but unless > > > > > > > > > > this is an isolated use case then besides fixing the bug I > > > > > > > > > > thought it > > > > > > > > > > would be nice to make it easier to get the test above right, > > > > > > > > > > or at least > > > > > > > > > > come close to it. > > > > > > > > > > > > > > > > > > > > Since null_pointer_constant_p already exists (but isn't > > > > > > > > > > suitable here > > > > > > > > > > because it returns true for plain literal zeros) > > > > > > > > > > > > > > > > > > Why is that unsuitable? A literal zero is a perfectly good > > > > > > > > > zero-initializer for a pointer. > > > > > > > > > > > > > > > > Right, that's why it's not suitable here. Because a literal zero > > > > > > > > is also not a pointer. > > > > > > > > > > > > > > > > The question the code asks is: "is the initializer expression > > > > > > > > a pointer (of any kind)?" > > > > > > > > > > > > > > Why is that a question we want to ask? What we need here is to know > > > > > > > whether the initializer expression is equivalent to implicit > > > > > > > zero-initialization. For initializing a pointer, a literal 0 is > > > > > > > equivalent, so we don't want to update last_nonzero. > > > > > > > > > > > > Yes, but that's not the bug we're fixing. The problem occurs with > > > > > > an integer array and a pointer initializer: > > > > > > > > > > > > int a[2] = { nullptr, 0 }; > > > > > > > > > > Aha, you're fixing a different bug than the one I was seeing. > > > > > > > > What is that one? (I'm not aware of any others in this area.) > > > > > > > > > > > > > > > and with elt_type = TREE_TYPE (a) and init_type TREE_TYPE (nullptr) > > > > > > the test > > > > > > > > > > > > POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) > > > > > > > > > > > > evaluates to false because neither type is a pointer type and > > > > > > > > > > > > type_initializer_zero_p (elt_type, elt_init) > > > > > > > > > > > > returns true because nullptr is zero, and so last_nonzero doesn't > > > > > > get set, the element gets trimmed, and the invalid initialization > > > > > > of int with nullptr isn't diagnosed. > > > > > > > > > > > > But I'm not sure if you're questioning the current code, the simple > > > > > > fix quoted above, or my assertion that null_pointer_constant_p would > > > > > > not be a suitable function to call to tell if an initializer is > > > > > > nullptr vs plain zero. > > > > > > > > > > > > > Also, why is the pointer check here rather than part of the > > > > > > > POINTER_TYPE_P handling in type_initializer_zero_p? > > > > > > > > > > > > type_initializer_zero_p is implemented in terms of initializer_zerop > > > > > > with the only difference that empty strings are considered to be zero > > > > > > only for char arrays and not char pointers. > > > > > > > > > > Yeah, but that's the fundamental problem: We're assuming that any zero > > > > > is suitable for initializing any type except for a few exceptions, and > > > > > adding more exceptions when we find a new testcase that breaks. > > > > > > > > > > Handling this in process_init_constructor_array avoids all these > > > > > problems by looking at the initializers after they've been converted to > > > > > the desired type, at which point it's much clearer whether they are zero > > > > > or not; then we don't need type_initializer_zero_p because the > > > > > initializer already has the proper type and for zero_init_p types we can > > > > > just use initializer_zero_p. > > > > > > > > I've already expressed my concerns with that change but if you are > > > > comfortable with it I won't insist on waiting until GCC 11. Your last > > > > request for that patch was to rework the second loop to avoid changing > > > > the counter of the previous loop. The attached update does that. > > > > > > > > I also added another C++ 2a test to exercise a few more cases with > > > > pointers to members. With it I ran into what looks like an unrelated > > > > bug in this area. I opened PR 94568 for it, CC'd you, and xfailed > > > > the problem case in the new test. > > > > > > > > > > > > > > We do probably want some function that tests whether a particular > > > > > initializer is equivalent to zero-initialization, which is either > > > > > initializer_zero_p for zero_init_p types, !expr for pointers to members, > > > > > and recursing for aggregates. Maybe cp_initializer_zero_p or > > > > > zero_init_expr_p? > > > > > > > > > > > It could be changed to return false for incompatible initializers > > > > > > like pointers (or even __null) for non-pointer types, even if they > > > > > > are zero, but that's not what it's designed to do. > > > > > > > > > > But that's exactly what we did for 90938. Now you're proposing another > > > > > small exception, only putting it in the caller instead. I think we'll > > > > > keep running into these problems until we fix the design issue. > > > > > > > > Somehow that felt different. But I don't have a problem with moving > > > > the pointer check there as well. It shouldn't be too much more > > > > intrusive than the original patch for this bug if you decide to > > > > go with it for now. > > > > > > > > > > > > > > It would also be possible to improve things by doing the conversion in > > > > > type_initializer_zero_p before considering its zeroness, but that would > > > > > again be duplicating work that we're already doing elsewhere. > > > > > > > > I agree that it's not worth the trouble given the long-term fix is > > > > in process_init_constructor_array. > > > > > > > > Attached is the updated patch with the process_init_constructor_array > > > > changes, retested on x86_64-linux. > > > > > > > + if (!trunc_zero || !type_initializer_zero_p (eltype, ce->value)) > > > > + last_nonzero = i; > > > > > > I think we can remove type_initializer_zero_p as well, and use > > > initializer_zerop here. > > > > > > > + if (last_nonzero < i - 1) > > > > + { > > > > + vec_safe_truncate (v, last_nonzero + 1); > > > > > > This looks like you will never truncate to length 0, which seems like a > > > problem with last_nonzero being both unsigned and an index; perhaps it > > > should be something like num_to_keep? > > > > This whole block appears to serve no real purpose. It trims trailing > > zeros only from arithmetic types, but the trimming only matters for > > pointers to members and that's done later. I've removed it. > > > > > > > > > + len = i = vec_safe_length (v); > > > > + } > > > > > > Nitpick: It seems you don't need to update len or i since you're about to > > > return. > > > > > > > - else if (TREE_CODE (next) == CONSTRUCTOR > > > > - && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next)) > > > > - { > > > > - /* As above. */ > > > > - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0; > > > > - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; > > > > - } > > > > > > This is from the recent fix for 90996, we want to keep it. > > > > Whoops. But no test failed with this change, not even pr90996.C (with > > make check-c++-all). I'm not sure how to write one that does fail. > > Hmm... it looks like the following hunk > > @@ -3247,7 +3247,7 @@ replace_placeholders (tree exp, tree obj, bool *seen_p /*= NULL*/) > > /* If the object isn't a (member of a) class, do nothing. */ > tree op0 = obj; > - while (TREE_CODE (op0) == COMPONENT_REF) > + while (handled_component_p (op0)) > op0 = TREE_OPERAND (op0, 0); > if (!CLASS_TYPE_P (strip_array_types (TREE_TYPE (op0)))) > return exp; > > which I added as an afterthought to my 90996 patch to also handle the > initialization 'T d{};' in pr90996.C (and for symmetry with > lookup_placeholder) is actually sufficient by itself to compile the > whole of pr90996.C and to fix PR90996. > > With that hunk, the call to replace_placeholders from > cp_gimplify_init_expr ends up doing the right thing when passed > exp = *(&<PLACEHOLDER_EXPR struct S>)->a > obj = c[i][j].b[0] for 0 <= i,j <= 1 > because the hunk lets replace_placeholders strip outer ARRAY_REF from > 'obj' to resolve each PLACEHOLDER_EXPR to c[i][j]. > > So if there is no observable advantage to replacing PLACEHOLDER_EXPRs > sooner in store_init_value versus rather than later in > cp_gimplify_init_expr, then the two hunks in > process_init_constructor_array are neither necessary nor sufficient. > Sorry I didn't catch this when writing the patch. > > Shall I commit the following after bootstrap/regtesting? I've committed this partial reversion of my PR90996 fix to trunk: -- >8 -- Subject: [committed] c++: Revert unnecessary parts of fix for [PR90996] The process_init_constructor_array part of my PR90996 patch turns out to be neither necessary nor sufficient to make the pr90996.C testcase work, and I wasn't able to come up with a testcase that demonstrates this part is ever necessary. gcc/cp/ChangeLog: Revert: 2020-04-07 Patrick Palka <ppalka@redhat.com> PR c++/90996 * typeck2.c (process_init_constructor_array): Propagate CONSTRUCTOR_PLACEHOLDER_BOUNDARY up from each element initializer to the array initializer. gcc/testsuite/ChangeLog: PR c++/90996 * g++.dg/cpp1y/pr90996.C: Turn into execution test to verify that each PLACEHOLDER_EXPR gets correctly resolved. --- gcc/cp/typeck2.c | 18 ------------------ gcc/testsuite/g++.dg/cpp1y/pr90996.C | 19 ++++++++++++++++++- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c index af84c257e96..5fd3b82fa89 100644 --- a/gcc/cp/typeck2.c +++ b/gcc/cp/typeck2.c @@ -1496,17 +1496,6 @@ process_init_constructor_array (tree type, tree init, int nested, int flags, = massage_init_elt (TREE_TYPE (type), ce->value, nested, flags, complain); - if (TREE_CODE (ce->value) == CONSTRUCTOR - && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value)) - { - /* Shift CONSTRUCTOR_PLACEHOLDER_BOUNDARY from the element initializer - up to the array initializer, so that the call to - replace_placeholders from store_init_value can resolve any - PLACEHOLDER_EXPRs inside this element initializer. */ - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value) = 0; - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; - } - gcc_checking_assert (ce->value == error_mark_node || (same_type_ignoring_top_level_qualifiers_p @@ -1535,13 +1524,6 @@ process_init_constructor_array (tree type, tree init, int nested, int flags, /* The default zero-initialization is fine for us; don't add anything to the CONSTRUCTOR. */ next = NULL_TREE; - else if (TREE_CODE (next) == CONSTRUCTOR - && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next)) - { - /* As above. */ - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0; - CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; - } } else if (!zero_init_p (TREE_TYPE (type))) next = build_zero_init (TREE_TYPE (type), diff --git a/gcc/testsuite/g++.dg/cpp1y/pr90996.C b/gcc/testsuite/g++.dg/cpp1y/pr90996.C index 780cbb4e3ac..eff5b62db28 100644 --- a/gcc/testsuite/g++.dg/cpp1y/pr90996.C +++ b/gcc/testsuite/g++.dg/cpp1y/pr90996.C @@ -1,5 +1,5 @@ // PR c++/90996 -// { dg-do compile { target c++14 } } +// { dg-do run { target c++14 } } struct S { @@ -15,3 +15,20 @@ struct T }; T d {}; + +int +main() +{ + if (++c[0][0].b[0] != 6 + || ++c[0][1].b[0] != 3 + || ++c[1][0].b[0] != 3 + || ++c[1][1].b[0] != 3) + __builtin_abort(); + + auto& e = d.c; + if (++e[0][0].b[0] != 8 + || ++e[0][1].b[0] != 3 + || ++e[1][0].b[0] != 3 + || ++e[1][1].b[0] != 3) + __builtin_abort(); +}
PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array gcc/cp/ChangeLog: PR c++/94510 * decl.c (reshape_init_array_1): Exclude mismatches with all kinds of pointers. gcc/testsuite/ChangeLog: PR c++/94510 * g++.dg/init/array57.C: New test. * g++.dg/init/array58.C: New test. diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index a127734af69..692c8ed73f4 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -6041,9 +6041,14 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d, TREE_CONSTANT (new_init) = false; /* Pointers initialized to strings must be treated as non-zero - even if the string is empty. */ + even if the string is empty. Handle all kinds of pointers, + including std::nullptr and GCC's __nullptr, neither of which + has a pointer type. */ tree init_type = TREE_TYPE (elt_init); - if (POINTER_TYPE_P (elt_type) != POINTER_TYPE_P (init_type) + bool init_is_ptr = (POINTER_TYPE_P (init_type) + || NULLPTR_TYPE_P (init_type) + || null_node_p (elt_init)); + if (POINTER_TYPE_P (elt_type) != init_is_ptr || !type_initializer_zero_p (elt_type, elt_init)) last_nonzero = index; diff --git a/gcc/testsuite/g++.dg/init/array57.C b/gcc/testsuite/g++.dg/init/array57.C new file mode 100644 index 00000000000..fdd7e76eb18 --- /dev/null +++ b/gcc/testsuite/g++.dg/init/array57.C @@ -0,0 +1,15 @@ +/* PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array + { dg-do compile } */ + +int ia1[2] = { (void*)0 }; // { dg-error "invalid conversion from 'void\\\*'" } +int ia2[2] = { (void*)0, 0 }; // { dg-error "invalid conversion from 'void\\\*'" } +int ia3[] = { (void*)0, 0 }; // { dg-error "invalid conversion from 'void\\\*'" } + +int ia4[2] = { __null }; // { dg-warning "\\\[-Wconversion-null" } +int ia5[2] = { __null, 0 }; // { dg-warning "\\\[-Wconversion-null" } +int ia6[] = { __null, 0 }; // { dg-warning "\\\[-Wconversion-null" } + + +const char ca1[2] = { (char*)0, 0 }; // { dg-error "invalid conversion from 'char\\\*'" } + +const char ca2[2] = { __null, 0 }; // { dg-warning "\\\[-Wconversion-null" } diff --git a/gcc/testsuite/g++.dg/init/array58.C b/gcc/testsuite/g++.dg/init/array58.C new file mode 100644 index 00000000000..655e08fa600 --- /dev/null +++ b/gcc/testsuite/g++.dg/init/array58.C @@ -0,0 +1,21 @@ +/* PR c++/94510 - nullptr_t implicitly cast to zero twice in std::array + { dg-do compile { target c++11 } } */ + +namespace std { +typedef __typeof__ (nullptr) nullptr_t; +} + +int ia1[2] = { nullptr }; // { dg-error "cannot convert 'std::nullptr_t' to 'int'" } +int ia2[2] = { nullptr, 0 }; // { dg-error "cannot convert 'std::nullptr_t' to 'int'" } +int ia3[] = { nullptr, 0 }; // { dg-error "cannot convert 'std::nullptr_t' to 'int'" } + +int ia4[2] = { (std::nullptr_t)0 }; // { dg-error "cannot convert 'std::nullptr_t' to 'int'" } +int ia5[2] = { (std::nullptr_t)0, 0 }; // { dg-error "cannot convert 'std::nullptr_t' to 'int'" } +int ia6[] = { (std::nullptr_t)0, 0 }; // { dg-error "cannot convert 'std::nullptr_t' to 'int'" } + + +const char ca1[2] = { nullptr, 0 }; // { dg-error "cannot convert 'std::nullptr_t' to 'const char'" } + +const char ca2[2] = { (char*)nullptr, 0 };// { dg-error "invalid conversion from 'char\\\*' to 'char'" } + +const char ca3[2] = { std::nullptr_t () };// { dg-error "cannot convert 'std::nullptr_t'" }