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 <ppa...@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(); +} -- 2.26.2.626.g172e8ff696