[Bug c++/114630] New: [modules] building module with submodule causes corrupt gcm

[Bug c++/114630] New: [modules] building module with submodule causes corrupt gcm

[nickbegg at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630

            Bug ID: 114630
           Summary: [modules] building module with submodule causes
                    corrupt gcm
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: nickbegg at gmail dot com
  Target Milestone: ---

Building module code with a submodule produces - failed to load pendings for
‘std::__format::__formatter_int’

This is similar to PR99426. Using test code very similar to PR113930 and
PR114229. this is compiling with a trunk debug gcc, with the patch proposed for
PR99426 applied. 

% /home/nick/inst/gcc-trunk-debug/bin/g++ --version
g++ (GCC) 14.0.1 20240405 (experimental)

Test code - 

//  modA.mpp
module;

#include <format>

export module modA;
import modB;

//  modB.mpp
module;

#include <format>

export module modB;

//  main.cpp
#include <format>

import modA;

//

Compiling trips over when loading modA when compiling main.cpp. Output -

modB.mpp:

 /home/nick/inst/gcc-trunk-debug/bin/g++   -g -std=gnu++23 -Wall -Wextra
-freport-bug --param=ggc-min-expand=10000 -MD -MT
CMakeFiles/moduleMin.dir/modB.mpp.o -MF CMakeFiles/moduleMin.dir/modB.mpp.o.d
-fmodules-ts -fmodule-mapper=CMakeFiles/moduleMin.dir/modB.mpp.o.modmap -MD
-fdeps-format=p1689r5 -x c++ -o CMakeFiles/moduleMin.dir/modB.mpp.o -c
/home/nick/src/moduleMin/modB.mpp

modA.mpp:

/home/nick/inst/gcc-trunk-debug/bin/g++   -g -std=gnu++23 -Wall -Wextra
-freport-bug --param=ggc-min-expand=10000 -MD -MT
CMakeFiles/moduleMin.dir/modA.mpp.o -MF CMakeFiles/moduleMin.dir/modA.mpp.o.d
-fmodules-ts -fmodule-mapper=CMakeFiles/moduleMin.dir/modA.mpp.o.modmap -MD
-fdeps-format=p1689r5 -x c++ -o CMakeFiles/moduleMin.dir/modA.mpp.o -c
/home/nick/src/moduleMin/modA.mpp

main.cpp: 

/home/nick/inst/gcc-trunk-debug/bin/g++   -g -std=gnu++23 -Wall -Wextra
-freport-bug --param=ggc-min-expand=10000 -MD -MT
CMakeFiles/moduleMin.dir/main.cpp.o -MF CMakeFiles/moduleMin.dir/main.cpp.o.d
-fmodules-ts -fmodule-mapper=CMakeFiles/moduleMin.dir/main.cpp.o.modmap -MD
-fdeps-format=p1689r5 -x c++ -o CMakeFiles/moduleMin.dir/main.cpp.o -c
/home/nick/src/moduleMin/main.cpp

In module imported at /home/nick/src/moduleMin/main.cpp:3:1:
modA: error: failed to read compiled module cluster 3008: Bad file data
modA: note: compiled module file is ‘CMakeFiles/moduleMin.dir/modA.gcm’
modA: error: failed to read compiled module cluster 3009: Bad file data
In file included from /home/nick/src/moduleMin/main.cpp:1:
/home/nick/inst/gcc-trunk-debug/include/c++/14.0.1/format:1899:35: fatal error:
failed to load pendings for ‘std::__format::__formatter_int’
 1899 |         return _M_f._M_parse<char>(__pc);
      |                ~~~~~~~~~~~~~~~~~~~^~~~~~
compilation terminated.

[Bug c++/114630] [14 Regression] [modules] building module with submodule causes corrupt gcm

[ppalka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630

Patrick Palka <ppalka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2024-04-08
     Ever confirmed|0                           |1
   Target Milestone|---                         |14.0
             Status|UNCONFIRMED                 |NEW
            Summary|[modules] building module   |[14 Regression] [modules]
                   |with submodule causes       |building module with
                   |corrupt gcm                 |submodule causes corrupt
                   |                            |gcm
                 CC|                            |nshead at gcc dot gnu.org,
                   |                            |ppalka at gcc dot gnu.org
      Known to work|                            |13.2.0

--- Comment #1 from Patrick Palka <ppalka at gcc dot gnu.org> ---
Thanks for the report, reduced:

$ cat 114630_a.C
module;

#include "format.ii"

export module modB;

$ cat 114630_b.C
module;

#include "format.ii"

export module modA;
import modB;

$ cat 114630_c.C
#include "format.ii"

import modA;

$ cat format.ii
template<typename _CharT>
void _M_do_parse() {
  struct A { };
  struct B { };
}

template<typename, typename>
struct formatter;

template<typename _Tp, typename _CharT>
requires __is_same(_Tp, int)
struct formatter<_Tp, _CharT> { };

template<typename _Tp, typename _CharT>
requires __is_same(_Tp, int*)
struct formatter<_Tp, _CharT> { };

template<> struct formatter<int, char> {
  void parse() { _M_do_parse<int>(); }
};

$ g++ -fmodules-ts -std=c++20 114630_*.C
In module imported at 114630_c.C:3:1:
modA: error: failed to read compiled module cluster 5: Bad file data
modA: note: compiled module file is ‘gcm.cache/modA.gcm’
In file included from 114630_c.C:1:
format.ii:19:34: fatal error: failed to load pendings for ‘::_M_do_parse’
   19 |   void parse() { _M_do_parse<int>(); }
      |                  ~~~~~~~~~~~~~~~~^~
compilation terminated.


We started rejecting this after r14-8408.  We seem to be streaming in the
definition of _M_do_parse<int>() twice, dunno if that's expected or not.

[Bug c++/114630] [14 Regression] [modules] building module with submodule causes corrupt gcm

[nshead at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630

--- Comment #2 from Nathaniel Shead <nshead at gcc dot gnu.org> ---
Created attachment 57919
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57919&action=edit
untested workaround

It looks like r14-8408 just exposed a pre-existing problem by no longer always
discarding emission of partial specialisations when declared in the GMF. But we
can get the same result without that change by declaring them in the module
purview under `extern "C++"`.

The attached patch fixes the issue with GM partial specialisations always being
emitted, which fixes the original test case but doesn't solve the underlying
issue: as can be shown by adding `formatter<int, char> f;` into the module
purview of `modA` and `modB` to make the declarations reachable again.

[Bug c++/114630] [14/15 Regression] [modules] building module with submodule causes corrupt gcm

[ppalka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630

Patrick Palka <ppalka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org

--- Comment #3 from Patrick Palka <ppalka at gcc dot gnu.org> ---
The following hack which forces instantiations induced from the GMF to actually
get instantiated within the GMF seems to fix the underlying issue:

diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
index 1339f210dde..dab98c02a63 100644
--- a/gcc/cp/decl2.cc
+++ b/gcc/cp/decl2.cc
@@ -2549,7 +2549,7 @@ decl_needed_p (tree decl)
 /* If necessary, write out the vtables for the dynamic class CTYPE.
    Returns true if any vtables were emitted.  */

-static bool
+bool
 maybe_emit_vtables (tree ctype, vec<tree> &consteval_vtables)
 {
   tree vtbl;
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 50d3ad35b61..7b91e2b6499 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -15173,6 +15173,23 @@ cp_parser_module_declaration (cp_parser *parser,
module_parse mp_state,
     }
   else
     {
+      if (mp_state == MP_GLOBAL)
+       /* ??? Make sure used specializations and vtables used within the
+          GMF are instantiated before starting the module purview.  */
+       for (int i = 0; i < 10; i++)
+         {
+           instantiate_pending_templates (0);
+           extern bool maybe_emit_vtables (tree, vec<tree> &);
+           tree t;
+           auto_vec<tree> consteval_vtables;
+           at_eof = 1;
+           for (int i = keyed_classes->length ();
+                keyed_classes->iterate (--i, &t);)
+             if (maybe_emit_vtables (t, consteval_vtables))
+               keyed_classes->unordered_remove (i);
+           at_eof = 0;
+         }
+
       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);


But I don't really understand why..  What changes is that for 114630_b.C we now
instantiate the local types _M_do_parse<int>()::A/B locally at the end of the
GMF rather than at EOF, and later when doing the import we deduplicate their
imported definition rather than installing their imported definition.  And in
114630_c.C we no longer try to stream in the definition of _M_do_parse<int>()
twice, for some reason.

Maybe related, but I noticed that for

module;
template<class T> int f() { return 42; }
inline int x = f<int>();
export module foo;

without the above fix we instantiate f<int>() at EOF time and so
set_instantiating_module sets DECL_MODULE_PURVIEW_P for it, even though the
instantiation was induced from the GMF not the module purview.  With the above
fix we now instantiate f<int>() while still within the GMF and no longer set
DECL_MODULE_PURVIEW_P for f<int>().  I wonder to what extent
DECL_MODULE_PURVIEW_P matters on implicit instantiations?

[Bug c++/114630] [14/15 Regression] [modules] building module with submodule causes corrupt gcm

[ppalka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630

--- Comment #4 from Patrick Palka <ppalka at gcc dot gnu.org> ---
(In reply to Patrick Palka from comment #3)
> And in 114630_c.C we no longer try to stream in the definition
> of _M_do_parse<int>() twice, for some reason.
Oops, looks like we still stream in the definition twice with the fix but the
local types therein get deduplicated correctly now.

[Bug c++/114630] [14/15 Regression] [modules] building module with submodule causes corrupt gcm

[nshead at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630

--- Comment #5 from Nathaniel Shead <nshead at gcc dot gnu.org> ---
I've been testing a patch series that tries to solve this from the other end
(by remembering whether we were in the module purview when we deferred the
instantiation and restoring that state within reopen_tinst_level, similarly to
how e.g. input_location is handled for error messages).

AFAICT, DECL_MODULE_PURVIEW_P on template instantiations should only matter for
determining whether it might be discarded (i.e. applying DB_UNREACHED_BIT);
otherwise, in general purviewness and attachment should be determined by the
base template, as implemented in 'get_originating_module_decl'.

That said, my version doesn't actually solve the underlying issue: my
adjustment of your testcase to put instantiations into the module purview (and
force emission there) still gives the same error for me.  So it sounds like
there's something else being affected by DECL_MODULE_PURVIEW_P being set when
maybe it shouldn't for an explicit specialisation.

[Bug c++/114630] [14/15 Regression] [modules] building module with submodule causes corrupt gcm

[ppalka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630

--- Comment #6 from Patrick Palka <ppalka at gcc dot gnu.org> ---
(In reply to Nathaniel Shead from comment #5)
> That said, my version doesn't actually solve the underlying issue: my
> adjustment of your testcase to put instantiations into the module purview
> (and force emission there) still gives the same error for me.  So it sounds
> like there's something else being affected by DECL_MODULE_PURVIEW_P being
> set when maybe it shouldn't for an explicit specialisation.
Interesting, I guess one difference with your approach is that for 114630_b.C
we'll still end up installing the imported instantiations of
_M_do_parse<int>()::A/B instead of instantiating them ourselves (within the
GMF), and so DECL_MODULE_IMPORT_P will be set on these local type
instantiations, and yet not on _M_do_parse<int>() because this function
specialization was formed (but not instantiated) within the GMF.

That seems like a weird inconsistency, that DECL_MODULE_IMPORT_P is set on the
local types but not on the containing function..

[Bug c++/114630] [14/15 Regression] [modules] building module with submodule causes corrupt gcm

[ppalka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630

--- Comment #7 from Patrick Palka <ppalka at gcc dot gnu.org> ---
The following seems to minimally fix it:

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index c35e70b8cb8..57ccaec5ebd 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -6798,7 +6798,7 @@ trees_in::core_vals (tree t)
               body) anyway.  */
            decl = maybe_duplicate (decl);

-           if (!DECL_P (decl) || DECL_CHAIN (decl))
+           if (!DECL_P (decl) || (*chain && *chain != decl))
              {
                set_overrun ();
                break;

In 114630_c.C end up streaming the definition of _M_do_parse<int>() twice, once
from modB and once from the transitive import of modA (which seems wasteful). 
The second time around the chain of local types _M_do_parse<int>::A/B get
streamed as simple back references and therefore on stream-in they'll already
have DECL_CHAIN properly set, so we shouldn't give up just because DECL_CHAIN
is already set.  Rather, only give up if DECL_CHAIN of the previous decl isn't
what we expect it to be, i.e. either empty or what we want to set it to.

This raises the question, why do we stream the definition of _M_do_parse<int>()
from the GMF of modA even though the same definition is already reachable (and
has been deduplicated) from the import of modB?  Seems like this is something
we could/should avoid.

[Bug c++/114630] [14/15 Regression] [modules] building module with submodule causes corrupt gcm

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630

--- Comment #8 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Nathaniel Shead <nshead@gcc.gnu.org>:

https://gcc.gnu.org/g:02917ac4528e32d1b2d0da5f45ef5937c56942cd

commit r15-101-g02917ac4528e32d1b2d0da5f45ef5937c56942cd
Author: Nathaniel Shead <nathanieloshead@gmail.com>
Date:   Thu Apr 11 19:15:35 2024 +1000

    c++: Don't emit unused GMF partial specializations [PR114630]

    The change in r14-8408 to also emit partial specializations in the
    global module fragment caused the regression in the linked PR; this
    patch fixes this by restricting emitted GM partial specializations to
    those that are actually used.

            PR c++/114630

    gcc/cp/ChangeLog:

            * module.cc (depset::hash::add_partial_entities): Mark GM
            specializations as unreached.
            (depset::hash::find_dependencies): Also reach entities in the
            DECL_TEMPLATE_SPECIALIZATIONS list.

    gcc/testsuite/ChangeLog:

            * g++.dg/modules/partial-3.C: New test.

    Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>

[Bug c++/114630] [14/15 Regression] [modules] building module with submodule causes corrupt gcm

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114630

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|14.0                        |14.2

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 14.1 is being released, retargeting bugs to GCC 14.2.