On Mon, Jul 08, 2024 at 02:21:41PM -0400, Jason Merrill wrote:
> On 7/6/24 10:07 PM, Nathaniel Shead wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> >
> > -- >8 --
> >
> > This fixes an ICE exposed by supporting exported non-function
> > using-decls. Sometimes when preparing to define a class, xref_tag will
> > find a using-decl belonging to a different namespace, which triggers the
> > checking_assert in modules handling.
> >
> > Ideally I feel that 'lookup_and_check_tag' should be told whether we're
> > about to define the type and handle erroring on redefinitions itself to
> > avoid this issue (and provide better diagnostics by acknowledging the
> > using-declaration), but this is complicated with the current
> > fragmentation of definition checking. So for this patch we just fixup
> > the assertion and ensure that pushdecl properly errors on the
> > conflicting declaration later.
>
> Or perhaps teaching duplicate_decls to handle using-decls better, so you
> don't need to strip them in the caller?
>
Yup, I have another patch I'd been working on which does this, but this
is a much more targeted approach without all the churn in diagnostics
I'm working through with that change.
But either way I think the change for 'lookup_and_check_tag' is
necessary, because under normal circumstances we don't actually go
through duplicate_decls here until much later, because we always just
work with the existing type name found as an elaborated type specifier.
I imagine we might need to rework this somehow to handle textual
redefinitions too, but I haven't yet really thought through what the
best way to approach that would be.
> The patch is OK.
Thanks.
Nathaniel
>
> > gcc/cp/ChangeLog:
> >
> > * decl.cc (xref_tag): Move assertion into condition.
> > * name-lookup.cc (check_module_override): Check for conflicting
> > types and using-decls.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/modules/using-19_a.C: New test.
> > * g++.dg/modules/using-19_b.C: New test.
> >
> > Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> > ---
> > gcc/cp/decl.cc | 6 +++--
> > gcc/cp/name-lookup.cc | 32 ++++++++++++++++-------
> > gcc/testsuite/g++.dg/modules/using-19_a.C | 18 +++++++++++++
> > gcc/testsuite/g++.dg/modules/using-19_b.C | 10 +++++++
> > 4 files changed, 54 insertions(+), 12 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/modules/using-19_a.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/using-19_b.C
> >
> > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> > index 5a823e2f94c..586dda19604 100644
> > --- a/gcc/cp/decl.cc
> > +++ b/gcc/cp/decl.cc
> > @@ -16737,12 +16737,14 @@ xref_tag (enum tag_types tag_code, tree name,
> > if (CLASS_TYPE_P (t) && CLASSTYPE_IS_TEMPLATE (t))
> > maybe_tmpl = CLASSTYPE_TI_TEMPLATE (t);
> > + /* FIXME: we should do a more precise check for redefinitions
> > + of a conflicting using-declaration here, as these diagnostics
> > + are not ideal. */
> > if (DECL_LANG_SPECIFIC (decl)
> > && DECL_MODULE_IMPORT_P (decl)
> > - && TREE_CODE (CP_DECL_CONTEXT (decl)) == NAMESPACE_DECL)
> > + && CP_DECL_CONTEXT (decl) == current_namespace)
> > {
> > /* Push it into this TU's symbol slot. */
> > - gcc_checking_assert (current_namespace == CP_DECL_CONTEXT (decl));
> > if (maybe_tmpl != decl)
> > /* We're in the template parm binding level.
> > Pushtag has logic to slide under that, but we're
> > diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> > index ddb4c0fe1c8..96d7f938162 100644
> > --- a/gcc/cp/name-lookup.cc
> > +++ b/gcc/cp/name-lookup.cc
> > @@ -3782,18 +3782,30 @@ check_module_override (tree decl, tree mvec, bool
> > hiding,
> > /* Errors could cause there to be nothing. */
> > continue;
> > + tree type = NULL_TREE;
> > if (STAT_HACK_P (bind))
> > - /* We do not have to check STAT_TYPE here, the xref_tag
> > - machinery deals with that problem. */
> > - bind = STAT_VISIBLE (bind);
> > + {
> > + /* If there was a matching STAT_TYPE here then xref_tag
> > + should have found it, but we need to check anyway because
> > + a conflicting using-declaration may exist. */
> > + if (STAT_TYPE_VISIBLE_P (bind))
> > + type = STAT_TYPE (bind);
> > + bind = STAT_VISIBLE (bind);
> > + }
> > - for (ovl_iterator iter (bind); iter; ++iter)
> > - if (!iter.using_p ())
> > - {
> > - match = duplicate_decls (decl, *iter, hiding);
> > - if (match)
> > - goto matched;
> > - }
> > + if (type)
> > + {
> > + match = duplicate_decls (decl, strip_using_decl (type), hiding);
> > + if (match)
> > + goto matched;
> > + }
> > +
> > + for (ovl_iterator iter (strip_using_decl (bind)); iter; ++iter)
> > + {
> > + match = duplicate_decls (decl, *iter, hiding);
> > + if (match)
> > + goto matched;
> > + }
> > }
> > if (TREE_PUBLIC (scope) && TREE_PUBLIC (STRIP_TEMPLATE (decl))
> > diff --git a/gcc/testsuite/g++.dg/modules/using-19_a.C
> > b/gcc/testsuite/g++.dg/modules/using-19_a.C
> > new file mode 100644
> > index 00000000000..693a70ce7d4
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/using-19_a.C
> > @@ -0,0 +1,18 @@
> > +// { dg-additional-options "-fmodules-ts -Wno-global-module" }
> > +// { dg-module-cmi M }
> > +
> > +module;
> > +
> > +namespace hidden {
> > + struct S {};
> > + enum E { e };
> > + void f();
> > +}
> > +
> > +export module M;
> > +export namespace exposed {
> > + using hidden::S;
> > + using hidden::E;
> > + using hidden::e;
> > + using hidden::f;
> > +}
> > diff --git a/gcc/testsuite/g++.dg/modules/using-19_b.C
> > b/gcc/testsuite/g++.dg/modules/using-19_b.C
> > new file mode 100644
> > index 00000000000..dbe8d9f3c01
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/using-19_b.C
> > @@ -0,0 +1,10 @@
> > +// { dg-additional-options "-fmodules-ts" }
> > +
> > +import M;
> > +
> > +namespace exposed {
> > + struct S {}; // { dg-error "redefinition" }
> > + enum E { x }; // { dg-error "multiple definition" }
> > + int e(); // { dg-error "redeclared" }
> > + int f; // { dg-error "redeclared" }
> > +}
>
