On Fri, 16 Feb 2024, Nathaniel Shead wrote:
> On Tue, Feb 13, 2024 at 07:52:01PM -0500, Jason Merrill wrote:
> > On 2/10/24 17:57, Nathaniel Shead wrote:
> > > The fix for PR107398 weakened the restrictions that lambdas must belong
> > > to namespace scope. However this was not sufficient: we also need to
> > > allow lambdas keyed to FIELD_DECLs or PARM_DECLs.
> >
> > I wonder about keying such lambdas to the class and function, respectively,
> > rather than specifically to the field or parameter, but I suppose it doesn't
> > matter.
>
> I did some more testing and realised my testcase didn't properly
> exercise whether I'd properly deduplicated or not, and an improved
> testcase proved that actually keying to the field rather than the class
> did cause issues. (Parameter vs. function doesn't seem to have mattered
> however.)
>
> Here's an updated patch that fixes this, and includes the changes for
> lambdas in base classes that I'd had as a separate patch earlier. I've
> also added some concepts testcases just in case.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>
> -- >8 --
>
> The fix for PR107398 weakened the restrictions that lambdas must belong
> to namespace scope. However this was not sufficient: we also need to
> allow lambdas attached to FIELD_DECLs, PARM_DECLs, and TYPE_DECLs.
>
> For field decls we key the lambda to its class rather than the field
> itself. This avoids some errors with deduplicating fields.
>
> Additionally, by [basic.link] p15.2 a lambda defined anywhere in a
> class-specifier should not be TU-local, which includes base-class
> declarations, so ensure that lambdas declared there are keyed
> appropriately as well.
>
> Because this now requires 'DECL_MODULE_KEYED_DECLS_P' to be checked on a
> fairly large number of different kinds of DECLs, and that in general
> it's safe to just get 'false' as a result of a check on an unexpected
> DECL type, this patch also removes the tree checking from the accessor.
>
> Finally, to handle deduplicating templated lambda fields, we need to
> ensure that we can determine that two lambdas from different field decls
> match. The modules code does not attempt to deduplicate expression
> nodes, which causes issues as the LAMBDA_EXPRs are then considered to be
> different. However, rather than checking the LAMBDA_EXPR directly we can
> instead check its type: the generated RECORD_TYPE for a LAMBDA_EXPR must
> also be unique, and /is/ deduplicated on import, so we can just check
> for that instead.
We probably should be deduplicating LAMBDA_EXPR on stream-in, perhaps
something like
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index e8eabb1f6f9..1b2ba2e0fa8 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -9183,6 +9183,13 @@ trees_in::tree_value ()
return NULL_TREE;
}
+ if (TREE_CODE (t) == LAMBDA_EXPR
+ && CLASSTYPE_LAMBDA_EXPR (TREE_TYPE (t)))
+ {
+ existing = CLASSTYPE_LAMBDA_EXPR (TREE_TYPE (t));
+ back_refs[~tag] = existing;
+ }
+
dump (dumper::TREE) && dump ("Read tree:%d %C:%N", tag, TREE_CODE (t), t);
if (TREE_CODE (existing) == INTEGER_CST && !TREE_OVERFLOW (existing))
would suffice? If not we probably need to take inspiration from the
TREE_BINFO streaming, and handle LAMBDA_EXPR similarly..
>
> PR c++/111710
>
> gcc/cp/ChangeLog:
>
> * cp-tree.h (DECL_MODULE_KEYED_DECLS_P): Remove tree checking.
> (struct lang_decl_base): Update comments and fix whitespace.
> * module.cc (trees_out::lang_decl_bools): Always write
> module_keyed_decls_p flag...
> (trees_in::lang_decl_bools): ...and always read it.
> (trees_out::decl_value): Handle all kinds of keyed decls.
> (trees_in::decl_value): Likewise.
> (maybe_key_decl): Also support lambdas attached to fields,
> parameters, and types. Key lambdas attached to fields to their
> class.
> (trees_out::get_merge_kind): Likewise.
> (trees_out::key_mergeable): Likewise.
> (trees_in::key_mergeable): Support keyed decls in a TYPE_DECL
> container.
> * parser.cc (cp_parser_class_head): Start a lambda scope when
> parsing base classes.
> * tree.cc (cp_tree_equal): Check equality of the types of
> LAMBDA_EXPRs instead of the exprs themselves.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/lambda-7.h: New test.
> * g++.dg/modules/lambda-7_a.H: New test.
> * g++.dg/modules/lambda-7_b.C: New test.
> * g++.dg/modules/lambda-7_c.C: New test.
>
> Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> ---
> gcc/cp/cp-tree.h | 26 +++----
> gcc/cp/module.cc | 94 +++++++++++++----------
> gcc/cp/parser.cc | 10 ++-
> gcc/cp/tree.cc | 4 +-
> gcc/testsuite/g++.dg/modules/lambda-7.h | 42 ++++++++++
> gcc/testsuite/g++.dg/modules/lambda-7_a.H | 4 +
> gcc/testsuite/g++.dg/modules/lambda-7_b.C | 5 ++
> gcc/testsuite/g++.dg/modules/lambda-7_c.C | 41 ++++++++++
> 8 files changed, 169 insertions(+), 57 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/lambda-7.h
> create mode 100644 gcc/testsuite/g++.dg/modules/lambda-7_a.H
> create mode 100644 gcc/testsuite/g++.dg/modules/lambda-7_b.C
> create mode 100644 gcc/testsuite/g++.dg/modules/lambda-7_c.C
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 334c11396c2..04c3aa6cd91 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -1773,9 +1773,8 @@ check_constraint_info (tree t)
> (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK (NODE))->u.base.module_entity_p)
>
> /* DECL that has attached decls for ODR-relatedness. */
> -#define DECL_MODULE_KEYED_DECLS_P(NODE) \
> - (DECL_LANG_SPECIFIC (TREE_CHECK2(NODE,FUNCTION_DECL,VAR_DECL))\
> - ->u.base.module_keyed_decls_p)
> +#define DECL_MODULE_KEYED_DECLS_P(NODE) \
> + (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK
> (NODE))->u.base.module_keyed_decls_p)
>
> /* Whether this is an exported DECL. Held on any decl that can appear
> at namespace scope (function, var, type, template, const or
> @@ -2887,21 +2886,20 @@ struct GTY(()) lang_decl_base {
> unsigned friend_or_tls : 1; /* var, fn, type or template
> */
> unsigned unknown_bound_p : 1; /* var */
> unsigned odr_used : 1; /* var or fn */
> - unsigned concept_p : 1; /* applies to vars and functions
> */
> + unsigned concept_p : 1; /* applies to vars and functions */
> unsigned var_declared_inline_p : 1; /* var */
> unsigned dependent_init_p : 1; /* var */
>
> - /* The following apply to VAR, FUNCTION, TYPE, CONCEPT, & NAMESPACE
> + /* The following four apply to VAR, FUNCTION, TYPE, CONCEPT, & NAMESPACE
> decls. */
> - unsigned module_purview_p : 1; // in named-module purview
> - unsigned module_attach_p : 1; // attached to named module
> - unsigned module_import_p : 1; /* from an import */
> - unsigned module_entity_p : 1; /* is in the entitity ary &
> - hash. */
> - /* VAR_DECL or FUNCTION_DECL has keyed decls. */
> - unsigned module_keyed_decls_p : 1;
> -
> - /* 12 spare bits. */
> + unsigned module_purview_p : 1; /* in named-module purview */
> + unsigned module_attach_p : 1; /* attached to named module
> */
> + unsigned module_import_p : 1; /* from an import */
> + unsigned module_entity_p : 1; /* is in the entitity ary &
> hash */
> +
> + unsigned module_keyed_decls_p : 1; /* has keys, applies to all decls */
> +
> + /* 11 spare bits. */
> };
>
> /* True for DECL codes which have template info and access. */
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 0291d456ff5..af99df2b79c 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -5662,8 +5662,7 @@ trees_out::lang_decl_bools (tree t)
> want to mark them as in module purview. */
> WB (lang->u.base.module_purview_p && !header_module_p ());
> WB (lang->u.base.module_attach_p);
> - if (VAR_OR_FUNCTION_DECL_P (t))
> - WB (lang->u.base.module_keyed_decls_p);
> + WB (lang->u.base.module_keyed_decls_p);
> switch (lang->u.base.selector)
> {
> default:
> @@ -5736,8 +5735,7 @@ trees_in::lang_decl_bools (tree t)
> RB (lang->u.base.dependent_init_p);
> RB (lang->u.base.module_purview_p);
> RB (lang->u.base.module_attach_p);
> - if (VAR_OR_FUNCTION_DECL_P (t))
> - RB (lang->u.base.module_keyed_decls_p);
> + RB (lang->u.base.module_keyed_decls_p);
> switch (lang->u.base.selector)
> {
> default:
> @@ -7867,8 +7865,7 @@ trees_out::decl_value (tree decl, depset *dep)
> install_entity (decl, dep);
> }
>
> - if (VAR_OR_FUNCTION_DECL_P (inner)
> - && DECL_LANG_SPECIFIC (inner)
> + if (DECL_LANG_SPECIFIC (inner)
> && DECL_MODULE_KEYED_DECLS_P (inner)
> && !is_key_order ())
> {
> @@ -8168,8 +8165,7 @@ trees_in::decl_value ()
> bool installed = install_entity (existing);
> bool is_new = existing == decl;
>
> - if (VAR_OR_FUNCTION_DECL_P (inner)
> - && DECL_LANG_SPECIFIC (inner)
> + if (DECL_LANG_SPECIFIC (inner)
> && DECL_MODULE_KEYED_DECLS_P (inner))
> {
> /* Read and maybe install the attached entities. */
> @@ -10480,12 +10476,17 @@ trees_out::get_merge_kind (tree decl, depset *dep)
> if (tree scope
> = LAMBDA_EXPR_EXTRA_SCOPE (CLASSTYPE_LAMBDA_EXPR
> (TREE_TYPE (decl))))
> - if (TREE_CODE (scope) == VAR_DECL
> - && DECL_MODULE_KEYED_DECLS_P (scope))
> - {
> - mk = MK_keyed;
> - break;
> - }
> + {
> + /* Lambdas attached to fields are keyed to its class. */
> + if (TREE_CODE (scope) == FIELD_DECL)
> + scope = TYPE_NAME (DECL_CONTEXT (scope));
> + if (DECL_LANG_SPECIFIC (scope)
> + && DECL_MODULE_KEYED_DECLS_P (scope))
> + {
> + mk = MK_keyed;
> + break;
> + }
> + }
>
> if (RECORD_OR_UNION_TYPE_P (ctx))
> {
> @@ -10785,7 +10786,13 @@ trees_out::key_mergeable (int tag, merge_kind mk,
> tree decl, tree inner,
> gcc_checking_assert (LAMBDA_TYPE_P (TREE_TYPE (inner)));
> tree scope = LAMBDA_EXPR_EXTRA_SCOPE (CLASSTYPE_LAMBDA_EXPR
> (TREE_TYPE (inner)));
> - gcc_checking_assert (TREE_CODE (scope) == VAR_DECL);
> + gcc_checking_assert (TREE_CODE (scope) == VAR_DECL
> + || TREE_CODE (scope) == FIELD_DECL
> + || TREE_CODE (scope) == PARM_DECL
> + || TREE_CODE (scope) == TYPE_DECL);
> + /* Lambdas attached to fields are keyed to the class. */
> + if (TREE_CODE (scope) == FIELD_DECL)
> + scope = TYPE_NAME (DECL_CONTEXT (scope));
> auto *root = keyed_table->get (scope);
> unsigned ix = root->length ();
> /* If we don't find it, we'll write a really big number
> @@ -11063,6 +11070,26 @@ trees_in::key_mergeable (int tag, merge_kind mk,
> tree decl, tree inner,
> }
> }
> }
> + else if (mk == MK_keyed
> + && DECL_LANG_SPECIFIC (name)
> + && DECL_MODULE_KEYED_DECLS_P (name))
> + {
> + gcc_checking_assert (TREE_CODE (container) == NAMESPACE_DECL
> + || TREE_CODE (container) == TYPE_DECL);
> + if (auto *set = keyed_table->get (name))
> + if (key.index < set->length ())
> + {
> + existing = (*set)[key.index];
> + if (existing)
> + {
> + gcc_checking_assert
> + (DECL_IMPLICIT_TYPEDEF_P (existing));
> + if (inner != decl)
> + existing
> + = CLASSTYPE_TI_TEMPLATE (TREE_TYPE (existing));
> + }
> + }
> + }
> else
> switch (TREE_CODE (container))
> {
> @@ -11070,27 +11097,8 @@ trees_in::key_mergeable (int tag, merge_kind mk,
> tree decl, tree inner,
> gcc_unreachable ();
>
> case NAMESPACE_DECL:
> - if (mk == MK_keyed)
> - {
> - if (DECL_LANG_SPECIFIC (name)
> - && VAR_OR_FUNCTION_DECL_P (name)
> - && DECL_MODULE_KEYED_DECLS_P (name))
> - if (auto *set = keyed_table->get (name))
> - if (key.index < set->length ())
> - {
> - existing = (*set)[key.index];
> - if (existing)
> - {
> - gcc_checking_assert
> - (DECL_IMPLICIT_TYPEDEF_P (existing));
> - if (inner != decl)
> - existing
> - = CLASSTYPE_TI_TEMPLATE (TREE_TYPE (existing));
> - }
> - }
> - }
> - else if (is_attached
> - && !(state->is_module () || state->is_partition ()))
> + if (is_attached
> + && !(state->is_module () || state->is_partition ()))
> kind = "unique";
> else
> {
> @@ -18980,11 +18988,19 @@ maybe_key_decl (tree ctx, tree decl)
> if (!modules_p ())
> return;
>
> - // FIXME: For now just deal with lambdas attached to var decls.
> - // This might be sufficient?
> - if (TREE_CODE (ctx) != VAR_DECL)
> + /* We only need to deal with lambdas attached to var, field,
> + parm, or type decls. */
> + if (TREE_CODE (ctx) != VAR_DECL
> + && TREE_CODE (ctx) != FIELD_DECL
> + && TREE_CODE (ctx) != PARM_DECL
> + && TREE_CODE (ctx) != TYPE_DECL)
> return;
>
> + /* For fields, key it to the containing type to handle deduplication
> + correctly. */
> + if (TREE_CODE (ctx) == FIELD_DECL)
> + ctx = TYPE_NAME (DECL_CONTEXT (ctx));
> +
> if (!keyed_table)
> keyed_table = new keyed_map_t (EXPERIMENT (1, 400));
>
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index 9d0914435fb..95d59973b6d 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -27671,10 +27671,16 @@ cp_parser_class_head (cp_parser* parser,
> if (cp_lexer_next_token_is (parser->lexer, CPP_COLON))
> {
> if (type)
> - pushclass (type);
> + {
> + pushclass (type);
> + start_lambda_scope (TYPE_NAME (type));
> + }
> bases = cp_parser_base_clause (parser);
> if (type)
> - popclass ();
> + {
> + finish_lambda_scope ();
> + popclass ();
> + }
> }
> else
> bases = NULL_TREE;
> diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
> index ad312710f68..ca598859b97 100644
> --- a/gcc/cp/tree.cc
> +++ b/gcc/cp/tree.cc
> @@ -4239,8 +4239,8 @@ cp_tree_equal (tree t1, tree t2)
> DEFERRED_NOEXCEPT_ARGS (t2)));
>
> case LAMBDA_EXPR:
> - /* Two lambda-expressions are never considered equivalent. */
> - return false;
> + /* Two lambda-expressions are only equivalent if their type is. */
> + return TREE_TYPE (t1) == TREE_TYPE (t2);
>
> case TYPE_ARGUMENT_PACK:
> case NONTYPE_ARGUMENT_PACK:
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-7.h
> b/gcc/testsuite/g++.dg/modules/lambda-7.h
> new file mode 100644
> index 00000000000..6f6080c1324
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-7.h
> @@ -0,0 +1,42 @@
> +struct S {
> + int (*a)(int) = [](int x) { return x * 2; };
> +
> + int b(int x, int (*f)(int) = [](int x) { return x * 3; }) {
> + return f(x);
> + }
> +
> + static int c(int x, int (*f)(int) = [](int x) { return x * 4; }) {
> + return f(x);
> + }
> +};
> +
> +inline int d(int x, int (*f)(int) = [](int x) { return x * 5; }) {
> + return f(x);
> +}
> +
> +// unevaluated lambdas
> +#if __cplusplus >= 202002L
> +struct E : decltype([](int x) { return x * 6; }) {
> + decltype([](int x) { return x * 7; }) f;
> +};
> +
> +template <typename T>
> +struct G : decltype([](int x) { return x * 8; }) {
> + decltype([](int x) { return x * 9; }) h;
> +};
> +
> +template <>
> +struct G<double> : decltype([](int x) { return x * 10; }) {
> + decltype([](int x) { return x * 11; }) i;
> +};
> +#endif
> +
> +// concepts
> +#if __cpp_concepts >= 201907L
> +template <typename T>
> +concept J = requires { []{ T(); }; };
> +
> +template <typename T>
> +concept K = []{ return sizeof(T) == 1; }();
> +#endif
> +
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-7_a.H
> b/gcc/testsuite/g++.dg/modules/lambda-7_a.H
> new file mode 100644
> index 00000000000..5197114f76c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-7_a.H
> @@ -0,0 +1,4 @@
> +// { dg-additional-options "-fmodule-header -Wno-subobject-linkage" }
> +// { dg-module-cmi {} }
> +
> +#include "lambda-7.h"
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-7_b.C
> b/gcc/testsuite/g++.dg/modules/lambda-7_b.C
> new file mode 100644
> index 00000000000..2d781e93067
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-7_b.C
> @@ -0,0 +1,5 @@
> +// { dg-additional-options "-fmodules-ts -fno-module-lazy
> -Wno-subobject-linkage" }
> +// Test for ODR deduplication
> +
> +#include "lambda-7.h"
> +import "lambda-7_a.H";
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-7_c.C
> b/gcc/testsuite/g++.dg/modules/lambda-7_c.C
> new file mode 100644
> index 00000000000..f283681fa96
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-7_c.C
> @@ -0,0 +1,41 @@
> +// { dg-module-do run }
> +// { dg-additional-options "-fmodules-ts -fno-module-lazy
> -Wno-subobject-linkage" }
> +
> +import "lambda-7_a.H";
> +
> +int main() {
> + S s;
> + if (s.a(10) != 20)
> + __builtin_abort();
> + if (s.b(10) != 30)
> + __builtin_abort();
> + if (s.c(10) != 40)
> + __builtin_abort();
> + if (d(10) != 50)
> + __builtin_abort();
> +
> +#if __cplusplus >= 202002L
> + E e;
> + if (e(10) != 60)
> + __builtin_abort();
> + if (e.f(10) != 70)
> + __builtin_abort();
> +
> + G<int> g1;
> + if (g1(10) != 80)
> + __builtin_abort();
> + if (g1.h(10) != 90)
> + __builtin_abort();
> +
> + G<double> g2;
> + if (g2(10) != 100)
> + __builtin_abort();
> + if (g2.i(10) != 110)
> + __builtin_abort();
> +#endif
> +
> +#if __cpp_concepts >= 201907L
> + static_assert(J<char>);
> + static_assert(K<char>);
> +#endif
> +}
> --
> 2.43.0
>
>
