Ping for https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638089.html
On Fri, Nov 24, 2023 at 10:32:13PM +1100, Nathaniel Shead wrote:
> On Thu, Nov 23, 2023 at 12:11:58PM -0500, Nathan Sidwell wrote:
> > On 11/14/23 01:24, Nathaniel Shead wrote:
> > > I'll also note that the comments above the parsing functions here no
> > > longer exactly match with the grammar in the standard, should they be
> > > updated as well?
> >
> > please.
> >
>
> As I was attempting to rewrite the comments I ended up splitting up the
> work that was being done by cp_parser_module_name a lot to better match
> the grammar, and also caught a few other segfaults that were occurring
> along the way.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>
> -- >8 --
>
> This patch cleans up the parsing of module-declarations and
> import-declarations to more closely follow the grammar defined by the
> standard.
>
> For instance, currently we allow declarations like 'import A:B', even
> from an unrelated source file (not part of module A), which causes
> errors in merging declarations. However, the syntax in [module.import]
> doesn't even allow this form of import, so this patch prevents this from
> parsing at all and avoids the error that way.
>
> Additionally, we sometimes allow statements like 'import :X' or
> 'module :X' even when not in a named module, and this causes segfaults,
> so we disallow this too.
>
> PR c++/110808
>
> gcc/cp/ChangeLog:
>
> * parser.cc (cp_parser_module_name): Rewrite to handle
> module-names and module-partitions independently.
> (cp_parser_module_partition): New function.
> (cp_parser_module_declaration): Parse module partitions
> explicitly. Don't change state if parsing module decl failed.
> (cp_parser_import_declaration): Handle different kinds of
> import-declarations locally.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/part-hdr-1_c.C: Fix syntax.
> * g++.dg/modules/part-mac-1_c.C: Likewise.
> * g++.dg/modules/mod-invalid-1.C: New test.
> * g++.dg/modules/part-8_a.C: New test.
> * g++.dg/modules/part-8_b.C: New test.
> * g++.dg/modules/part-8_c.C: New test.
>
> Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> ---
> gcc/cp/parser.cc | 100 ++++++++++++-------
> gcc/testsuite/g++.dg/modules/mod-invalid-1.C | 7 ++
> gcc/testsuite/g++.dg/modules/part-8_a.C | 6 ++
> gcc/testsuite/g++.dg/modules/part-8_b.C | 6 ++
> gcc/testsuite/g++.dg/modules/part-8_c.C | 8 ++
> gcc/testsuite/g++.dg/modules/part-hdr-1_c.C | 2 +-
> gcc/testsuite/g++.dg/modules/part-mac-1_c.C | 2 +-
> 7 files changed, 95 insertions(+), 36 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/mod-invalid-1.C
> create mode 100644 gcc/testsuite/g++.dg/modules/part-8_a.C
> create mode 100644 gcc/testsuite/g++.dg/modules/part-8_b.C
> create mode 100644 gcc/testsuite/g++.dg/modules/part-8_c.C
>
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index f6d088bc73f..20bd8d45a08 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -14853,58 +14853,64 @@ cp_parser_already_scoped_statement (cp_parser*
> parser, bool *if_p,
>
> /* Modules */
>
> -/* Parse a module-name,
> - identifier
> - module-name . identifier
> - header-name
> +/* Parse a module-name or module-partition.
>
> - Returns a pointer to module object, NULL. */
> + module-name:
> + module-name-qualifier [opt] identifier
>
> -static module_state *
> -cp_parser_module_name (cp_parser *parser)
> -{
> - cp_token *token = cp_lexer_peek_token (parser->lexer);
> - if (token->type == CPP_HEADER_NAME)
> - {
> - cp_lexer_consume_token (parser->lexer);
> + module-partition:
> + : module-name-qualifier [opt] identifier
>
> - return get_module (token->u.value);
> - }
> + module-name-qualifier:
> + identifier .
> + module-name-qualifier identifier .
>
> - module_state *parent = NULL;
> - bool partitioned = false;
> - if (token->type == CPP_COLON && named_module_p ())
> - {
> - partitioned = true;
> - cp_lexer_consume_token (parser->lexer);
> - }
> + Returns a pointer to the module object, or NULL on failure.
> + For PARTITION_P, PARENT is the module this is a partition of. */
> +
> +static module_state *
> +cp_parser_module_name (cp_parser *parser, bool partition_p = false,
> + module_state *parent = NULL)
> +{
> + if (partition_p
> + && cp_lexer_consume_token (parser->lexer)->type != CPP_COLON)
> + return NULL;
>
> for (;;)
> {
> if (cp_lexer_peek_token (parser->lexer)->type != CPP_NAME)
> {
> - cp_parser_error (parser, "expected module-name");
> - break;
> + if (partition_p)
> + cp_parser_error (parser, "expected module-partition");
> + else
> + cp_parser_error (parser, "expected module-name");
> + return NULL;
> }
>
> tree name = cp_lexer_consume_token (parser->lexer)->u.value;
> - parent = get_module (name, parent, partitioned);
> - token = cp_lexer_peek_token (parser->lexer);
> - if (!partitioned && token->type == CPP_COLON)
> - partitioned = true;
> - else if (token->type != CPP_DOT)
> + parent = get_module (name, parent, partition_p);
> + if (cp_lexer_peek_token (parser->lexer)->type != CPP_DOT)
> break;
>
> cp_lexer_consume_token (parser->lexer);
> - }
> + }
>
> return parent;
> }
>
> +/* Parse a module-partition. Defers to cp_parser_module_name. */
> +
> +static module_state *
> +cp_parser_module_partition (cp_parser *parser, module_state *parent = NULL)
> +{
> + return cp_parser_module_name (parser, /*partition_p=*/true, parent);
> +}
> +
> /* Named module-declaration
> __module ; PRAGMA_EOL
> - __module private ; PRAGMA_EOL (unimplemented)
> - [__export] __module module-name attr-spec-seq-opt ; PRAGMA_EOL
> + __module : private ; PRAGMA_EOL (unimplemented)
> + [__export] __module module-name module-partition [opt]
> + attr-spec-seq-opt ; PRAGMA_EOL
> */
>
> static module_parse
> @@ -14962,9 +14968,12 @@ cp_parser_module_declaration (cp_parser *parser,
> module_parse mp_state,
> else
> {
> module_state *mod = cp_parser_module_name (parser);
> + if (mod && cp_lexer_peek_token (parser->lexer)->type == CPP_COLON)
> + mod = cp_parser_module_partition (parser, mod);
> tree attrs = cp_parser_attributes_opt (parser);
>
> - mp_state = MP_PURVIEW_IMPORTS;
> + if (mod)
> + mp_state = MP_PURVIEW_IMPORTS;
> if (!mod || !cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON))
> goto skip_eol;
>
> @@ -14976,7 +14985,10 @@ cp_parser_module_declaration (cp_parser *parser,
> module_parse mp_state,
> }
>
> /* Import-declaration
> - [__export] __import module-name attr-spec-seq-opt ; PRAGMA_EOL */
> + __import module-name attr-spec-seq-opt ; PRAGMA_EOL
> + __import module-partition attr-spec-seq-opt ; PRAGMA_EOL
> + __import header-name attr-spec-seq-opt ; PRAGMA_EOL
> +*/
>
> static void
> cp_parser_import_declaration (cp_parser *parser, module_parse mp_state,
> @@ -15004,7 +15016,27 @@ cp_parser_import_declaration (cp_parser *parser,
> module_parse mp_state,
> }
> else
> {
> - module_state *mod = cp_parser_module_name (parser);
> + module_state *mod = NULL;
> + cp_token *next = cp_lexer_peek_token (parser->lexer);
> + if (next->type == CPP_HEADER_NAME)
> + {
> + cp_lexer_consume_token (parser->lexer);
> + mod = get_module (next->u.value);
> + }
> + else if (next->type == CPP_COLON)
> + {
> + /* An import specifying a module-partition shall only appear after the
> + module-declaration in a module unit: [module.import]/4. */
> + if (named_module_p ()
> + && (mp_state == MP_PURVIEW_IMPORTS
> + || mp_state == MP_PRIVATE_IMPORTS))
> + mod = cp_parser_module_partition (parser);
> + else
> + error_at (next->location, "import specifying a module-partition"
> + " must appear after a named module-declaration");
> + }
> + else
> + mod = cp_parser_module_name (parser);
> tree attrs = cp_parser_attributes_opt (parser);
>
> if (!mod || !cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON))
> diff --git a/gcc/testsuite/g++.dg/modules/mod-invalid-1.C
> b/gcc/testsuite/g++.dg/modules/mod-invalid-1.C
> new file mode 100644
> index 00000000000..fadaba0b560
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/mod-invalid-1.C
> @@ -0,0 +1,7 @@
> +// { dg-additional-options "-fmodules-ts" }
> +
> +module;
> +
> +module :foo; // { dg-error "expected module-name" }
> +
> +import :foo; // { dg-error "import specifying a module-partition must
> appear after a named module-declaration" }
> diff --git a/gcc/testsuite/g++.dg/modules/part-8_a.C
> b/gcc/testsuite/g++.dg/modules/part-8_a.C
> new file mode 100644
> index 00000000000..09f956ff36f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/part-8_a.C
> @@ -0,0 +1,6 @@
> +// PR c++/110808
> +// { dg-additional-options "-fmodules-ts" }
> +// { dg-module-cmi group:tres }
> +
> +export module group:tres;
> +int mul() { return 0; }
> diff --git a/gcc/testsuite/g++.dg/modules/part-8_b.C
> b/gcc/testsuite/g++.dg/modules/part-8_b.C
> new file mode 100644
> index 00000000000..1ade029495c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/part-8_b.C
> @@ -0,0 +1,6 @@
> +// PR c++/110808
> +// { dg-additional-options "-fmodules-ts" }
> +// { dg-module-cmi group }
> +
> +export module group;
> +export import :tres;
> diff --git a/gcc/testsuite/g++.dg/modules/part-8_c.C
> b/gcc/testsuite/g++.dg/modules/part-8_c.C
> new file mode 100644
> index 00000000000..2351f28f909
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/part-8_c.C
> @@ -0,0 +1,8 @@
> +// PR c++/110808
> +// { dg-additional-options "-fmodules-ts" }
> +
> +import group:tres; // { dg-error "expected .;." }
> +
> +int main() {
> + return mul(); // { dg-error "not declared" }
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/part-hdr-1_c.C
> b/gcc/testsuite/g++.dg/modules/part-hdr-1_c.C
> index 78a53d2fda3..db57adcef44 100644
> --- a/gcc/testsuite/g++.dg/modules/part-hdr-1_c.C
> +++ b/gcc/testsuite/g++.dg/modules/part-hdr-1_c.C
> @@ -2,4 +2,4 @@
> // { dg-module-cmi {mod} }
>
> export module mod;
> -import mod:impl;
> +import :impl;
> diff --git a/gcc/testsuite/g++.dg/modules/part-mac-1_c.C
> b/gcc/testsuite/g++.dg/modules/part-mac-1_c.C
> index 78a53d2fda3..db57adcef44 100644
> --- a/gcc/testsuite/g++.dg/modules/part-mac-1_c.C
> +++ b/gcc/testsuite/g++.dg/modules/part-mac-1_c.C
> @@ -2,4 +2,4 @@
> // { dg-module-cmi {mod} }
>
> export module mod;
> -import mod:impl;
> +import :impl;
> --
> 2.42.0
>
