From 14886cbe300c144a4390245d0515cdf28fc5f68f Mon Sep 17 00:00:00 2001 From: Nathan Sidwell Date: Fri, 19 Feb 2021 13:10:27 -0800 Subject: [PATCH] c++: Incorrect module-number ordering [PR 98741] One of the very strong invariants in modules is that module numbers are allocated such that (other than the current TU), all imports have lesser module numbers, and also that the binding vector is only appended to with increasing module numbers. This broke down when module-directives became a thing and the preprocessing became entirely decoupled from parsing. We'd load header units and their macros (but not symbols of course) during preprocessing. Then we'd load named modules during parsing. This could lead to the situation where a header unit appearing after a named import had a lower module number than the import. Consequently, if they both bound the same identifier, the binding vector would be misorderd and bad things happen. This patch restores a pending import queue I previously had, but in simpler form (hurrah). During preprocessing we queue all module-directives and when we meet one for a header unit we do the minimal loading for all of the queue, so they get appropriate numbering. Then we load the preprocessor state for the header unit. PR c++/98741 gcc/cp/ * module.cc (pending_imports): New. (declare_module): Adjust test condition. (name_pending_imports): New. (preprocess_module): Reimplement using pending_imports. (preprocessed_module): Move name-getting to name_pending_imports. * name-lookup.c (append_imported_binding_slot): Assert module ordering is increasing. gcc/testsuite/ * g++.dg/modules/pr98741_a.H: New. * g++.dg/modules/pr98741_b.H: New. * g++.dg/modules/pr98741_c.C: New. * g++.dg/modules/pr98741_d.C: New. --- gcc/cp/module.cc | 183 +++++++++++++---------- gcc/cp/name-lookup.c | 5 + gcc/testsuite/g++.dg/modules/pr98741_a.H | 7 + gcc/testsuite/g++.dg/modules/pr98741_b.H | 6 + gcc/testsuite/g++.dg/modules/pr98741_c.C | 4 + gcc/testsuite/g++.dg/modules/pr98741_d.C | 3 + 6 files changed, 132 insertions(+), 76 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/pr98741_a.H create mode 100644 gcc/testsuite/g++.dg/modules/pr98741_b.H create mode 100644 gcc/testsuite/g++.dg/modules/pr98741_c.C create mode 100644 gcc/testsuite/g++.dg/modules/pr98741_d.C diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 691381bf995..3d17b8ddcdb 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -3873,6 +3873,9 @@ module_state_hash::equal (const value_type existing, /* Mapper name. */ static const char *module_mapper_name; +/* Deferred import queue (FIFO). */ +static vec *pending_imports; + /* CMI repository path and workspace. */ static char *cmi_repo; static size_t cmi_repo_length; @@ -18944,7 +18947,7 @@ declare_module (module_state *module, location_t from_loc, bool exporting_p, gcc_assert (global_namespace == current_scope ()); module_state *current = (*modules)[0]; - if (module_purview_p () || module->loadedness != ML_NONE) + if (module_purview_p () || module->loadedness > ML_CONFIG) { error_at (from_loc, module_purview_p () ? G_("module already declared") @@ -19275,6 +19278,70 @@ module_begin_main_file (cpp_reader *reader, line_maps *lmaps, } } +/* Process the pending_import queue, making sure we know the + filenames. */ + +static void +name_pending_imports (cpp_reader *reader, bool at_end) +{ + auto *mapper = get_mapper (cpp_main_loc (reader)); + + bool only_headers = (flag_preprocess_only + && !bool (mapper->get_flags () & Cody::Flags::NameOnly) + && !cpp_get_deps (reader)); + if (at_end + && (!vec_safe_length (pending_imports) || only_headers)) + /* Not doing anything. */ + return; + + timevar_start (TV_MODULE_MAPPER); + + dump.push (NULL); + dump () && dump ("Resolving direct import names"); + + mapper->Cork (); + for (unsigned ix = 0; ix != pending_imports->length (); ix++) + { + module_state *module = (*pending_imports)[ix]; + gcc_checking_assert (module->is_direct ()); + if (!module->filename) + { + Cody::Flags flags + = (flag_preprocess_only ? Cody::Flags::None + : Cody::Flags::NameOnly); + + if (only_headers && !module->is_header ()) + ; + else if (module->module_p + && (module->is_partition () || module->exported_p)) + mapper->ModuleExport (module->get_flatname (), flags); + else + mapper->ModuleImport (module->get_flatname (), flags); + } + } + + auto response = mapper->Uncork (); + auto r_iter = response.begin (); + for (unsigned ix = 0; ix != pending_imports->length (); ix++) + { + module_state *module = (*pending_imports)[ix]; + gcc_checking_assert (module->is_direct ()); + if (only_headers && !module->is_header ()) + ; + else if (!module->filename) + { + Cody::Packet const &p = *r_iter; + ++r_iter; + + module->set_filename (p); + } + } + + dump.pop (0); + + timevar_stop (TV_MODULE_MAPPER); +} + /* We've just lexed a module-specific control line for MODULE. Mark the module as a direct import, and possibly load up its macro state. Returns the primary module, if this is a module @@ -19322,17 +19389,22 @@ preprocess_module (module_state *module, location_t from_loc, } } + auto desired = ML_CONFIG; if (is_import - && !module->is_module () && module->is_header () - && module->loadedness < ML_PREPROCESSOR + && module->is_header () && (!cpp_get_options (reader)->preprocessed || cpp_get_options (reader)->directives_only)) + /* We need preprocessor state now. */ + desired = ML_PREPROCESSOR; + + if (!is_import || module->loadedness < desired) { - timevar_start (TV_MODULE_IMPORT); - unsigned n = dump.push (module); + vec_safe_push (pending_imports, module); - if (module->loadedness == ML_NONE) + if (desired == ML_PREPROCESSOR) { + name_pending_imports (reader, false); + unsigned pre_hwm = 0; /* Preserve the state of the line-map. */ @@ -19345,25 +19417,38 @@ preprocess_module (module_state *module, location_t from_loc, spans.maybe_init (); spans.close (); - if (!module->filename) + timevar_start (TV_MODULE_IMPORT); + + /* Load the config of each pending import -- we must assign + module numbers monotonically. */ + for (unsigned ix = 0; ix != pending_imports->length (); ix++) { - auto *mapper = get_mapper (cpp_main_loc (reader)); - auto packet = mapper->ModuleImport (module->get_flatname ()); - module->set_filename (packet); + auto *import = (*pending_imports)[ix]; + if (!(import->module_p + && (import->is_partition () || import->exported_p)) + && import->loadedness == ML_NONE + && (import->is_header () || !flag_preprocess_only)) + { + unsigned n = dump.push (import); + import->do_import (reader, true); + dump.pop (n); + } } - module->do_import (reader, true); + vec_free (pending_imports); /* Restore the line-map state. */ linemap_module_restore (line_table, pre_hwm); spans.open (); - } - if (module->loadedness < ML_PREPROCESSOR) - if (module->read_preprocessor (true)) - module->import_macros (); + /* Now read the preprocessor state of this particular + import. */ + unsigned n = dump.push (module); + if (module->read_preprocessor (true)) + module->import_macros (); + dump.pop (n); - dump.pop (n); - timevar_stop (TV_MODULE_IMPORT); + timevar_stop (TV_MODULE_IMPORT); + } } return is_import ? NULL : get_primary (module); @@ -19377,68 +19462,13 @@ preprocess_module (module_state *module, location_t from_loc, void preprocessed_module (cpp_reader *reader) { - auto *mapper = get_mapper (cpp_main_loc (reader)); + name_pending_imports (reader, true); + vec_free (pending_imports); spans.maybe_init (); spans.close (); - /* Stupid GTY doesn't grok a typedef here. And using type = is, too - modern. */ -#define iterator hash_table::iterator - /* using iterator = hash_table::iterator; */ - - /* Walk the module hash, asking for the names of all unknown - direct imports and informing of an export (if that's what we - are). Notice these are emitted even when preprocessing as they - inform the server of dependency edges. */ - timevar_start (TV_MODULE_MAPPER); - - dump.push (NULL); - dump () && dump ("Resolving direct import names"); - - if (!flag_preprocess_only - || bool (mapper->get_flags () & Cody::Flags::NameOnly) - || cpp_get_deps (reader)) - { - mapper->Cork (); - iterator end = modules_hash->end (); - for (iterator iter = modules_hash->begin (); iter != end; ++iter) - { - module_state *module = *iter; - if (module->is_direct () && !module->filename) - { - Cody::Flags flags - = (flag_preprocess_only ? Cody::Flags::None - : Cody::Flags::NameOnly); - - if (module->module_p - && (module->is_partition () || module->exported_p)) - mapper->ModuleExport (module->get_flatname (), flags); - else - mapper->ModuleImport (module->get_flatname (), flags); - } - } - - auto response = mapper->Uncork (); - auto r_iter = response.begin (); - for (iterator iter = modules_hash->begin (); iter != end; ++iter) - { - module_state *module = *iter; - - if (module->is_direct () && !module->filename) - { - Cody::Packet const &p = *r_iter; - ++r_iter; - - module->set_filename (p); - } - } - } - - dump.pop (0); - - timevar_stop (TV_MODULE_MAPPER); - + using iterator = hash_table::iterator; if (mkdeps *deps = cpp_get_deps (reader)) { /* Walk the module hash, informing the dependency machinery. */ @@ -19462,6 +19492,8 @@ preprocessed_module (cpp_reader *reader) if (flag_header_unit && !flag_preprocess_only) { + /* Find the main module -- remember, it's not yet in the module + array. */ iterator end = modules_hash->end (); for (iterator iter = modules_hash->begin (); iter != end; ++iter) { @@ -19473,7 +19505,6 @@ preprocessed_module (cpp_reader *reader) } } } -#undef iterator } /* VAL is a global tree, add it to the global vec if it is diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 6a4c10681a3..8e3038cb7ad 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -367,6 +367,11 @@ append_imported_binding_slot (tree *slot, tree name, unsigned ix) last->indices[off].base = ix; last->indices[off].span = 1; last->slots[off] = NULL_TREE; + /* Check monotonicity. */ + gcc_checking_assert (last[off ? 0 : -1] + .indices[off ? off - 1 + : BINDING_VECTOR_SLOTS_PER_CLUSTER - 1] + .base < ix); return &last->slots[off]; } diff --git a/gcc/testsuite/g++.dg/modules/pr98741_a.H b/gcc/testsuite/g++.dg/modules/pr98741_a.H new file mode 100644 index 00000000000..c8d5d6604bc --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr98741_a.H @@ -0,0 +1,7 @@ +// PR 98741 incorrect order of module number assignments +// { dg-additional-options {-fmodule-header} } +// { dg-module-cmi {} } + +namespace foo +{ +} diff --git a/gcc/testsuite/g++.dg/modules/pr98741_b.H b/gcc/testsuite/g++.dg/modules/pr98741_b.H new file mode 100644 index 00000000000..c2bdf84ca9c --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr98741_b.H @@ -0,0 +1,6 @@ +// { dg-additional-options {-fmodule-header} } +// { dg-module-cmi {} } + +namespace foo +{ +} diff --git a/gcc/testsuite/g++.dg/modules/pr98741_c.C b/gcc/testsuite/g++.dg/modules/pr98741_c.C new file mode 100644 index 00000000000..60bbe327996 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr98741_c.C @@ -0,0 +1,4 @@ +// { dg-additional-options {-fmodules-ts} } +export module Foo; +// { dg-module-cmi {Foo} } +import "pr98741_a.H"; diff --git a/gcc/testsuite/g++.dg/modules/pr98741_d.C b/gcc/testsuite/g++.dg/modules/pr98741_d.C new file mode 100644 index 00000000000..b8f48652e8e --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr98741_d.C @@ -0,0 +1,3 @@ +// { dg-additional-options {-fmodules-ts} } +module Foo; +import "pr98741_b.H"; -- 2.30.2