On Tue, 5 Aug 2025, Jason Merrill wrote:
> On 7/31/25 10:51 AM, Patrick Palka wrote:
> > On Thu, 31 Jul 2025, Jason Merrill wrote:
> >
> > > On 7/30/25 5:48 PM, Patrick Palka wrote:
> > > > On Wed, 30 Jul 2025, Patrick Palka wrote:
> > > >
> > > > > On Wed, 30 Jul 2025, Patrick Palka wrote:
> > > > >
> > > > > > On Wed, 30 Jul 2025, Patrick Palka wrote:
> > > > > >
> > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
> > > > > > > OK for trunk?
> > > > > > >
> > > > > > > -- >8 --
> > > > > > >
> > > > > > > Here the result of A::make(0, 0, 1), (0, 1, 0) and (1, 0, 0) are
> > > > > > > each
> > > > > > > represented as a single-element CONSTRUCTOR with
> > > > > > > CONSTRUCTOR_NO_CLEARING
> > > > > > > cleared, and we end up mangling them all as A{1}, i.e. eliding
> > > > > > > both
> > > > > > > the
> > > > > > > implicit initial and trailing zeros. Mangling them all the same
> > > > > > > seems
> > > > > > > clearly wrong since they're logically different values.
> > > > > >
> > > > > > Just realized that A::make(1, 0, 1) is also mangled incorrectly --
> > > > > > as
> > > > > > A{1, 1} instead of A{1, 0, 1}. So we need to consider intermediate
> > > > > > implicit zeroes as well, not just initial zeroes...
> > > > >
> > > > > Here's an updated patch that also considers intermediate implicit
> > > > > zeros.
> > > > >
> > > > > Bootstrap and regtest on x86_64-pc-linux-gnu in progress, does this
> > > > > look
> > > > > OK for
> > > > > trunk if successful?
> > > > >
> > > > > -- >8 --
> > > > >
> > > > > Subject: [PATCH] c++: mangling cNTTP object w/ implicit non-trailing
> > > > > zeros
> > > > > [PR121231]
> > > > >
> > > > > Here the result of A::make(0, 0, 1), (0, 1, 0) and (1, 0, 0) are each
> > > > > represented as a single-element CONSTRUCTOR with
> > > > > CONSTRUCTOR_NO_CLEARING
> > > > > cleared, and we end up mangling them all as A{1}, i.e. eliding both
> > > > > the
> > > > > implicit initial and trailing zeros. Mangling them all the same seems
> > > > > clearly wrong since they're logically different values.
> > > > >
> > > > > It turns out we also omit intermediate zeros, e.g. A::make(1, 0, 1) is
> > > > > mangled as A{1, 1} same as A::make(1, 1, 0).
> > > > >
> > > > > It seems we can't omit both trailing and non-trailing implicit zeros
> > > > > without introducing mangling ambiguities. This patch makes us include
> > > > > non-trailing zeros in these manglings, while continuing to omit
> > > > > trailing zeros, which matches e.g. Clang.
> > > > >
> > > > > PR c++/121231
> > > > >
> > > > > gcc/ChangeLog:
> > > > >
> > > > > * common.opt: Document additional ABI version 21 change.
> > > > > * doc/invoke.texi: Likewise.
> > > > >
> > > > > gcc/cp/ChangeLog:
> > > > >
> > > > > * mangle.cc (write_expression): Write out implicit
> > > > > non-trailing
> > > > > zeroes of a CONSTRUCTOR when the ABI version is at least 21.
> > > > >
> > > > > gcc/testsuite/ChangeLog:
> > > > >
> > > > > * g++.dg/abi/mangle82.C: New test.
> > > > > ---
> > > > > gcc/common.opt | 2 +
> > > > > gcc/cp/mangle.cc | 49 ++++++++++++++++
> > > > > gcc/doc/invoke.texi | 5 +-
> > > > > gcc/testsuite/g++.dg/abi/mangle82.C | 86
> > > > > +++++++++++++++++++++++++++++
> > > > > 4 files changed, 140 insertions(+), 2 deletions(-)
> > > > > create mode 100644 gcc/testsuite/g++.dg/abi/mangle82.C
> > > > >
> > > > > diff --git a/gcc/common.opt b/gcc/common.opt
> > > > > index 70659fabebd5..bf38f60d194b 100644
> > > > > --- a/gcc/common.opt
> > > > > +++ b/gcc/common.opt
> > > > > @@ -1067,6 +1067,8 @@ Driver Undocumented
> > > > > ;
> > > > > ; 21: Fix noexcept lambda capture pruning.
> > > > > ; Fix C++20 layout of base with all explicitly defaulted
> > > > > constructors.
> > > > > +; Fix mangling of class and array objects with implicitly
> > > > > +; zero-initialized non-trailing subojects.
> > > > > ; Default in G++ 16.
> > > > > ;
> > > > > ; Additional positive integers will be assigned as new versions of
> > > > > diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc
> > > > > index 13d5dedebd29..296e0f34552e 100644
> > > > > --- a/gcc/cp/mangle.cc
> > > > > +++ b/gcc/cp/mangle.cc
> > > > > @@ -3745,9 +3745,55 @@ write_expression (tree expr)
> > > > > || !zero_init_expr_p (ce->value))
> > > > > last_nonzero = i;
> > > > > + tree prev_field = NULL_TREE;
> > > > > + if (TREE_CODE (TREE_TYPE (expr)) == RECORD_TYPE)
> > > > > + prev_field = first_field (etype);
> > > > > + else if (TREE_CODE (TREE_TYPE (expr)) == ARRAY_TYPE)
> > > > > + prev_field = size_int (0);
> > > >
> > > > On second thought I think the logic is a little clearer if prev_field is
> > > > NULL_TREE on the first iteration, and we adjust the i == 0 logic
> > > > accordingly. Like so (passes initial testisg, full testing in
> > > > progress):
> > > >
> > > > -- >8 --
> > > >
> > > > Here the result of A::make(0, 0, 1), (0, 1, 0) and (1, 0, 0) are each
> > > > represented as a single-element CONSTRUCTOR with CONSTRUCTOR_NO_CLEARING
> > > > cleared, and we end up mangling them all as A{1}, i.e. eliding both the
> > > > implicit initial and trailing zeros. Mangling them all the same seems
> > > > clearly wrong since they're logically different values.
> > > >
> > > > It turns out we also omit intermediate zeros, e.g. A::make(1, 0, 1) is
> > > > mangled as A{1, 1}, the same as A::make(1, 1, 0).
> > > >
> > > > It seems we can't omit both trailing and non-trailing implicit zeros
> > > > without introducing mangling ambiguities. This patch makes us include
> > > > non-trailing zeros in these manglings, while continuing to omit
> > > > trailing zeros, which matches e.g. Clang.
> > > >
> > > > PR c++/121231
> > > >
> > > > gcc/ChangeLog:
> > > >
> > > > * common.opt: Document additional ABI version 21 change.
> > > > * doc/invoke.texi: Likewise.
> > > >
> > > > gcc/cp/ChangeLog:
> > > >
> > > > * mangle.cc (write_expression): Write out implicit non-trailing
> > > > zeroes of a CONSTRUCTOR when the ABI version is at least 21.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > * g++.dg/abi/mangle82.C: New test.
> > > > ---
> > > > gcc/common.opt | 2 +
> > > > gcc/cp/mangle.cc | 50 +++++++++++++++++
> > > > gcc/doc/invoke.texi | 5 +-
> > > > gcc/testsuite/g++.dg/abi/mangle82.C | 86
> > > > +++++++++++++++++++++++++++++
> > > > 4 files changed, 141 insertions(+), 2 deletions(-)
> > > > create mode 100644 gcc/testsuite/g++.dg/abi/mangle82.C
> > > >
> > > > diff --git a/gcc/common.opt b/gcc/common.opt
> > > > index 70659fabebd5..bf38f60d194b 100644
> > > > --- a/gcc/common.opt
> > > > +++ b/gcc/common.opt
> > > > @@ -1067,6 +1067,8 @@ Driver Undocumented
> > > > ;
> > > > ; 21: Fix noexcept lambda capture pruning.
> > > > ; Fix C++20 layout of base with all explicitly defaulted
> > > > constructors.
> > > > +; Fix mangling of class and array objects with implicitly
> > > > +; zero-initialized non-trailing subojects.
> > > > ; Default in G++ 16.
> > > > ;
> > > > ; Additional positive integers will be assigned as new versions of
> > > > diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc
> > > > index 13d5dedebd29..23b21c0a5b7a 100644
> > > > --- a/gcc/cp/mangle.cc
> > > > +++ b/gcc/cp/mangle.cc
> > > > @@ -3745,11 +3745,58 @@ write_expression (tree expr)
> > > > || !zero_init_expr_p (ce->value))
> > > > last_nonzero = i;
> > > > + tree prev_field = NULL_TREE;
> > > > if (undigested || last_nonzero != UINT_MAX)
> > > > for (HOST_WIDE_INT i = 0; vec_safe_iterate (elts, i,
> > > > &ce);
> > > > ++i)
> > > > {
> > > > if (i > last_nonzero)
> > > > break;
> > > > + if (!undigested && !CONSTRUCTOR_NO_CLEARING (expr)
> > > > + && (TREE_CODE (etype) == RECORD_TYPE
> > > > + || TREE_CODE (etype) == ARRAY_TYPE))
> > > > + {
> > > > + /* Write out any implicit non-trailing zeros
> > > > + (which we neglected to do before v21). */
> > > > + if (TREE_CODE (etype) == RECORD_TYPE)
> > > > + {
> > > > + tree field;
> > > > + if (i == 0)
> > > > + field = first_field (etype);
> > > > + else
> > > > + field = DECL_CHAIN (prev_field);
> > > > + for (; field; field = DECL_CHAIN (field))
> > > > + {
> > > > + field = next_subobject_field (field);
> > > > + if (!field || field == ce->index)
> > > > + break;
> > > > + if (abi_check (21))
> > > > + write_expression (build_zero_cst
> > > > + (TREE_TYPE
> > > > (field)));
> > >
> > > This seems to assume that field will have scalar type, surely that isn't
> > > necessarily true?
> >
> > build_zero_cst also handles aggregate types by returning an empty
> > CONSTRUCTOR, which we in turn mangle as T{}.
>
> Ah, indeed. The patch is OK.
Thanks, pushed as r16-3046-g0a7eae02dea519 after noting a couple of
other PRs that are fixed by this.
I wonder on second thought whether this fix should be unconditional
instead of controlled by -fabi-version? Mainly because the bug is
wrong-code, not just wrong mangling, since the identity of class NTTP
arguments is tied to their mangling (and so we can end up conflating
different specializations involving different class NTTP arguments).
If we fixed it unconditionally then we could backport the fix to 15.
I don't feel strongly about it but I thought I'd raise the idea.
>
> Jason
>
>
