On Thu, Mar 06, 2025 at 11:00:46AM -0500, Jason Merrill wrote:
> On 2/8/25 7:32 AM, 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.
>
> So the problem is that we never make_dependency for 'name' because it
> happens to be a DECL_TEMPLATE_RESULT, so find_dependency returns null, so
> has_tu_local_dep gives the wrong answer.
>
Right. Worth noting that if it wasn't actually TU-local then we would
eventually have done make_dependency on 'name, but we still wouldn't
have made the dependency for 'name' by that point because it would only
be created once we walked the TEMPLATE_DECL.
> > > 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.
>
> Or make has_tu_local_dep (or find_dependency) look through DECL_TI_TEMPLATE
> in this case like trees_out::decl_node does?
>
> Jason
>
Yup, that works. Here's an updated patch; tested modules.exp so far, OK
for trunk if full bootstrap+regtest succeeds?
-- >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::has_tu_local_dep): Also look at the
TI_TEMPLATE if we don't find a dep for a decl.
(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 | 72 +++++++++++++++++++---
gcc/testsuite/g++.dg/modules/internal-10.C | 25 ++++++++
2 files changed, 87 insertions(+), 10 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 8b0f42951c2..97c3549093c 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -9632,6 +9632,15 @@ trees_out::has_tu_local_dep (tree decl) const
decl = TYPE_NAME (DECL_CONTEXT (decl));
depset *dep = dep_hash->find_dependency (decl);
+ if (!dep)
+ {
+ /* This might be the DECL_TEMPLATE_RESULT of a TEMPLATE_DECL
+ which we found was TU-local and gave up early. */
+ int use_tpl = -1;
+ if (tree ti = node_template_info (decl, use_tpl))
+ dep = dep_hash->find_dependency (TI_TEMPLATE (ti));
+ }
+
return dep && dep->is_tu_local ();
}
@@ -13504,19 +13513,31 @@ depset::hash::is_tu_local_entity (tree decl, bool
explain/*=false*/)
We consider types with names for linkage purposes as having names, since
these aren't really TU-local. */
- if (TREE_CODE (decl) == TYPE_DECL
+ tree inner = STRIP_TEMPLATE (decl);
+ if (inner
+ && TREE_CODE (inner) == TYPE_DECL
&& TYPE_ANON_P (type)
- && !DECL_SELF_REFERENCE_P (decl)
+ && !DECL_SELF_REFERENCE_P (inner)
/* An enum with an enumerator name for linkage. */
&& !(UNSCOPED_ENUM_P (type) && TYPE_VALUES (type)))
{
tree main_decl = TYPE_MAIN_DECL (type);
- if (!DECL_CLASS_SCOPE_P (main_decl)
- && !decl_function_context (main_decl)
- /* LAMBDA_EXPR_EXTRA_SCOPE will be set for lambdas defined in
- contexts where they would not be TU-local. */
- && !(LAMBDA_TYPE_P (type)
- && LAMBDA_TYPE_EXTRA_SCOPE (type)))
+ if (LAMBDA_TYPE_P (type))
+ {
+ /* A lambda expression is, in practice, TU-local iff it has no
+ mangling scope. This currently doesn't line up exactly with
+ the standard's definition due to some ABI issues, but it's
+ pretty close, and avoids other issues down the line. */
+ if (!LAMBDA_TYPE_EXTRA_SCOPE (type))
+ {
+ if (explain)
+ inform (loc, "%qT has no name and cannot be differentiated "
+ "from similar lambdas in other TUs", type);
+ return true;
+ }
+ }
+ else if (!DECL_CLASS_SCOPE_P (main_decl)
+ && !decl_function_context (main_decl))
{
if (explain)
inform (loc, "%qT has no name and is not defined within a class, "
@@ -13820,6 +13841,35 @@ depset::hash::make_dependency (tree decl, entity_kind
ek)
return dep;
}
+/* Whether REF is an exposure of a member type of SOURCE.
+
+ This comes up with exposures of class-scope lambdas, that we currently
+ treat as TU-local due to ABI reasons. In such a case the type of the
+ lambda will be exposed in two places, first by the class type it is in
+ the TYPE_FIELDS list of, and second by the actual member declaring that
+ lambda. We only want the second case to warn. */
+
+static bool
+is_exposure_of_member_type (depset *source, depset *ref)
+{
+ gcc_checking_assert (source->refs_tu_local () && ref->is_tu_local ());
+ tree source_entity = STRIP_TEMPLATE (source->get_entity ());
+ tree ref_entity = STRIP_TEMPLATE (ref->get_entity ());
+
+ if (source_entity
+ && ref_entity
+ && DECL_IMPLICIT_TYPEDEF_P (source_entity)
+ && DECL_IMPLICIT_TYPEDEF_P (ref_entity)
+ && DECL_CLASS_SCOPE_P (ref_entity)
+ && DECL_CONTEXT (ref_entity) == TREE_TYPE (source_entity))
+ {
+ gcc_checking_assert (LAMBDA_TYPE_P (TREE_TYPE (ref_entity)));
+ return true;
+ }
+ else
+ return false;
+}
+
/* DEP is a newly discovered dependency. Append it to current's
depset. */
@@ -13832,7 +13882,7 @@ depset::hash::add_dependency (depset *dep)
if (dep->is_tu_local ())
{
current->set_flag_bit<DB_REFS_TU_LOCAL_BIT> ();
- if (!ignore_tu_local)
+ if (!ignore_tu_local && !is_exposure_of_member_type (current, dep))
current->set_flag_bit<DB_EXPOSURE_BIT> ();
}
@@ -14705,7 +14755,9 @@ depset::hash::finalize_dependencies ()
tree decl = dep->get_entity ();
for (depset *rdep : dep->deps)
- if (!rdep->is_binding () && rdep->is_tu_local ())
+ if (!rdep->is_binding ()
+ && rdep->is_tu_local ()
+ && !is_exposure_of_member_type (dep, rdep))
{
// FIXME:QOI Better location information? We're
// losing, so it doesn't matter about efficiency
diff --git a/gcc/testsuite/g++.dg/modules/internal-10.C
b/gcc/testsuite/g++.dg/modules/internal-10.C
new file mode 100644
index 00000000000..e374aaa9145
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/internal-10.C
@@ -0,0 +1,25 @@
+// { dg-additional-options "-fmodules -std=c++20" }
+// { dg-module-cmi !M }
+
+// These are not TU-local according to the standard,
+// but we treat as TU-local due to ABI limitations.
+
+export module M;
+
+struct A {
+ static inline auto x = []{};
+
+ template <typename T>
+ static inline auto y = []{};
+};
+
+struct B {
+ template <typename T>
+ static inline decltype([]{}) x = {}; // { dg-bogus "TU-local" "" { xfail
*-*-* } }
+};
+
+template <typename T>
+struct C {
+ template <typename U>
+ static inline decltype([]{}) x = {}; // { dg-bogus "TU-local" "" { xfail
*-*-* } }
+};
--
2.47.0
