[Bug c++/99170] New: [modules] ICE in get_merge_kind with std::string NSDMI

[Bug c++/99170] New: [modules] ICE in get_merge_kind with std::string NSDMI

[remi.galanalfonso at gmail dot com]

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

            Bug ID: 99170
           Summary: [modules] ICE in get_merge_kind with std::string NSDMI
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: remi.galanalfonso at gmail dot com
  Target Milestone: ---

Created attachment 50223
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50223&action=edit
preprocessed file

Hello, I ran into the following ICE when testing modules a bit, it doesn't look
like any currently open module bug (reproduced here with a recent trunk, commit
f86e187e12d).

The reproducer is as follows:

$ cat test_modules.cpp 
export module test;

import <string>;

export class A {
        std::string str{"ayyy"};
};

The <string> header unit was compiled with the following:

$ g++ -std=c++20 -fmodules-ts -x c++-system-header string

And the command line used and result are the following (along with -v for
information):

$ g++ -v -save-temps -std=c++20 -fmodules-ts -c test_modules.cpp
Using built-in specs.
COLLECT_GCC=g++
Target: x86_64-pc-linux-gnu
Configured with: ./configure --prefix=/home/remi/Projects/gcc/build-install
--disable-multilib : (reconfigured) ./configure
--prefix=/home/remi/Projects/gcc/build-install --disable-multilib :
(reconfigured) ./configure --prefix=/home/remi/Projects/gcc/build-install
--disable-multilib CC=gcc-9 CXX=g++-9 --enable-languages=c,c++,fortran,lto,objc
--no-create --no-recursion
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 11.0.0 20210219 (experimental) (GCC) 
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-std=c++20' '-fmodules-ts' '-c'
'-shared-libgcc' '-mtune=generic' '-march=x86-64'

/home/remi/Projects/gcc/build-install/libexec/gcc/x86_64-pc-linux-gnu/11.0.0/cc1plus
-E -quiet -v -imultiarch x86_64-linux-gnu -D_GNU_SOURCE test_modules.cpp
-mtune=generic -march=x86-64 -std=c++20 -fmodules-ts -fpch-preprocess -o
test_modules.ii
ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
ignoring nonexistent directory
"/home/remi/Projects/gcc/build-install/lib/gcc/x86_64-pc-linux-gnu/11.0.0/../../../../x86_64-pc-linux-gnu/include"
#include "..." search starts here:
#include <...> search starts here:

/home/remi/Projects/gcc/build-install/lib/gcc/x86_64-pc-linux-gnu/11.0.0/../../../../include/c++/11.0.0

/home/remi/Projects/gcc/build-install/lib/gcc/x86_64-pc-linux-gnu/11.0.0/../../../../include/c++/11.0.0/x86_64-pc-linux-gnu

/home/remi/Projects/gcc/build-install/lib/gcc/x86_64-pc-linux-gnu/11.0.0/../../../../include/c++/11.0.0/backward

/home/remi/Projects/gcc/build-install/lib/gcc/x86_64-pc-linux-gnu/11.0.0/include
 /usr/local/include
 /home/remi/Projects/gcc/build-install/include

/home/remi/Projects/gcc/build-install/lib/gcc/x86_64-pc-linux-gnu/11.0.0/include-fixed
 /usr/include/x86_64-linux-gnu
 /usr/include
End of search list.
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-std=c++20' '-fmodules-ts' '-c'
'-shared-libgcc' '-mtune=generic' '-march=x86-64'

/home/remi/Projects/gcc/build-install/libexec/gcc/x86_64-pc-linux-gnu/11.0.0/cc1plus
-fpreprocessed test_modules.ii -quiet -dumpbase test_modules.cpp -dumpbase-ext
.cpp -mtune=generic -march=x86-64 -std=c++20 -version -fmodules-ts -o
test_modules.s
GNU C++20 (GCC) version 11.0.0 20210219 (experimental) (x86_64-pc-linux-gnu)
        compiled by GNU C version 11.0.0 20210218 (experimental), GMP version
6.2.1, MPFR version 4.1.0, MPC version 1.2.0, isl version none
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
GNU C++20 (GCC) version 11.0.0 20210219 (experimental) (x86_64-pc-linux-gnu)
        compiled by GNU C version 11.0.0 20210218 (experimental), GMP version
6.2.1, MPFR version 4.1.0, MPC version 1.2.0, isl version none
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
Compiler executable checksum: 8e9853a7486525ef35ca23365b38ea4d
test_modules.cpp:1:9: internal compiler error: in get_merge_kind, at
cp/module.cc:10158
    1 | export module test;
      |         ^~~~~~
0x699387 trees_out::get_merge_kind(tree_node*, depset*)
        ../.././gcc/cp/module.cc:10158
0xa13f63 trees_out::decl_value(tree_node*, depset*)
        ../.././gcc/cp/module.cc:7618
0xa14ebb trees_out::decl_node(tree_node*, walk_kind)
        ../.././gcc/cp/module.cc:8613
0xa15eb2 trees_out::tree_node(tree_node*)
        ../.././gcc/cp/module.cc:9168
0xa1b813 module_state::write_pendings(elf_out*, vec<depset*, va_heap, vl_ptr>,
depset::hash&, unsigned int, unsigned int*)
        ../.././gcc/cp/module.cc:15403
0xa1d050 module_state::write(elf_out*, cpp_reader*)
        ../.././gcc/cp/module.cc:17748
0xa1d71c finish_module_processing(cpp_reader*)
        ../.././gcc/cp/module.cc:19787
0x9b170b c_parse_final_cleanups()
        ../.././gcc/cp/decl2.c:5175
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.

The ICE disappears when removing the NSDMI, or when calling the default
constructor instead in the NSDMI.

I have attached the .ii file as advised in https://gcc.gnu.org/bugs/, even
though it doesn't look too interesting in this particular case.

[Bug c++/99170] [modules] ICE in get_merge_kind with std::string NSDMI

[remi.galanalfonso at gmail dot com]

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

--- Comment #1 from Rémi Galan Alfonso <remi.galanalfonso at gmail dot com> ---
Created attachment 50224
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50224&action=edit
preprocessed file

... And with the correct preprocessed file this time, apologies for my mistake.

[Bug c++/99170] [modules] ICE in get_merge_kind with std::string NSDMI

[nathan at gcc dot gnu.org]

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

Nathan Sidwell <nathan at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-02-24

[Bug c++/99170] [modules] ICE in get_merge_kind with std::string NSDMI

[nathan at gcc dot gnu.org]

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

--- Comment #2 from Nathan Sidwell <nathan at gcc dot gnu.org> ---
Thanks for checking this didn't seem a duplicate!

Reduced tescase:
// 99170_a.H
namespace STD {
class string {
public:
  template <typename T>
  string (const T *);
};
}

// 99170_b.C
export module test;
import "99170_a.H";
export class A {
  STD::string str{"ayyy"};
};

./cc1plus -fmodule-header -quiet -std=c++17 99170_a.H && ./cc1plus -fmodules-ts
-quiet -std=c++17 99170_b.C
99170_b.C:1:8: internal compiler error: in get_merge_kind, at
cp/module.cc:10169
    1 | export module test;
      |        ^~~~~~
0xcc98a8 trees_out::get_merge_kind(tree_node*, depset*)
        ../../../src/gcc/cp/module.cc:10169
0xcbd290 trees_out::decl_value(tree_node*, depset*)
        ../../../src/gcc/cp/module.cc:7629
0xcc2b15 trees_out::decl_node(tree_node*, walk_kind)
        ../../../src/gcc/cp/module.cc:8624
0xcc507f trees_out::tree_node(tree_node*)

[Bug c++/99170] [modules] ICE in get_merge_kind with std::string NSDMI

[cvs-commit at gcc dot gnu.org]

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

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Nathan Sidwell <nathan@gcc.gnu.org>:

https://gcc.gnu.org/g:499193a692efa33c9b2fe3ad8da0f4d5e5fd0e0c

commit r11-7493-g499193a692efa33c9b2fe3ad8da0f4d5e5fd0e0c
Author: Nathan Sidwell <nathan@acm.org>
Date:   Wed Mar 3 11:22:56 2021 -0800

    c++: Defer specialization registration [PR 99170]

    This defers inserting specializations into the specialization table,
    until we have completed their streaming.  When streaming a cluster we
    ensure that all imports are populated before any of the cluster, so
    they need no visibility of other specializations.  Further within the
    same import, we've already partitioned the graph, so no earlier
    cluster can be refering to a specialization in a later cluster.
    Inserting them early causes problems when other specializations of the
    same template are inserted.  (This doesn't fix 99170, but is a
    necessary change for that PR).

    Earlier on, I had less deferred processing, but it has become clearer
    that deferred worklists are the right way of handling a few things.
    This patch highlights a fixme, in that we're streaming a key twice,
    and need not do that, but I wanted to get correctness first.  Besides
    the second streaming will end up being a back reference, which is of
    course much cheaper than a by-value stream.

            PR c++/99170
            gcc/cp/
            * module.cc (trees_out::decl_value): Stream specialization keys
            after decl.
            (trees_in::decl_value): Stream them back and insert after
            completing the decl.
            (trees_out::key_mergeable): Drop some streaming here ...
            (trees_in::key_mergeable): ... and here.  Don't insert into
            specialization tables.

[Bug c++/99170] [modules] ICE in get_merge_kind with std::string NSDMI

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Nathan Sidwell <nathan@gcc.gnu.org>:

https://gcc.gnu.org/g:c390c5df71bbc95627c8e5e649a3161091239fd9

commit r11-7495-gc390c5df71bbc95627c8e5e649a3161091239fd9
Author: Nathan Sidwell <nathan@acm.org>
Date:   Wed Mar 3 12:38:20 2021 -0800

    c++: Defer cloning to post-loading [PR 99170]

    It turns out that cloning can cause use to load things. Specifically when
    checking paramter shadows (this is avoidable), and also the delete
    operator of a deleting dtor (not avoidable).  Doing that in the middle of
    loading is a bad thing.  This defers it to a post-load worklist.  If it
    causes more loading at that point there is no problem, as we've completed
    the first set of loads, bar this bit of cleanup.

    Again, this doesn't fix 99170, but is a step towards a solution.

            PR c++/99170
            gcc/cp/
            * module.cc (post_load_decls): New.
            (lazy_snum, recursive_lazy): Move earlier.
            (module_state::read_cluster): Push cloning onto post_load_decls.
            (post_load_processing): New.  Do the cloning here.
            (module_state::read_inits): Call post_load_processing.
            (module_state::read_language): Likewise.
            (lazy_load_binding, lazy_load_specializations): Likewise
            (lazy_load_members): Likewise

[Bug c++/99170] [modules] ICE in get_merge_kind with std::string NSDMI

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Nathan Sidwell <nathan@gcc.gnu.org>:

https://gcc.gnu.org/g:c778a237c1c605c2c5606c212c1ace756739442b

commit r11-7506-gc778a237c1c605c2c5606c212c1ace756739442b
Author: Nathan Sidwell <nathan@acm.org>
Date:   Wed Mar 3 10:09:41 2021 -0800

    c++: Redesign pending entity handling [PR 99170]

    This patch addresses 99170.  with modules (and in particular header
    units), one module can provide a (maybe nested) class or template and
    another module can provide a definition or (maybe partial)
    specialization of said entity, or member thereof.  when both are
    imported into a 3rd TU, and that TU instantiates or uses the class, it
    needs to stream in those entities (in general).  But how does it key
    those entities to the original?  It can't /just/ use the entity index,
    because, when header-units and/or partitions are in play, the entity
    index /is not unique/.  I had two complicated schemes that tried to
    unify that, but it failed.  Here's a simpler scheme.  Such pending
    entities are keyed to the namespace and identifier of the
    namespace-scope entity that contains them.  Thus the final TU needs to
    find that entity and look in a hash table for lists of sections that
    need loading just before instantiating a template or looking inside a
    class.

    I would like to make this more efficient, but given the complex scheme
    failed, I'm shooting for correctness right now.  There will be a
    follow up patch to complete the cleanup this enables.

            PR c++/99170
            gcc/cp/
            * cp-tree.h
            * lex.c (cxx_dup_lang_specific_decl): Adjust for module_attached_p
            rename.
            * module.cc (class pending_key): New.
            (default_hash_traits<pending_key>): New specialization.
            (pending_map_t): New typedef.
            (pending_table): Replace old table.
            (trees_out::lang_decl_bools): Adjust.
            (trees_in::lang_decl_bools): Adjust.
            (trees_in::install_entity): Drop pending member and specialization
            handling.
            (find_pending_key): New.
            (depset::hash::fiund_dependencies): Use it.
            (pendset_lazy_load): Delete.
            (module_state::write_cluster): Don't count pendings here.  Bye
            Duff's device-like thing.
            (module_state::write_pendings): Reimplement.
            (module_state::read_pendings): Reimplement.
            (lazy_specializations_p): Delete.
            (module_state::write): Adjust write_pendings call.
            (lazy_load_pendings): New.
            (lazy_load_specializations): Delete.
            (lazy_load_members): Delete.
            (init_modules): Adjust.
            * name-lookup.c (maybe_lazily_declare): Call lazy_load_pendings
            not lazy_load_members.
            (note_pending_specializations): Delete.
            (load_pending_specializations): Delete.
            * name-lookup.h (BINDING_VECTR_PENDING_SPECIALIZATIONS_P): Delete.
            (BINDING_VECTOR_PENDING_MEMBERS_P): Delete.
            (BINDING_VECTR_PENDING_MEMBERS_P): Delete.
            (note_pending_specializations): Delete.
            (load_pending_specializations): Delete.
            * pt.c (lookup_template_class_1): Call lazy_load_pendings not
            lazy_load_specializations.
            (instantiate_template_class_1): Likewise.
            (instantiate_decl): Call lazy_load_pendings.
            * typeck.c (complete_type): Likewise.
            gcc/testsuite/
            * g++.dg/modules/pr99170-1_a.H: New.
            * g++.dg/modules/pr99170-1_b.C: New.
            * g++.dg/modules/pr99170-2.h: New.
            * g++.dg/modules/pr99170-2_a.C: New.
            * g++.dg/modules/pr99170-2_b.C: New.
            * g++.dg/modules/pr99170-3_a.H: New.
            * g++.dg/modules/pr99170-3_b.C: New.
            * g++.dg/modules/inst-2_b.C: Adjust scan.
            * g++.dg/modules/inst-4_a.C: Adjust scan.
            * g++.dg/modules/inst-4_b.C: Adjust scan.
            * g++.dg/modules/member-def-1_b.C: Adjust scan.
            * g++.dg/modules/member-def-1_c.C: Adjust scan.
            * g++.dg/modules/tpl-spec-1_a.C: Adjust scan.
            * g++.dg/modules/tpl-spec-1_b.C: Adjust scan.
            * g++.dg/modules/tpl-spec-2_b.C: Adjust scan.
            * g++.dg/modules/tpl-spec-2_c.C: Adjust scan.
            * g++.dg/modules/tpl-spec-2_d.C: Adjust scan.
            * g++.dg/modules/tpl-spec-3_a.C: Adjust scan.
            * g++.dg/modules/tpl-spec-3_b.C: Adjust scan.
            * g++.dg/modules/tpl-spec-4_a.C: Adjust scan.
            * g++.dg/modules/tpl-spec-4_b.C: Adjust scan.
            * g++.dg/modules/tpl-spec-5_a.C: Adjust scan.
            * g++.dg/modules/tpl-spec-5_b.C: Adjust scan.

[Bug c++/99170] [modules] ICE in get_merge_kind with std::string NSDMI

[nathan at gcc dot gnu.org]

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

Nathan Sidwell <nathan at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #6 from Nathan Sidwell <nathan at gcc dot gnu.org> ---
 c778a237c1c 2021-03-03 | c++: Redesign pending entity handling [PR 99170]

[Bug c++/99170] [modules] ICE in get_merge_kind with std::string NSDMI

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Nathan Sidwell <nathan@gcc.gnu.org>:

https://gcc.gnu.org/g:9553c8a1b9dd2ca2f0f30d8b23fc6844c7e4a223

commit r11-7509-g9553c8a1b9dd2ca2f0f30d8b23fc6844c7e4a223
Author: Nathan Sidwell <nathan@acm.org>
Date:   Wed Mar 3 12:30:45 2021 -0800

    c++: Post-pending redesign cleanup [PR 99170]

    With pending entities reimplemented, the remaining use of uintset can just
    use a regular hash map -- I only used a uintset because it was there.
    So one adhoc hash-table/vector structure goes away.

            PR c++/99170
            gcc/cp/
            * module.cc (class uintset): Delete.
            (typedef attached_map_t): A hash map.
            (attached_table): Use attached_map_t.  Adjust uses ...
            (trees_out::decl_value, trees_in::decl_value): ... here ...
            (trees_out::key_mergeable): ... here ...
            (trees_in::key_mergeable): ... here ...
            (maybe_attach_decl): ... here ...
            (direct_import): ... and here.

[Bug c++/99170] [modules] ICE in get_merge_kind with std::string NSDMI

[remi.galanalfonso at gmail dot com]

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

--- Comment #8 from Rémi Galan Alfonso <remi.galanalfonso at gmail dot com> ---
I tested it and it works perfectly, thanks!