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
