Ping for this; was there any changes that you wanted me to make here?
On Sat, Feb 08, 2025 at 11:32:28PM +1100, Nathaniel Shead wrote:
> On Fri, Feb 07, 2025 at 11:11:21AM -0500, Jason Merrill wrote:
> > On 2/7/25 9:28 AM, Nathaniel Shead wrote:
> > > 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?
> > >
> > > 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.
> >
> > My question is why has_tu_local_dep (name) can be false while
> > has_tu_local_dep (tmpl) is true.
>
> In this case it's an early exit thing; when walking the TYPE_NAME, we
> find that it's a TEMPLATE_DECL_RESULT in 'trees_out::decl_node', and
> walk the TI_TEMPLATE of that decl there.
>
> When walking this TEMPLATE_DECL we end up calling 'add_dependency' on
> it, which creates the depset and adds it to the worklist. This doesn't
> add the TYPE_DECL in this pass (we don't walk the TEMPLATE_DECL dep
> we've just made yet, we're still walking whatever decl we were
> processing when we saw this type), so when we come back to this checking
> assertion we haven't made a dep for the TYPE_NAME yet.
>
> > I notice that find_tu_local_decl doesn't walk into TYPE_TI_TEMPLATE.
>
> So my thinking had been that 'find_tu_local_decl' doesn't need to handle
> this case, because it's only ran when '!is_initial_scan', so all
> dependencies that we could possibly have seen will have been created by
> now.
>
> But because 'decl_value' early exits when we see a TU-local dep, we
> never end up building the TYPE_NAME depset at all, which would indeed
> cause 'find_tu_local_decl' to break. I haven't been able to make a
> testcase for this however, as to actually reach this point we would have
> needed to do the above walk in the body of a function template and find
> just the TU-local DECL_TEMPLATE_RESULT in a type node, without walking
> into any other TU-local decl as a byproduct. I'm not sure this is
> possible, as use of decltype would require naming another TU-local decl.
>
> If we're sure that this case can never come up in practice, we could
> maybe add a checking assert to 'find_tu_local_decl' that if we found a
> decl and it didn't have a TU-local dep, it also didn't have a template
> info that had a TU-local dep.
>
> Or the safe way to handle this possibility is to remove the early exit
> from 'decl_value', and just do the extra walking of the value that we'll
> throw away later anyway. Or a slightly less expensive alternative would
> be to, if we have a TEMPLATE_DECL, also build the depset for the
> DECL_TEMPLATE_RESULT before bailing.
>
> Nathaniel
