On Fri, Feb 07, 2025 at 08:14:23AM -0500, Jason Merrill wrote:
> On 1/31/25 8:46 AM, Nathaniel Shead wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> >
> > Happy to remove the custom inform for lambdas, but I felt that the
> > original message (which suggests that defining it within a class should
> > make it OK) was unhelpful here.
> >
> > Similarly the 'is_exposure_of_member_type' function is not necessary to
> > fix the bug, and is just for slightly nicer diagnostics.
> >
> > -- >8 --
> >
> > Previously, 'is_tu_local_entity' wouldn't detect the exposure of the (in
> > practice) TU-local lambda in the following example, unless instantiated:
> >
> > struct S {
> > template <typename>
> > static inline decltype([]{}) x = {};
> > };
> >
> > This is for two reasons. Firstly, when traversing the TYPE_FIELDS of S
> > we only see the TEMPLATE_DECL, and never end up building a dependency on
> > its DECL_TEMPLATE_RESULT (due to not being instantiated). This patch
> > fixes this by stripping any templates before checking for unnamed types.
> >
> > The second reason is that we currently assume all class-scope entities
> > are not TU-local. Despite this being unambiguous in the standard, this
> > is not actually true in our implementation just yet, due to issues with
> > mangling lambdas in some circumstances. Allowing these lambdas to be
> > exported can cause issues in importers with apparently conflicting
> > declarations, so this patch treats them as TU-local as well.
> >
> > After these changes, we now get double diagnostics from the two ways
> > that we can see the above lambda being exposed, via 'S' (through
> > TYPE_FIELDS) or via 'S::x'. To workaround this we hide diagnostics from
> > the first case, so we only get errors from 'S::x' which will be closer
> > to the point the offending lambda is declared.
> >
> > gcc/cp/ChangeLog:
> >
> > * module.cc (trees_out::type_node): Adjust assertion.
> > (depset::hash::is_tu_local_entity): Handle unnamed template
> > types, treat lambdas specially.
> > (is_exposure_of_member_type): New function.
> > (depset::hash::add_dependency): Use it.
> > (depset::hash::finalize_dependencies): Likewise.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/modules/internal-10.C: New test.
> >
> > Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> > ---
> > gcc/cp/module.cc | 67 ++++++++++++++++++----
> > gcc/testsuite/g++.dg/modules/internal-10.C | 25 ++++++++
> > 2 files changed, 81 insertions(+), 11 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/modules/internal-10.C
> >
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index c89834c1abd..59b7270f4a5 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -9261,7 +9261,9 @@ trees_out::type_node (tree type)
> > /* We'll have either visited this type or have newly discovered
> > that it's TU-local; either way we won't need to visit it again. */
> > - gcc_checking_assert (TREE_VISITED (type) || has_tu_local_dep (name));
> > + gcc_checking_assert (TREE_VISITED (type)
> > + || has_tu_local_dep (TYPE_NAME (type))
> > + || has_tu_local_dep (TYPE_TI_TEMPLATE (type)));
>
> Why doesn't the template having a TU-local dep imply that the TYPE_NAME
> does?
>
> Jason
>
I may not have written this the most clearly; the type doesn't
necessarily even have a template, but if it's not visited and its
TYPE_NAME hasn't had a TU-local dep made then we must instead have seen
a TYPE_TI_TEMPLATE that does have a TU-local dep.
Would you prefer me to write it like this?
gcc_checking_assert (TREE_VISITED (type)
|| has_tu_local_dep (TYPE_NAME (type))
|| (TYPE_TEMPLATE_INFO (type)
&& TYPE_TI_TEMPLATE (type)
&& has_tu_local_dep (TYPE_TI_TEMPLATE (type))));
Alternatively, I suppose we could attempt to optimise the assertion to
only call has_tu_local_dep a maximum of one time like
gcc_checking_assert (TREE_VISITED (type)
|| has_tu_local_dep ((TYPE_TEMPLATE_INFO (type)
&& TYPE_TI_TEMPLATE (type))
? TYPE_TI_TEMPLATE (type)
: TYPE_NAME (type)));
but I don't think this is worth it, has_tu_local_dep is basically just a
couple of conditions and a hash table lookup anyway.
Nathaniel
