[2/4] c++/modules: Track module purview for deferred instantiations [PR114630]

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

When calling instantiate_pending_templates at end of parsing, any new
functions that are instantiated from this point have their module
purview set based on the current value of module_kind.

This is unideal, however, as the modules code will then treat these
instantiations as reachable and cause large swathes of the GMF to be
emitted into the module CMI, despite no code in the actual module
purview referencing it.

This patch fixes this by also remembering the value of module_kind when
the instantiation was deferred, and restoring it when doing this
deferred instantiation.  That way newly instantiated declarations
appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
instantiation was required, meaning that GMF entities won't be counted
as reachable unless referenced by an actually reachable entity.

Note that purviewness and attachment etc. is generally only determined
by the base template: this is purely for determining whether a
specialisation was declared in the module purview and hence whether it
should be streamed out.  See the comment on 'set_instantiating_module'.

	PR c++/114630
	PR c++/114795

gcc/cp/ChangeLog:

	* cp-tree.h (struct tinst_level): Add field for tracking
	module_kind.
	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
	(reopen_tinst_level): Restore module_kind from level.
	(instantiate_pending_templates): Save and restore module_kind so
	it isn't affected by reopen_tinst_level.

gcc/testsuite/ChangeLog:

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

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/cp-tree.h                     |  3 +++
 gcc/cp/pt.cc                         |  4 ++++
 gcc/testsuite/g++.dg/modules/gmf-3.C | 13 +++++++++++++
 3 files changed, 20 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C

On Wed, 1 May 2024, Nathaniel Shead wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> -- >8 --
> 
> When calling instantiate_pending_templates at end of parsing, any new
> functions that are instantiated from this point have their module
> purview set based on the current value of module_kind.
> 
> This is unideal, however, as the modules code will then treat these
> instantiations as reachable and cause large swathes of the GMF to be
> emitted into the module CMI, despite no code in the actual module
> purview referencing it.
> 
> This patch fixes this by also remembering the value of module_kind when
> the instantiation was deferred, and restoring it when doing this
> deferred instantiation.  That way newly instantiated declarations
> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> instantiation was required, meaning that GMF entities won't be counted
> as reachable unless referenced by an actually reachable entity.
> 
> Note that purviewness and attachment etc. is generally only determined
> by the base template: this is purely for determining whether a
> specialisation was declared in the module purview and hence whether it
> should be streamed out.  See the comment on 'set_instantiating_module'.
> 
> 	PR c++/114630
> 	PR c++/114795
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (struct tinst_level): Add field for tracking
> 	module_kind.
> 	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
> 	(reopen_tinst_level): Restore module_kind from level.
> 	(instantiate_pending_templates): Save and restore module_kind so
> 	it isn't affected by reopen_tinst_level.

LGTM.  Another approach is to instantiate all so-far deferred
instantiations and vtables once we reach the start of the module
purview, but your approach is much cleaner.

Note that deferred instantiation can induce more deferred instantiation,
but your approach will do the right thing here (module_kind will be
inherited from the point of instantiation).

What if an instantiation is needed from both the GMF and the module
purview?  Then IIUC it'll be instantiated as if from the GMF, which
seems right to me.

> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/gmf-3.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/cp-tree.h                     |  3 +++
>  gcc/cp/pt.cc                         |  4 ++++
>  gcc/testsuite/g++.dg/modules/gmf-3.C | 13 +++++++++++++
>  3 files changed, 20 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 1938ada0268..0e619120ccc 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -6626,6 +6626,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
>    /* The location where the template is instantiated.  */
>    location_t locus;
>  
> +  /* The module kind where the template is instantiated. */
> +  unsigned module_kind;
> +
>    /* errorcount + sorrycount when we pushed this level.  */
>    unsigned short errors;
>  
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 1c3eef60c06..401aa92bc3e 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -11277,6 +11277,7 @@ push_tinst_level_loc (tree tldcl, tree targs, location_t loc)
>    new_level->tldcl = tldcl;
>    new_level->targs = targs;
>    new_level->locus = loc;
> +  new_level->module_kind = module_kind;
>    new_level->errors = errorcount + sorrycount;
>    new_level->next = NULL;
>    new_level->refcount = 0;
> @@ -11345,6 +11346,7 @@ reopen_tinst_level (struct tinst_level *level)
>    for (t = level; t; t = t->next)
>      ++tinst_depth;
>  
> +  module_kind = level->module_kind;
>    set_refcount_ptr (current_tinst_level, level);
>    pop_tinst_level ();
>    if (current_tinst_level)
> @@ -27442,6 +27444,7 @@ instantiate_pending_templates (int retries)
>  {
>    int reconsider;
>    location_t saved_loc = input_location;
> +  unsigned saved_module_kind = module_kind;
>  
>    /* Instantiating templates may trigger vtable generation.  This in turn
>       may require further template instantiations.  We place a limit here
> @@ -27532,6 +27535,7 @@ instantiate_pending_templates (int retries)
>    while (reconsider);
>  
>    input_location = saved_loc;
> +  module_kind = saved_module_kind;
>  }
>  
>  /* Substitute ARGVEC into T, which is a list of initializers for
> diff --git a/gcc/testsuite/g++.dg/modules/gmf-3.C b/gcc/testsuite/g++.dg/modules/gmf-3.C
> new file mode 100644
> index 00000000000..e52ae904ea9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/gmf-3.C
> @@ -0,0 +1,13 @@
> +// PR c++/114630
> +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> +// { dg-module-cmi M }
> +
> +module;
> +template <typename> struct allocator {
> +  allocator() {}
> +};
> +template class allocator<wchar_t>;
> +export module M;
> +
> +// The whole GMF should be discarded here
> +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> -- 
> 2.43.2
> 
>

On Wed, May 01, 2024 at 10:11:20AM -0400, Patrick Palka wrote:
> On Wed, 1 May 2024, Nathaniel Shead wrote:
> 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > 
> > -- >8 --
> > 
> > When calling instantiate_pending_templates at end of parsing, any new
> > functions that are instantiated from this point have their module
> > purview set based on the current value of module_kind.
> > 
> > This is unideal, however, as the modules code will then treat these
> > instantiations as reachable and cause large swathes of the GMF to be
> > emitted into the module CMI, despite no code in the actual module
> > purview referencing it.
> > 
> > This patch fixes this by also remembering the value of module_kind when
> > the instantiation was deferred, and restoring it when doing this
> > deferred instantiation.  That way newly instantiated declarations
> > appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> > instantiation was required, meaning that GMF entities won't be counted
> > as reachable unless referenced by an actually reachable entity.
> > 
> > Note that purviewness and attachment etc. is generally only determined
> > by the base template: this is purely for determining whether a
> > specialisation was declared in the module purview and hence whether it
> > should be streamed out.  See the comment on 'set_instantiating_module'.
> > 
> > 	PR c++/114630
> > 	PR c++/114795
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* cp-tree.h (struct tinst_level): Add field for tracking
> > 	module_kind.
> > 	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
> > 	(reopen_tinst_level): Restore module_kind from level.
> > 	(instantiate_pending_templates): Save and restore module_kind so
> > 	it isn't affected by reopen_tinst_level.
> 
> LGTM.  Another approach is to instantiate all so-far deferred
> instantiations and vtables once we reach the start of the module
> purview, but your approach is much cleaner.
> 
> Note that deferred instantiation can induce more deferred instantiation,
> but your approach will do the right thing here (module_kind will be
> inherited from the point of instantiation).
> 
> What if an instantiation is needed from both the GMF and the module
> purview?  Then IIUC it'll be instantiated as if from the GMF, which
> seems right to me.
> 

Hm..., so I believe it'll be marked according to whichever instantiates
it "first", so if e.g. deferred from GMF and then instantiated
non-deferred from purview it'll be marked purview (and the deferred
instantiation will be skipped as it's already instantiated).  This could
mean it gets unnecessarily emitted if it only got instantiated because
it got used in e.g. a non-inline function body, I suppose; but that's
true in general actually, at the moment.

Maybe then a better approach would be to instead always use the
DECL_MODULE_PURVIEW_P of the instantiating template instead of the
global 'module_purview_p' function in 'set_instantiating_module'?
I think that should still make sure that instantiations are emitted
when they need to be (because they'll be reachable from a declaration
in the purview): I might try to experiment with something like this.

> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/modules/gmf-3.C: New test.
> > 
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> >  gcc/cp/cp-tree.h                     |  3 +++
> >  gcc/cp/pt.cc                         |  4 ++++
> >  gcc/testsuite/g++.dg/modules/gmf-3.C | 13 +++++++++++++
> >  3 files changed, 20 insertions(+)
> >  create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C
> > 
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index 1938ada0268..0e619120ccc 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -6626,6 +6626,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
> >    /* The location where the template is instantiated.  */
> >    location_t locus;
> >  
> > +  /* The module kind where the template is instantiated. */
> > +  unsigned module_kind;
> > +
> >    /* errorcount + sorrycount when we pushed this level.  */
> >    unsigned short errors;
> >  
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index 1c3eef60c06..401aa92bc3e 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -11277,6 +11277,7 @@ push_tinst_level_loc (tree tldcl, tree targs, location_t loc)
> >    new_level->tldcl = tldcl;
> >    new_level->targs = targs;
> >    new_level->locus = loc;
> > +  new_level->module_kind = module_kind;
> >    new_level->errors = errorcount + sorrycount;
> >    new_level->next = NULL;
> >    new_level->refcount = 0;
> > @@ -11345,6 +11346,7 @@ reopen_tinst_level (struct tinst_level *level)
> >    for (t = level; t; t = t->next)
> >      ++tinst_depth;
> >  
> > +  module_kind = level->module_kind;
> >    set_refcount_ptr (current_tinst_level, level);
> >    pop_tinst_level ();
> >    if (current_tinst_level)
> > @@ -27442,6 +27444,7 @@ instantiate_pending_templates (int retries)
> >  {
> >    int reconsider;
> >    location_t saved_loc = input_location;
> > +  unsigned saved_module_kind = module_kind;
> >  
> >    /* Instantiating templates may trigger vtable generation.  This in turn
> >       may require further template instantiations.  We place a limit here
> > @@ -27532,6 +27535,7 @@ instantiate_pending_templates (int retries)
> >    while (reconsider);
> >  
> >    input_location = saved_loc;
> > +  module_kind = saved_module_kind;
> >  }
> >  
> >  /* Substitute ARGVEC into T, which is a list of initializers for
> > diff --git a/gcc/testsuite/g++.dg/modules/gmf-3.C b/gcc/testsuite/g++.dg/modules/gmf-3.C
> > new file mode 100644
> > index 00000000000..e52ae904ea9
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/gmf-3.C
> > @@ -0,0 +1,13 @@
> > +// PR c++/114630
> > +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> > +// { dg-module-cmi M }
> > +
> > +module;
> > +template <typename> struct allocator {
> > +  allocator() {}
> > +};
> > +template class allocator<wchar_t>;
> > +export module M;
> > +
> > +// The whole GMF should be discarded here
> > +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> > -- 
> > 2.43.2
> > 
> > 
>

On 5/1/24 07:11, Patrick Palka wrote:
> On Wed, 1 May 2024, Nathaniel Shead wrote:
> 
>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>
>> -- >8 --
>>
>> When calling instantiate_pending_templates at end of parsing, any new
>> functions that are instantiated from this point have their module
>> purview set based on the current value of module_kind.
>>
>> This is unideal, however, as the modules code will then treat these
>> instantiations as reachable and cause large swathes of the GMF to be
>> emitted into the module CMI, despite no code in the actual module
>> purview referencing it.
>>
>> This patch fixes this by also remembering the value of module_kind when
>> the instantiation was deferred, and restoring it when doing this
>> deferred instantiation.  That way newly instantiated declarations
>> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
>> instantiation was required, meaning that GMF entities won't be counted
>> as reachable unless referenced by an actually reachable entity.
>>
>> Note that purviewness and attachment etc. is generally only determined
>> by the base template: this is purely for determining whether a
>> specialisation was declared in the module purview and hence whether it
>> should be streamed out.  See the comment on 'set_instantiating_module'.
>>
>> 	PR c++/114630
>> 	PR c++/114795
>>
>> gcc/cp/ChangeLog:
>>
>> 	* cp-tree.h (struct tinst_level): Add field for tracking
>> 	module_kind.
>> 	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
>> 	(reopen_tinst_level): Restore module_kind from level.
>> 	(instantiate_pending_templates): Save and restore module_kind so
>> 	it isn't affected by reopen_tinst_level.
> 
> LGTM.  Another approach is to instantiate all so-far deferred
> instantiations and vtables once we reach the start of the module
> purview, but your approach is much cleaner.

Hmm, I actually think I like the approach of instantiating at that point 
better, so that instantiations in the GMF can't depend on things in 
module purview.

And probably do the same again when switching to the private module 
fragment, when that's implemented.

Jason

On Thu, 2 May 2024, Nathaniel Shead wrote:

> On Wed, May 01, 2024 at 10:11:20AM -0400, Patrick Palka wrote:
> > On Wed, 1 May 2024, Nathaniel Shead wrote:
> > 
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > 
> > > -- >8 --
> > > 
> > > When calling instantiate_pending_templates at end of parsing, any new
> > > functions that are instantiated from this point have their module
> > > purview set based on the current value of module_kind.
> > > 
> > > This is unideal, however, as the modules code will then treat these
> > > instantiations as reachable and cause large swathes of the GMF to be
> > > emitted into the module CMI, despite no code in the actual module
> > > purview referencing it.
> > > 
> > > This patch fixes this by also remembering the value of module_kind when
> > > the instantiation was deferred, and restoring it when doing this
> > > deferred instantiation.  That way newly instantiated declarations
> > > appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> > > instantiation was required, meaning that GMF entities won't be counted
> > > as reachable unless referenced by an actually reachable entity.
> > > 
> > > Note that purviewness and attachment etc. is generally only determined
> > > by the base template: this is purely for determining whether a
> > > specialisation was declared in the module purview and hence whether it
> > > should be streamed out.  See the comment on 'set_instantiating_module'.
> > > 
> > > 	PR c++/114630
> > > 	PR c++/114795
> > > 
> > > gcc/cp/ChangeLog:
> > > 
> > > 	* cp-tree.h (struct tinst_level): Add field for tracking
> > > 	module_kind.
> > > 	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
> > > 	(reopen_tinst_level): Restore module_kind from level.
> > > 	(instantiate_pending_templates): Save and restore module_kind so
> > > 	it isn't affected by reopen_tinst_level.
> > 
> > LGTM.  Another approach is to instantiate all so-far deferred
> > instantiations and vtables once we reach the start of the module
> > purview, but your approach is much cleaner.
> > 
> > Note that deferred instantiation can induce more deferred instantiation,
> > but your approach will do the right thing here (module_kind will be
> > inherited from the point of instantiation).
> > 
> > What if an instantiation is needed from both the GMF and the module
> > purview?  Then IIUC it'll be instantiated as if from the GMF, which
> > seems right to me.
> > 
> 
> Hm..., so I believe it'll be marked according to whichever instantiates
> it "first", so if e.g. deferred from GMF and then instantiated
> non-deferred from purview it'll be marked purview (and the deferred
> instantiation will be skipped as it's already instantiated).  This could
> mean it gets unnecessarily emitted if it only got instantiated because
> it got used in e.g. a non-inline function body, I suppose; but that's
> true in general actually, at the moment.

FWIW I think the most relevant situation where we need to instantiate
immediately and can't defer until EOF is during constexpr evaluation of
a variable initializer.  (We also immediately instantiate variable
template specializations, and function template specializations with
deduced return type, but in these cases we'd always instantiate from the
first use, i.e. from the GMF in this scenario.)

So I guess a testcase for this situation could be something like:

    module;
    template<class T> constexpr int f(T t) { return t; }
    template int f(int);

    export module foo;
    constexpr int w = f(0);
    // f<int> ideally shouldn't be emitted in the CMI?

> 
> Maybe then a better approach would be to instead always use the
> DECL_MODULE_PURVIEW_P of the instantiating template instead of the
> global 'module_purview_p' function in 'set_instantiating_module'?
> I think that should still make sure that instantiations are emitted
> when they need to be (because they'll be reachable from a declaration
> in the purview): I might try to experiment with something like this.

The approach of instantiating all so-far deferred instantiations/vtables
at the start of the module purview should handle this properly too.

> 
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 
> > > 	* g++.dg/modules/gmf-3.C: New test.
> > > 
> > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > > ---
> > >  gcc/cp/cp-tree.h                     |  3 +++
> > >  gcc/cp/pt.cc                         |  4 ++++
> > >  gcc/testsuite/g++.dg/modules/gmf-3.C | 13 +++++++++++++
> > >  3 files changed, 20 insertions(+)
> > >  create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C
> > > 
> > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > > index 1938ada0268..0e619120ccc 100644
> > > --- a/gcc/cp/cp-tree.h
> > > +++ b/gcc/cp/cp-tree.h
> > > @@ -6626,6 +6626,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
> > >    /* The location where the template is instantiated.  */
> > >    location_t locus;
> > >  
> > > +  /* The module kind where the template is instantiated. */
> > > +  unsigned module_kind;
> > > +
> > >    /* errorcount + sorrycount when we pushed this level.  */
> > >    unsigned short errors;
> > >  
> > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > > index 1c3eef60c06..401aa92bc3e 100644
> > > --- a/gcc/cp/pt.cc
> > > +++ b/gcc/cp/pt.cc
> > > @@ -11277,6 +11277,7 @@ push_tinst_level_loc (tree tldcl, tree targs, location_t loc)
> > >    new_level->tldcl = tldcl;
> > >    new_level->targs = targs;
> > >    new_level->locus = loc;
> > > +  new_level->module_kind = module_kind;
> > >    new_level->errors = errorcount + sorrycount;
> > >    new_level->next = NULL;
> > >    new_level->refcount = 0;
> > > @@ -11345,6 +11346,7 @@ reopen_tinst_level (struct tinst_level *level)
> > >    for (t = level; t; t = t->next)
> > >      ++tinst_depth;
> > >  
> > > +  module_kind = level->module_kind;
> > >    set_refcount_ptr (current_tinst_level, level);
> > >    pop_tinst_level ();
> > >    if (current_tinst_level)
> > > @@ -27442,6 +27444,7 @@ instantiate_pending_templates (int retries)
> > >  {
> > >    int reconsider;
> > >    location_t saved_loc = input_location;
> > > +  unsigned saved_module_kind = module_kind;
> > >  
> > >    /* Instantiating templates may trigger vtable generation.  This in turn
> > >       may require further template instantiations.  We place a limit here
> > > @@ -27532,6 +27535,7 @@ instantiate_pending_templates (int retries)
> > >    while (reconsider);
> > >  
> > >    input_location = saved_loc;
> > > +  module_kind = saved_module_kind;
> > >  }
> > >  
> > >  /* Substitute ARGVEC into T, which is a list of initializers for
> > > diff --git a/gcc/testsuite/g++.dg/modules/gmf-3.C b/gcc/testsuite/g++.dg/modules/gmf-3.C
> > > new file mode 100644
> > > index 00000000000..e52ae904ea9
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/modules/gmf-3.C
> > > @@ -0,0 +1,13 @@
> > > +// PR c++/114630
> > > +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> > > +// { dg-module-cmi M }
> > > +
> > > +module;
> > > +template <typename> struct allocator {
> > > +  allocator() {}
> > > +};
> > > +template class allocator<wchar_t>;
> > > +export module M;
> > > +
> > > +// The whole GMF should be discarded here
> > > +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> > > -- 
> > > 2.43.2
> > > 
> > > 
> > 
> 
>

On 5/1/24 08:54, Patrick Palka wrote:
> On Thu, 2 May 2024, Nathaniel Shead wrote:
> 
>> On Wed, May 01, 2024 at 10:11:20AM -0400, Patrick Palka wrote:
>>> On Wed, 1 May 2024, Nathaniel Shead wrote:
>>>
>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>>>
>>>> -- >8 --
>>>>
>>>> When calling instantiate_pending_templates at end of parsing, any new
>>>> functions that are instantiated from this point have their module
>>>> purview set based on the current value of module_kind.
>>>>
>>>> This is unideal, however, as the modules code will then treat these
>>>> instantiations as reachable and cause large swathes of the GMF to be
>>>> emitted into the module CMI, despite no code in the actual module
>>>> purview referencing it.
>>>>
>>>> This patch fixes this by also remembering the value of module_kind when
>>>> the instantiation was deferred, and restoring it when doing this
>>>> deferred instantiation.  That way newly instantiated declarations
>>>> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
>>>> instantiation was required, meaning that GMF entities won't be counted
>>>> as reachable unless referenced by an actually reachable entity.
>>>>
>>>> Note that purviewness and attachment etc. is generally only determined
>>>> by the base template: this is purely for determining whether a
>>>> specialisation was declared in the module purview and hence whether it
>>>> should be streamed out.  See the comment on 'set_instantiating_module'.
>>>>
>>>> 	PR c++/114630
>>>> 	PR c++/114795
>>>>
>>>> gcc/cp/ChangeLog:
>>>>
>>>> 	* cp-tree.h (struct tinst_level): Add field for tracking
>>>> 	module_kind.
>>>> 	* pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
>>>> 	(reopen_tinst_level): Restore module_kind from level.
>>>> 	(instantiate_pending_templates): Save and restore module_kind so
>>>> 	it isn't affected by reopen_tinst_level.
>>>
>>> LGTM.  Another approach is to instantiate all so-far deferred
>>> instantiations and vtables once we reach the start of the module
>>> purview, but your approach is much cleaner.
>>>
>>> Note that deferred instantiation can induce more deferred instantiation,
>>> but your approach will do the right thing here (module_kind will be
>>> inherited from the point of instantiation).
>>>
>>> What if an instantiation is needed from both the GMF and the module
>>> purview?  Then IIUC it'll be instantiated as if from the GMF, which
>>> seems right to me.
>>>
>>
>> Hm..., so I believe it'll be marked according to whichever instantiates
>> it "first", so if e.g. deferred from GMF and then instantiated
>> non-deferred from purview it'll be marked purview (and the deferred
>> instantiation will be skipped as it's already instantiated).  This could
>> mean it gets unnecessarily emitted if it only got instantiated because
>> it got used in e.g. a non-inline function body, I suppose; but that's
>> true in general actually, at the moment.
> 
> FWIW I think the most relevant situation where we need to instantiate
> immediately and can't defer until EOF is during constexpr evaluation of
> a variable initializer.  (We also immediately instantiate variable
> template specializations, and function template specializations with
> deduced return type, but in these cases we'd always instantiate from the
> first use, i.e. from the GMF in this scenario.)
> 
> So I guess a testcase for this situation could be something like:
> 
>      module;
>      template<class T> constexpr int f(T t) { return t; }
>      template int f(int);
> 
>      export module foo;
>      constexpr int w = f(0);
>      // f<int> ideally shouldn't be emitted in the CMI?

Does the explicit instantiation change that at all?  Either way it seams 
like f and f<int> aren't decl-reachable from w if we determine that 
after constant evaluation.

Jason

On Wed, 1 May 2024, Jason Merrill wrote:

> On 5/1/24 08:54, Patrick Palka wrote:
> > On Thu, 2 May 2024, Nathaniel Shead wrote:
> > 
> > > On Wed, May 01, 2024 at 10:11:20AM -0400, Patrick Palka wrote:
> > > > On Wed, 1 May 2024, Nathaniel Shead wrote:
> > > > 
> > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > > > 
> > > > > -- >8 --
> > > > > 
> > > > > When calling instantiate_pending_templates at end of parsing, any new
> > > > > functions that are instantiated from this point have their module
> > > > > purview set based on the current value of module_kind.
> > > > > 
> > > > > This is unideal, however, as the modules code will then treat these
> > > > > instantiations as reachable and cause large swathes of the GMF to be
> > > > > emitted into the module CMI, despite no code in the actual module
> > > > > purview referencing it.
> > > > > 
> > > > > This patch fixes this by also remembering the value of module_kind
> > > > > when
> > > > > the instantiation was deferred, and restoring it when doing this
> > > > > deferred instantiation.  That way newly instantiated declarations
> > > > > appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> > > > > instantiation was required, meaning that GMF entities won't be counted
> > > > > as reachable unless referenced by an actually reachable entity.
> > > > > 
> > > > > Note that purviewness and attachment etc. is generally only determined
> > > > > by the base template: this is purely for determining whether a
> > > > > specialisation was declared in the module purview and hence whether it
> > > > > should be streamed out.  See the comment on
> > > > > 'set_instantiating_module'.
> > > > > 
> > > > > 	PR c++/114630
> > > > > 	PR c++/114795
> > > > > 
> > > > > gcc/cp/ChangeLog:
> > > > > 
> > > > > 	* cp-tree.h (struct tinst_level): Add field for tracking
> > > > > 	module_kind.
> > > > > 	* pt.cc (push_tinst_level_loc): Cache module_kind in
> > > > > new_level.
> > > > > 	(reopen_tinst_level): Restore module_kind from level.
> > > > > 	(instantiate_pending_templates): Save and restore module_kind
> > > > > so
> > > > > 	it isn't affected by reopen_tinst_level.
> > > > 
> > > > LGTM.  Another approach is to instantiate all so-far deferred
> > > > instantiations and vtables once we reach the start of the module
> > > > purview, but your approach is much cleaner.
> > > > 
> > > > Note that deferred instantiation can induce more deferred instantiation,
> > > > but your approach will do the right thing here (module_kind will be
> > > > inherited from the point of instantiation).
> > > > 
> > > > What if an instantiation is needed from both the GMF and the module
> > > > purview?  Then IIUC it'll be instantiated as if from the GMF, which
> > > > seems right to me.
> > > > 
> > > 
> > > Hm..., so I believe it'll be marked according to whichever instantiates
> > > it "first", so if e.g. deferred from GMF and then instantiated
> > > non-deferred from purview it'll be marked purview (and the deferred
> > > instantiation will be skipped as it's already instantiated).  This could
> > > mean it gets unnecessarily emitted if it only got instantiated because
> > > it got used in e.g. a non-inline function body, I suppose; but that's
> > > true in general actually, at the moment.
> > 
> > FWIW I think the most relevant situation where we need to instantiate
> > immediately and can't defer until EOF is during constexpr evaluation of
> > a variable initializer.  (We also immediately instantiate variable
> > template specializations, and function template specializations with
> > deduced return type, but in these cases we'd always instantiate from the
> > first use, i.e. from the GMF in this scenario.)
> > 
> > So I guess a testcase for this situation could be something like:
> > 
> >      module;
> >      template<class T> constexpr int f(T t) { return t; }
> >      template int f(int);
> > 
> >      export module foo;
> >      constexpr int w = f(0);
> >      // f<int> ideally shouldn't be emitted in the CMI?
> 
> Does the explicit instantiation change that at all?  Either way it seams like
> f and f<int> aren't decl-reachable from w if we determine that after constant
> evaluation.

Currently due to the bug in question we handle this as if the explicit
instantiation was performed from the module purview:

  module;
  template<class T> constexpr int f(T t) { return t; }

  export module foo;
  template int f(int);
  constexpr int w = f(0);

I think we do want to emit f<int> in that case?

On 5/1/24 12:11, Patrick Palka wrote:
> On Wed, 1 May 2024, Jason Merrill wrote:
> 
>> On 5/1/24 08:54, Patrick Palka wrote:
>>> On Thu, 2 May 2024, Nathaniel Shead wrote:
>>>
>>>> On Wed, May 01, 2024 at 10:11:20AM -0400, Patrick Palka wrote:
>>>>> On Wed, 1 May 2024, Nathaniel Shead wrote:
>>>>>
>>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>>>>>
>>>>>> -- >8 --
>>>>>>
>>>>>> When calling instantiate_pending_templates at end of parsing, any new
>>>>>> functions that are instantiated from this point have their module
>>>>>> purview set based on the current value of module_kind.
>>>>>>
>>>>>> This is unideal, however, as the modules code will then treat these
>>>>>> instantiations as reachable and cause large swathes of the GMF to be
>>>>>> emitted into the module CMI, despite no code in the actual module
>>>>>> purview referencing it.
>>>>>>
>>>>>> This patch fixes this by also remembering the value of module_kind
>>>>>> when
>>>>>> the instantiation was deferred, and restoring it when doing this
>>>>>> deferred instantiation.  That way newly instantiated declarations
>>>>>> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
>>>>>> instantiation was required, meaning that GMF entities won't be counted
>>>>>> as reachable unless referenced by an actually reachable entity.
>>>>>>
>>>>>> Note that purviewness and attachment etc. is generally only determined
>>>>>> by the base template: this is purely for determining whether a
>>>>>> specialisation was declared in the module purview and hence whether it
>>>>>> should be streamed out.  See the comment on
>>>>>> 'set_instantiating_module'.
>>>>>>
>>>>>> 	PR c++/114630
>>>>>> 	PR c++/114795
>>>>>>
>>>>>> gcc/cp/ChangeLog:
>>>>>>
>>>>>> 	* cp-tree.h (struct tinst_level): Add field for tracking
>>>>>> 	module_kind.
>>>>>> 	* pt.cc (push_tinst_level_loc): Cache module_kind in
>>>>>> new_level.
>>>>>> 	(reopen_tinst_level): Restore module_kind from level.
>>>>>> 	(instantiate_pending_templates): Save and restore module_kind
>>>>>> so
>>>>>> 	it isn't affected by reopen_tinst_level.
>>>>>
>>>>> LGTM.  Another approach is to instantiate all so-far deferred
>>>>> instantiations and vtables once we reach the start of the module
>>>>> purview, but your approach is much cleaner.
>>>>>
>>>>> Note that deferred instantiation can induce more deferred instantiation,
>>>>> but your approach will do the right thing here (module_kind will be
>>>>> inherited from the point of instantiation).
>>>>>
>>>>> What if an instantiation is needed from both the GMF and the module
>>>>> purview?  Then IIUC it'll be instantiated as if from the GMF, which
>>>>> seems right to me.
>>>>>
>>>>
>>>> Hm..., so I believe it'll be marked according to whichever instantiates
>>>> it "first", so if e.g. deferred from GMF and then instantiated
>>>> non-deferred from purview it'll be marked purview (and the deferred
>>>> instantiation will be skipped as it's already instantiated).  This could
>>>> mean it gets unnecessarily emitted if it only got instantiated because
>>>> it got used in e.g. a non-inline function body, I suppose; but that's
>>>> true in general actually, at the moment.
>>>
>>> FWIW I think the most relevant situation where we need to instantiate
>>> immediately and can't defer until EOF is during constexpr evaluation of
>>> a variable initializer.  (We also immediately instantiate variable
>>> template specializations, and function template specializations with
>>> deduced return type, but in these cases we'd always instantiate from the
>>> first use, i.e. from the GMF in this scenario.)
>>>
>>> So I guess a testcase for this situation could be something like:
>>>
>>>       module;
>>>       template<class T> constexpr int f(T t) { return t; }
>>>       template int f(int);
>>>
>>>       export module foo;
>>>       constexpr int w = f(0);
>>>       // f<int> ideally shouldn't be emitted in the CMI?
>>
>> Does the explicit instantiation change that at all?  Either way it seams like
>> f and f<int> aren't decl-reachable from w if we determine that after constant
>> evaluation.
> 
> Currently due to the bug in question we handle this as if the explicit
> instantiation was performed from the module purview:
> 
>    module;
>    template<class T> constexpr int f(T t) { return t; }
> 
>    export module foo;
>    template int f(int);
>    constexpr int w = f(0);
> 
> I think we do want to emit f<int> in that case?

Agreed, because f is decl-reachable from the explicit instantiation in 
the module purview.

I was suggesting that there's no effective difference between explicit 
instantiation in the GMF and implicit instantiation.

Jason

On 5/1/24 11:27 AM, Jason Merrill wrote:
> On 5/1/24 07:11, Patrick Palka wrote:
>> On Wed, 1 May 2024, Nathaniel Shead wrote:
>>
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>>
>>> -- >8 --
>>>
>>> When calling instantiate_pending_templates at end of parsing, any new
>>> functions that are instantiated from this point have their module
>>> purview set based on the current value of module_kind.
>>>
>>> This is unideal, however, as the modules code will then treat these
>>> instantiations as reachable and cause large swathes of the GMF to be
>>> emitted into the module CMI, despite no code in the actual module
>>> purview referencing it.
>>>
>>> This patch fixes this by also remembering the value of module_kind when
>>> the instantiation was deferred, and restoring it when doing this
>>> deferred instantiation.  That way newly instantiated declarations
>>> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
>>> instantiation was required, meaning that GMF entities won't be counted
>>> as reachable unless referenced by an actually reachable entity.
>>>
>>> Note that purviewness and attachment etc. is generally only determined
>>> by the base template: this is purely for determining whether a
>>> specialisation was declared in the module purview and hence whether it
>>> should be streamed out.  See the comment on 'set_instantiating_module'.
>>>
>>>     PR c++/114630
>>>     PR c++/114795
>>>
>>> gcc/cp/ChangeLog:
>>>
>>>     * cp-tree.h (struct tinst_level): Add field for tracking
>>>     module_kind.
>>>     * pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
>>>     (reopen_tinst_level): Restore module_kind from level.
>>>     (instantiate_pending_templates): Save and restore module_kind so
>>>     it isn't affected by reopen_tinst_level.
>>
>> LGTM.  Another approach is to instantiate all so-far deferred
>> instantiations and vtables once we reach the start of the module
>> purview, but your approach is much cleaner.
> 
> Hmm, I actually think I like the approach of instantiating at that point 
> better, so that instantiations in the GMF can't depend on things in 
> module purview.
> 
> And probably do the same again when switching to the private module 
> fragment, when that's implemented.

Patrick mentioned these patches again today; I still think this is the 
right approach, particularly for the private module fragment.

That said, could we just clear module_kind at EOF?  As you say, only the 
purviewness of the template matters for implicit instantiations.  It is 
seeming to me like "set_instantiating_module" should only apply for 
explicit instantiations, at the point of the explicit instantiation 
declaration.

Jason

On Wed, Jul 17, 2024 at 01:12:34PM -0400, Jason Merrill wrote:
> On 5/1/24 11:27 AM, Jason Merrill wrote:
> > On 5/1/24 07:11, Patrick Palka wrote:
> > > On Wed, 1 May 2024, Nathaniel Shead wrote:
> > > 
> > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > > 
> > > > -- >8 --
> > > > 
> > > > When calling instantiate_pending_templates at end of parsing, any new
> > > > functions that are instantiated from this point have their module
> > > > purview set based on the current value of module_kind.
> > > > 
> > > > This is unideal, however, as the modules code will then treat these
> > > > instantiations as reachable and cause large swathes of the GMF to be
> > > > emitted into the module CMI, despite no code in the actual module
> > > > purview referencing it.
> > > > 
> > > > This patch fixes this by also remembering the value of module_kind when
> > > > the instantiation was deferred, and restoring it when doing this
> > > > deferred instantiation.  That way newly instantiated declarations
> > > > appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> > > > instantiation was required, meaning that GMF entities won't be counted
> > > > as reachable unless referenced by an actually reachable entity.
> > > > 
> > > > Note that purviewness and attachment etc. is generally only determined
> > > > by the base template: this is purely for determining whether a
> > > > specialisation was declared in the module purview and hence whether it
> > > > should be streamed out.  See the comment on 'set_instantiating_module'.
> > > > 
> > > >     PR c++/114630
> > > >     PR c++/114795
> > > > 
> > > > gcc/cp/ChangeLog:
> > > > 
> > > >     * cp-tree.h (struct tinst_level): Add field for tracking
> > > >     module_kind.
> > > >     * pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
> > > >     (reopen_tinst_level): Restore module_kind from level.
> > > >     (instantiate_pending_templates): Save and restore module_kind so
> > > >     it isn't affected by reopen_tinst_level.
> > > 
> > > LGTM.  Another approach is to instantiate all so-far deferred
> > > instantiations and vtables once we reach the start of the module
> > > purview, but your approach is much cleaner.
> > 
> > Hmm, I actually think I like the approach of instantiating at that point
> > better, so that instantiations in the GMF can't depend on things in
> > module purview.
> > 
> > And probably do the same again when switching to the private module
> > fragment, when that's implemented.
> 
> Patrick mentioned these patches again today; I still think this is the right
> approach, particularly for the private module fragment.
> 
> That said, could we just clear module_kind at EOF?  As you say, only the
> purviewness of the template matters for implicit instantiations.  It is
> seeming to me like "set_instantiating_module" should only apply for explicit
> instantiations, at the point of the explicit instantiation declaration.
> 
> Jason
> 

Yup, I agree that this is the right approach, since as you say we'll
need to do this for private module fragment anyway; I've just left this
on the backburner because it didn't look particularly easy to do neatly
and have been focussing on crashes instead for now.

That sounds like a good idea, I've given that a shot and it succeeds
bootstrap + regtest (so far just modules.exp) on x86_64-pc-linux-gnu.
Here's the patch, OK for trunk if full regtest succeeds?

-- >8 --

Subject: [PATCH v2] c++/modules: Clear module_purview_p at EOF [PR114630]

When calling instantiate_pending_templates at end of parsing, any new
functions that are instantiated from this point have their module
purview set based on the current value of module_kind.

This is unideal, however, as the modules code will then treat these
instantiations as reachable and cause large swathes of the GMF to be
emitted into the module CMI, despite no code in the actual module
purview referencing it.

Note that purviewness and attachment etc. is generally only determined
by the base template: this is purely for determining whether a
specialisation was declared in the module purview and hence whether it
should be streamed out.  See the comment on 'set_instantiating_module'.

As such, this patch fixes the issue by clearing MK_PURVIEW and MK_ATTACH
from module_kind at EOF.  This ensures that deferred instantiations
don't get marked as purview and retained in the module CMI
unnecessarily.  We also add a new test to ensure that all code in the
standard library headers are properly discarded to prevent regressions.

For future work we probably want to eagerly perform any deferred
instantiations at the end of the global module fragment, which will
avoid this issue as well.  We will need to do this before any private
module fragments (once we support them) regardless.

	PR c++/114630
	PR c++/114795

gcc/cp/ChangeLog:

	* decl2.cc (c_parse_final_cleanups): Clear MK_PURVIEW and
	MK_ATTACH flags.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/xtreme-header.h: Include new header files.
	* g++.dg/modules/gmf-3.C: New test.
	* g++.dg/modules/gmf-4.C: New test.
	* g++.dg/modules/xtreme-header-8.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/decl2.cc                               |  4 +++
 gcc/testsuite/g++.dg/modules/gmf-3.C          | 13 +++++++++
 gcc/testsuite/g++.dg/modules/gmf-4.C          | 27 +++++++++++++++++++
 .../g++.dg/modules/xtreme-header-8.C          |  9 +++++++
 gcc/testsuite/g++.dg/modules/xtreme-header.h  | 25 +++++++++++------
 5 files changed, 70 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C
 create mode 100644 gcc/testsuite/g++.dg/modules/gmf-4.C
 create mode 100644 gcc/testsuite/g++.dg/modules/xtreme-header-8.C

diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
index 6d674684931..acabf65e136 100644
--- a/gcc/cp/decl2.cc
+++ b/gcc/cp/decl2.cc
@@ -5168,6 +5168,10 @@ c_parse_final_cleanups (void)
 
   /* FIXME - huh?  was  input_line -= 1;*/
 
+  /* Clear module kind so that deferred instantiations don't get unnecessarily
+     marked as purview, preventing GMF discarding.  */
+  module_kind &= ~(MK_PURVIEW | MK_ATTACH);
+
   /* We now have to write out all the stuff we put off writing out.
      These include:
 
diff --git a/gcc/testsuite/g++.dg/modules/gmf-3.C b/gcc/testsuite/g++.dg/modules/gmf-3.C
new file mode 100644
index 00000000000..e52ae904ea9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/gmf-3.C
@@ -0,0 +1,13 @@
+// PR c++/114630
+// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
+// { dg-module-cmi M }
+
+module;
+template <typename> struct allocator {
+  allocator() {}
+};
+template class allocator<wchar_t>;
+export module M;
+
+// The whole GMF should be discarded here
+// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
diff --git a/gcc/testsuite/g++.dg/modules/gmf-4.C b/gcc/testsuite/g++.dg/modules/gmf-4.C
new file mode 100644
index 00000000000..c95bc782cea
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/gmf-4.C
@@ -0,0 +1,27 @@
+// PR c++/114630
+// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
+// { dg-module-cmi M }
+
+// Deferred instantiation of GM virtual functions should not place
+// newly discovered declarations in the module purview
+
+module;
+
+template <typename T>
+void go() {
+  extern T fn_decl();
+}
+
+template <typename T>
+struct S {
+  virtual void f() {
+    go<char>();
+  }
+};
+
+S<int> s;
+
+export module M;
+
+// The whole GMF should be discarded here
+// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header-8.C b/gcc/testsuite/g++.dg/modules/xtreme-header-8.C
new file mode 100644
index 00000000000..b0d0ae87534
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/xtreme-header-8.C
@@ -0,0 +1,9 @@
+// { dg-additional-options "-fmodules-ts -fdump-lang-module" }
+// { dg-module-cmi empty }
+
+module;
+#include "xtreme-header.h"
+export module empty;
+
+// The whole GMF should be discarded here
+// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header.h b/gcc/testsuite/g++.dg/modules/xtreme-header.h
index 3147aaf00f4..6f98a3acecd 100644
--- a/gcc/testsuite/g++.dg/modules/xtreme-header.h
+++ b/gcc/testsuite/g++.dg/modules/xtreme-header.h
@@ -89,7 +89,6 @@
 
 // C++20
 #if __cplusplus > 201703
-#if 1
 #include <version>
 #include <barrier>
 #include <bit>
@@ -98,6 +97,7 @@
 #if __cpp_coroutines
 #include <coroutine>
 #endif
+#include <format>
 #include <latch>
 #include <numbers>
 #include <ranges>
@@ -106,24 +106,33 @@
 #include <span>
 #include <stop_token>
 #include <syncstream>
-#if 0
-// Unimplemented
-#include <format>
-#endif
-#endif
 #endif
 
 // C++23
 #if __cplusplus > 202002L
 #include <expected>
+#include <generator>
+#include <print>
 #include <spanstream>
 #include <stacktrace>
+#include <stdfloat>
 #if 0
 // Unimplemented
 #include <flat_map>
 #include <flat_set>
-#include <generator>
 #include <mdspan>
-#include <print>
+#endif
+#endif
+
+// C++26
+#if __cplusplus > 202302L
+#if 0
+// Unimplemented
+#include <debugging>
+#include <execution>
+#include <hazard_pointer>
+#include <linalg>
+#include <rcu>
+#include <text_encoding>
 #endif
 #endif

On 7/17/24 11:04 PM, Nathaniel Shead wrote:
> On Wed, Jul 17, 2024 at 01:12:34PM -0400, Jason Merrill wrote:
>> On 5/1/24 11:27 AM, Jason Merrill wrote:
>>> On 5/1/24 07:11, Patrick Palka wrote:
>>>> On Wed, 1 May 2024, Nathaniel Shead wrote:
>>>>
>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>>>>
>>>>> -- >8 --
>>>>>
>>>>> When calling instantiate_pending_templates at end of parsing, any new
>>>>> functions that are instantiated from this point have their module
>>>>> purview set based on the current value of module_kind.
>>>>>
>>>>> This is unideal, however, as the modules code will then treat these
>>>>> instantiations as reachable and cause large swathes of the GMF to be
>>>>> emitted into the module CMI, despite no code in the actual module
>>>>> purview referencing it.
>>>>>
>>>>> This patch fixes this by also remembering the value of module_kind when
>>>>> the instantiation was deferred, and restoring it when doing this
>>>>> deferred instantiation.  That way newly instantiated declarations
>>>>> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
>>>>> instantiation was required, meaning that GMF entities won't be counted
>>>>> as reachable unless referenced by an actually reachable entity.
>>>>>
>>>>> Note that purviewness and attachment etc. is generally only determined
>>>>> by the base template: this is purely for determining whether a
>>>>> specialisation was declared in the module purview and hence whether it
>>>>> should be streamed out.  See the comment on 'set_instantiating_module'.
>>>>>
>>>>>      PR c++/114630
>>>>>      PR c++/114795
>>>>>
>>>>> gcc/cp/ChangeLog:
>>>>>
>>>>>      * cp-tree.h (struct tinst_level): Add field for tracking
>>>>>      module_kind.
>>>>>      * pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
>>>>>      (reopen_tinst_level): Restore module_kind from level.
>>>>>      (instantiate_pending_templates): Save and restore module_kind so
>>>>>      it isn't affected by reopen_tinst_level.
>>>>
>>>> LGTM.  Another approach is to instantiate all so-far deferred
>>>> instantiations and vtables once we reach the start of the module
>>>> purview, but your approach is much cleaner.
>>>
>>> Hmm, I actually think I like the approach of instantiating at that point
>>> better, so that instantiations in the GMF can't depend on things in
>>> module purview.
>>>
>>> And probably do the same again when switching to the private module
>>> fragment, when that's implemented.
>>
>> Patrick mentioned these patches again today; I still think this is the right
>> approach, particularly for the private module fragment.
>>
>> That said, could we just clear module_kind at EOF?  As you say, only the
>> purviewness of the template matters for implicit instantiations.  It is
>> seeming to me like "set_instantiating_module" should only apply for explicit
>> instantiations, at the point of the explicit instantiation declaration.
>>
>> Jason
>>
> 
> Yup, I agree that this is the right approach, since as you say we'll
> need to do this for private module fragment anyway; I've just left this
> on the backburner because it didn't look particularly easy to do neatly
> and have been focussing on crashes instead for now.
> 
> That sounds like a good idea, I've given that a shot and it succeeds
> bootstrap + regtest (so far just modules.exp) on x86_64-pc-linux-gnu.
> Here's the patch, OK for trunk if full regtest succeeds?

Is there a test that an explicit instantiation in module purview is not 
discarded?  OK if so.

> -- >8 --
> 
> Subject: [PATCH v2] c++/modules: Clear module_purview_p at EOF [PR114630]
> 
> When calling instantiate_pending_templates at end of parsing, any new
> functions that are instantiated from this point have their module
> purview set based on the current value of module_kind.
> 
> This is unideal, however, as the modules code will then treat these
> instantiations as reachable and cause large swathes of the GMF to be
> emitted into the module CMI, despite no code in the actual module
> purview referencing it.
> 
> Note that purviewness and attachment etc. is generally only determined
> by the base template: this is purely for determining whether a
> specialisation was declared in the module purview and hence whether it
> should be streamed out.  See the comment on 'set_instantiating_module'.
> 
> As such, this patch fixes the issue by clearing MK_PURVIEW and MK_ATTACH
> from module_kind at EOF.  This ensures that deferred instantiations
> don't get marked as purview and retained in the module CMI
> unnecessarily.  We also add a new test to ensure that all code in the
> standard library headers are properly discarded to prevent regressions.
> 
> For future work we probably want to eagerly perform any deferred
> instantiations at the end of the global module fragment, which will
> avoid this issue as well.  We will need to do this before any private
> module fragments (once we support them) regardless.
> 
> 	PR c++/114630
> 	PR c++/114795
> 
> gcc/cp/ChangeLog:
> 
> 	* decl2.cc (c_parse_final_cleanups): Clear MK_PURVIEW and
> 	MK_ATTACH flags.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/xtreme-header.h: Include new header files.
> 	* g++.dg/modules/gmf-3.C: New test.
> 	* g++.dg/modules/gmf-4.C: New test.
> 	* g++.dg/modules/xtreme-header-8.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/decl2.cc                               |  4 +++
>   gcc/testsuite/g++.dg/modules/gmf-3.C          | 13 +++++++++
>   gcc/testsuite/g++.dg/modules/gmf-4.C          | 27 +++++++++++++++++++
>   .../g++.dg/modules/xtreme-header-8.C          |  9 +++++++
>   gcc/testsuite/g++.dg/modules/xtreme-header.h  | 25 +++++++++++------
>   5 files changed, 70 insertions(+), 8 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/gmf-4.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/xtreme-header-8.C
> 
> diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
> index 6d674684931..acabf65e136 100644
> --- a/gcc/cp/decl2.cc
> +++ b/gcc/cp/decl2.cc
> @@ -5168,6 +5168,10 @@ c_parse_final_cleanups (void)
>   
>     /* FIXME - huh?  was  input_line -= 1;*/
>   
> +  /* Clear module kind so that deferred instantiations don't get unnecessarily
> +     marked as purview, preventing GMF discarding.  */
> +  module_kind &= ~(MK_PURVIEW | MK_ATTACH);
> +
>     /* We now have to write out all the stuff we put off writing out.
>        These include:
>   
> diff --git a/gcc/testsuite/g++.dg/modules/gmf-3.C b/gcc/testsuite/g++.dg/modules/gmf-3.C
> new file mode 100644
> index 00000000000..e52ae904ea9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/gmf-3.C
> @@ -0,0 +1,13 @@
> +// PR c++/114630
> +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> +// { dg-module-cmi M }
> +
> +module;
> +template <typename> struct allocator {
> +  allocator() {}
> +};
> +template class allocator<wchar_t>;
> +export module M;
> +
> +// The whole GMF should be discarded here
> +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> diff --git a/gcc/testsuite/g++.dg/modules/gmf-4.C b/gcc/testsuite/g++.dg/modules/gmf-4.C
> new file mode 100644
> index 00000000000..c95bc782cea
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/gmf-4.C
> @@ -0,0 +1,27 @@
> +// PR c++/114630
> +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> +// { dg-module-cmi M }
> +
> +// Deferred instantiation of GM virtual functions should not place
> +// newly discovered declarations in the module purview
> +
> +module;
> +
> +template <typename T>
> +void go() {
> +  extern T fn_decl();
> +}
> +
> +template <typename T>
> +struct S {
> +  virtual void f() {
> +    go<char>();
> +  }
> +};
> +
> +S<int> s;
> +
> +export module M;
> +
> +// The whole GMF should be discarded here
> +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header-8.C b/gcc/testsuite/g++.dg/modules/xtreme-header-8.C
> new file mode 100644
> index 00000000000..b0d0ae87534
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/xtreme-header-8.C
> @@ -0,0 +1,9 @@
> +// { dg-additional-options "-fmodules-ts -fdump-lang-module" }
> +// { dg-module-cmi empty }
> +
> +module;
> +#include "xtreme-header.h"
> +export module empty;
> +
> +// The whole GMF should be discarded here
> +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header.h b/gcc/testsuite/g++.dg/modules/xtreme-header.h
> index 3147aaf00f4..6f98a3acecd 100644
> --- a/gcc/testsuite/g++.dg/modules/xtreme-header.h
> +++ b/gcc/testsuite/g++.dg/modules/xtreme-header.h
> @@ -89,7 +89,6 @@
>   
>   // C++20
>   #if __cplusplus > 201703
> -#if 1
>   #include <version>
>   #include <barrier>
>   #include <bit>
> @@ -98,6 +97,7 @@
>   #if __cpp_coroutines
>   #include <coroutine>
>   #endif
> +#include <format>
>   #include <latch>
>   #include <numbers>
>   #include <ranges>
> @@ -106,24 +106,33 @@
>   #include <span>
>   #include <stop_token>
>   #include <syncstream>
> -#if 0
> -// Unimplemented
> -#include <format>
> -#endif
> -#endif
>   #endif
>   
>   // C++23
>   #if __cplusplus > 202002L
>   #include <expected>
> +#include <generator>
> +#include <print>
>   #include <spanstream>
>   #include <stacktrace>
> +#include <stdfloat>
>   #if 0
>   // Unimplemented
>   #include <flat_map>
>   #include <flat_set>
> -#include <generator>
>   #include <mdspan>
> -#include <print>
> +#endif
> +#endif
> +
> +// C++26
> +#if __cplusplus > 202302L
> +#if 0
> +// Unimplemented
> +#include <debugging>
> +#include <execution>
> +#include <hazard_pointer>
> +#include <linalg>
> +#include <rcu>
> +#include <text_encoding>
>   #endif
>   #endif

On Wed, Jul 17, 2024 at 11:36:26PM -0400, Jason Merrill wrote:
> On 7/17/24 11:04 PM, Nathaniel Shead wrote:
> > On Wed, Jul 17, 2024 at 01:12:34PM -0400, Jason Merrill wrote:
> > > On 5/1/24 11:27 AM, Jason Merrill wrote:
> > > > On 5/1/24 07:11, Patrick Palka wrote:
> > > > > On Wed, 1 May 2024, Nathaniel Shead wrote:
> > > > > 
> > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > > > > 
> > > > > > -- >8 --
> > > > > > 
> > > > > > When calling instantiate_pending_templates at end of parsing, any new
> > > > > > functions that are instantiated from this point have their module
> > > > > > purview set based on the current value of module_kind.
> > > > > > 
> > > > > > This is unideal, however, as the modules code will then treat these
> > > > > > instantiations as reachable and cause large swathes of the GMF to be
> > > > > > emitted into the module CMI, despite no code in the actual module
> > > > > > purview referencing it.
> > > > > > 
> > > > > > This patch fixes this by also remembering the value of module_kind when
> > > > > > the instantiation was deferred, and restoring it when doing this
> > > > > > deferred instantiation.  That way newly instantiated declarations
> > > > > > appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> > > > > > instantiation was required, meaning that GMF entities won't be counted
> > > > > > as reachable unless referenced by an actually reachable entity.
> > > > > > 
> > > > > > Note that purviewness and attachment etc. is generally only determined
> > > > > > by the base template: this is purely for determining whether a
> > > > > > specialisation was declared in the module purview and hence whether it
> > > > > > should be streamed out.  See the comment on 'set_instantiating_module'.
> > > > > > 
> > > > > >      PR c++/114630
> > > > > >      PR c++/114795
> > > > > > 
> > > > > > gcc/cp/ChangeLog:
> > > > > > 
> > > > > >      * cp-tree.h (struct tinst_level): Add field for tracking
> > > > > >      module_kind.
> > > > > >      * pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
> > > > > >      (reopen_tinst_level): Restore module_kind from level.
> > > > > >      (instantiate_pending_templates): Save and restore module_kind so
> > > > > >      it isn't affected by reopen_tinst_level.
> > > > > 
> > > > > LGTM.  Another approach is to instantiate all so-far deferred
> > > > > instantiations and vtables once we reach the start of the module
> > > > > purview, but your approach is much cleaner.
> > > > 
> > > > Hmm, I actually think I like the approach of instantiating at that point
> > > > better, so that instantiations in the GMF can't depend on things in
> > > > module purview.
> > > > 
> > > > And probably do the same again when switching to the private module
> > > > fragment, when that's implemented.
> > > 
> > > Patrick mentioned these patches again today; I still think this is the right
> > > approach, particularly for the private module fragment.
> > > 
> > > That said, could we just clear module_kind at EOF?  As you say, only the
> > > purviewness of the template matters for implicit instantiations.  It is
> > > seeming to me like "set_instantiating_module" should only apply for explicit
> > > instantiations, at the point of the explicit instantiation declaration.
> > > 
> > > Jason
> > > 
> > 
> > Yup, I agree that this is the right approach, since as you say we'll
> > need to do this for private module fragment anyway; I've just left this
> > on the backburner because it didn't look particularly easy to do neatly
> > and have been focussing on crashes instead for now.
> > 
> > That sounds like a good idea, I've given that a shot and it succeeds
> > bootstrap + regtest (so far just modules.exp) on x86_64-pc-linux-gnu.
> > Here's the patch, OK for trunk if full regtest succeeds?
> 
> Is there a test that an explicit instantiation in module purview is not
> discarded?  OK if so.
> 

Ah, there doesn't appear to be; I created a new test but it looks like
all explicit instantiations are deferred, which means that they then
aren't marked as purview.  So it doesn't look like this solution by
itself actually works.

  module;
  template <typename T> void foo() {}
  export module M;
  template void foo<int>();

  // impl.cpp
  module M;
  void bar() { foo<int>(); }  // error, foo has been discarded

As I was writing this I also did a bit more testing and there are other
things that we don't currently detect as reachable even when they should
be; for instance, according to [module.global.frag] p3.1 the following
should probably work, but currently does not:

  module;
  inline int x = 123;
  export module M;
  int y = x;  // x is decl-reachable from y

  // impl.cpp
  module M;
  int z = x;  // currently errors because 'x' has been discarded

So there's probably a fair bit of work to do here.

It also strikes me that the standard requires us to keep decls reachable
when referred to from even non-inline function bodies, whereas our
current implementation quite deliberately tries to prevent this from
occurring.

> > -- >8 --
> > 
> > Subject: [PATCH v2] c++/modules: Clear module_purview_p at EOF [PR114630]
> > 
> > When calling instantiate_pending_templates at end of parsing, any new
> > functions that are instantiated from this point have their module
> > purview set based on the current value of module_kind.
> > 
> > This is unideal, however, as the modules code will then treat these
> > instantiations as reachable and cause large swathes of the GMF to be
> > emitted into the module CMI, despite no code in the actual module
> > purview referencing it.
> > 
> > Note that purviewness and attachment etc. is generally only determined
> > by the base template: this is purely for determining whether a
> > specialisation was declared in the module purview and hence whether it
> > should be streamed out.  See the comment on 'set_instantiating_module'.
> > 
> > As such, this patch fixes the issue by clearing MK_PURVIEW and MK_ATTACH
> > from module_kind at EOF.  This ensures that deferred instantiations
> > don't get marked as purview and retained in the module CMI
> > unnecessarily.  We also add a new test to ensure that all code in the
> > standard library headers are properly discarded to prevent regressions.
> > 
> > For future work we probably want to eagerly perform any deferred
> > instantiations at the end of the global module fragment, which will
> > avoid this issue as well.  We will need to do this before any private
> > module fragments (once we support them) regardless.
> > 
> > 	PR c++/114630
> > 	PR c++/114795
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* decl2.cc (c_parse_final_cleanups): Clear MK_PURVIEW and
> > 	MK_ATTACH flags.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/modules/xtreme-header.h: Include new header files.
> > 	* g++.dg/modules/gmf-3.C: New test.
> > 	* g++.dg/modules/gmf-4.C: New test.
> > 	* g++.dg/modules/xtreme-header-8.C: New test.
> > 
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> >   gcc/cp/decl2.cc                               |  4 +++
> >   gcc/testsuite/g++.dg/modules/gmf-3.C          | 13 +++++++++
> >   gcc/testsuite/g++.dg/modules/gmf-4.C          | 27 +++++++++++++++++++
> >   .../g++.dg/modules/xtreme-header-8.C          |  9 +++++++
> >   gcc/testsuite/g++.dg/modules/xtreme-header.h  | 25 +++++++++++------
> >   5 files changed, 70 insertions(+), 8 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/modules/gmf-3.C
> >   create mode 100644 gcc/testsuite/g++.dg/modules/gmf-4.C
> >   create mode 100644 gcc/testsuite/g++.dg/modules/xtreme-header-8.C
> > 
> > diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
> > index 6d674684931..acabf65e136 100644
> > --- a/gcc/cp/decl2.cc
> > +++ b/gcc/cp/decl2.cc
> > @@ -5168,6 +5168,10 @@ c_parse_final_cleanups (void)
> >     /* FIXME - huh?  was  input_line -= 1;*/
> > +  /* Clear module kind so that deferred instantiations don't get unnecessarily
> > +     marked as purview, preventing GMF discarding.  */
> > +  module_kind &= ~(MK_PURVIEW | MK_ATTACH);
> > +
> >     /* We now have to write out all the stuff we put off writing out.
> >        These include:
> > diff --git a/gcc/testsuite/g++.dg/modules/gmf-3.C b/gcc/testsuite/g++.dg/modules/gmf-3.C
> > new file mode 100644
> > index 00000000000..e52ae904ea9
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/gmf-3.C
> > @@ -0,0 +1,13 @@
> > +// PR c++/114630
> > +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> > +// { dg-module-cmi M }
> > +
> > +module;
> > +template <typename> struct allocator {
> > +  allocator() {}
> > +};
> > +template class allocator<wchar_t>;
> > +export module M;
> > +
> > +// The whole GMF should be discarded here
> > +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> > diff --git a/gcc/testsuite/g++.dg/modules/gmf-4.C b/gcc/testsuite/g++.dg/modules/gmf-4.C
> > new file mode 100644
> > index 00000000000..c95bc782cea
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/gmf-4.C
> > @@ -0,0 +1,27 @@
> > +// PR c++/114630
> > +// { dg-additional-options "-fmodules-ts -Wno-global-module -fdump-lang-module" }
> > +// { dg-module-cmi M }
> > +
> > +// Deferred instantiation of GM virtual functions should not place
> > +// newly discovered declarations in the module purview
> > +
> > +module;
> > +
> > +template <typename T>
> > +void go() {
> > +  extern T fn_decl();
> > +}
> > +
> > +template <typename T>
> > +struct S {
> > +  virtual void f() {
> > +    go<char>();
> > +  }
> > +};
> > +
> > +S<int> s;
> > +
> > +export module M;
> > +
> > +// The whole GMF should be discarded here
> > +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> > diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header-8.C b/gcc/testsuite/g++.dg/modules/xtreme-header-8.C
> > new file mode 100644
> > index 00000000000..b0d0ae87534
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/xtreme-header-8.C
> > @@ -0,0 +1,9 @@
> > +// { dg-additional-options "-fmodules-ts -fdump-lang-module" }
> > +// { dg-module-cmi empty }
> > +
> > +module;
> > +#include "xtreme-header.h"
> > +export module empty;
> > +
> > +// The whole GMF should be discarded here
> > +// { dg-final { scan-lang-dump "Wrote 0 clusters" module } }
> > diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header.h b/gcc/testsuite/g++.dg/modules/xtreme-header.h
> > index 3147aaf00f4..6f98a3acecd 100644
> > --- a/gcc/testsuite/g++.dg/modules/xtreme-header.h
> > +++ b/gcc/testsuite/g++.dg/modules/xtreme-header.h
> > @@ -89,7 +89,6 @@
> >   // C++20
> >   #if __cplusplus > 201703
> > -#if 1
> >   #include <version>
> >   #include <barrier>
> >   #include <bit>
> > @@ -98,6 +97,7 @@
> >   #if __cpp_coroutines
> >   #include <coroutine>
> >   #endif
> > +#include <format>
> >   #include <latch>
> >   #include <numbers>
> >   #include <ranges>
> > @@ -106,24 +106,33 @@
> >   #include <span>
> >   #include <stop_token>
> >   #include <syncstream>
> > -#if 0
> > -// Unimplemented
> > -#include <format>
> > -#endif
> > -#endif
> >   #endif
> >   // C++23
> >   #if __cplusplus > 202002L
> >   #include <expected>
> > +#include <generator>
> > +#include <print>
> >   #include <spanstream>
> >   #include <stacktrace>
> > +#include <stdfloat>
> >   #if 0
> >   // Unimplemented
> >   #include <flat_map>
> >   #include <flat_set>
> > -#include <generator>
> >   #include <mdspan>
> > -#include <print>
> > +#endif
> > +#endif
> > +
> > +// C++26
> > +#if __cplusplus > 202302L
> > +#if 0
> > +// Unimplemented
> > +#include <debugging>
> > +#include <execution>
> > +#include <hazard_pointer>
> > +#include <linalg>
> > +#include <rcu>
> > +#include <text_encoding>
> >   #endif
> >   #endif
>

On 7/18/24 12:33 AM, Nathaniel Shead wrote:
> On Wed, Jul 17, 2024 at 11:36:26PM -0400, Jason Merrill wrote:
>> On 7/17/24 11:04 PM, Nathaniel Shead wrote:
>>> On Wed, Jul 17, 2024 at 01:12:34PM -0400, Jason Merrill wrote:
>>>> On 5/1/24 11:27 AM, Jason Merrill wrote:
>>>>> On 5/1/24 07:11, Patrick Palka wrote:
>>>>>> On Wed, 1 May 2024, Nathaniel Shead wrote:
>>>>>>
>>>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>>>>>>
>>>>>>> -- >8 --
>>>>>>>
>>>>>>> When calling instantiate_pending_templates at end of parsing, any new
>>>>>>> functions that are instantiated from this point have their module
>>>>>>> purview set based on the current value of module_kind.
>>>>>>>
>>>>>>> This is unideal, however, as the modules code will then treat these
>>>>>>> instantiations as reachable and cause large swathes of the GMF to be
>>>>>>> emitted into the module CMI, despite no code in the actual module
>>>>>>> purview referencing it.
>>>>>>>
>>>>>>> This patch fixes this by also remembering the value of module_kind when
>>>>>>> the instantiation was deferred, and restoring it when doing this
>>>>>>> deferred instantiation.  That way newly instantiated declarations
>>>>>>> appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
>>>>>>> instantiation was required, meaning that GMF entities won't be counted
>>>>>>> as reachable unless referenced by an actually reachable entity.
>>>>>>>
>>>>>>> Note that purviewness and attachment etc. is generally only determined
>>>>>>> by the base template: this is purely for determining whether a
>>>>>>> specialisation was declared in the module purview and hence whether it
>>>>>>> should be streamed out.  See the comment on 'set_instantiating_module'.
>>>>>>>
>>>>>>>       PR c++/114630
>>>>>>>       PR c++/114795
>>>>>>>
>>>>>>> gcc/cp/ChangeLog:
>>>>>>>
>>>>>>>       * cp-tree.h (struct tinst_level): Add field for tracking
>>>>>>>       module_kind.
>>>>>>>       * pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
>>>>>>>       (reopen_tinst_level): Restore module_kind from level.
>>>>>>>       (instantiate_pending_templates): Save and restore module_kind so
>>>>>>>       it isn't affected by reopen_tinst_level.
>>>>>>
>>>>>> LGTM.  Another approach is to instantiate all so-far deferred
>>>>>> instantiations and vtables once we reach the start of the module
>>>>>> purview, but your approach is much cleaner.
>>>>>
>>>>> Hmm, I actually think I like the approach of instantiating at that point
>>>>> better, so that instantiations in the GMF can't depend on things in
>>>>> module purview.
>>>>>
>>>>> And probably do the same again when switching to the private module
>>>>> fragment, when that's implemented.
>>>>
>>>> Patrick mentioned these patches again today; I still think this is the right
>>>> approach, particularly for the private module fragment.
>>>>
>>>> That said, could we just clear module_kind at EOF?  As you say, only the
>>>> purviewness of the template matters for implicit instantiations.  It is
>>>> seeming to me like "set_instantiating_module" should only apply for explicit
>>>> instantiations, at the point of the explicit instantiation declaration.
>>>>
>>>> Jason
>>>>
>>>
>>> Yup, I agree that this is the right approach, since as you say we'll
>>> need to do this for private module fragment anyway; I've just left this
>>> on the backburner because it didn't look particularly easy to do neatly
>>> and have been focussing on crashes instead for now.
>>>
>>> That sounds like a good idea, I've given that a shot and it succeeds
>>> bootstrap + regtest (so far just modules.exp) on x86_64-pc-linux-gnu.
>>> Here's the patch, OK for trunk if full regtest succeeds?
>>
>> Is there a test that an explicit instantiation in module purview is not
>> discarded?  OK if so.
> 
> Ah, there doesn't appear to be; I created a new test but it looks like
> all explicit instantiations are deferred, which means that they then
> aren't marked as purview.  So it doesn't look like this solution by
> itself actually works.
> 
>    module;
>    template <typename T> void foo() {}
>    export module M;
>    template void foo<int>();
> 
>    // impl.cpp
>    module M;
>    void bar() { foo<int>(); }  // error, foo has been discarded
> 
> As I was writing this I also did a bit more testing and there are other
> things that we don't currently detect as reachable even when they should
> be; for instance, according to [module.global.frag] p3.1 the following
> should probably work, but currently does not:
> 
>    module;
>    inline int x = 123;
>    export module M;
>    int y = x;  // x is decl-reachable from y
> 
>    // impl.cpp
>    module M;
>    int z = x;  // currently errors because 'x' has been discarded

I would expect that error whether or not x is discarded; x is not 
exported or attached to M, so it's not found by name lookup under
https://eel.is/c++draft/basic#lookup.general-2.3

> So there's probably a fair bit of work to do here.
> 
> It also strikes me that the standard requires us to keep decls reachable
> when referred to from even non-inline function bodies, whereas our
> current implementation quite deliberately tries to prevent this from
> occurring.

Hmm, I agree.  That surprises me, so I just sent this to CWG:

--
The rationale for P1779 included
"The inline keyword therefore allows a module author to choose, for each 
exposed function, whether to expose its implementation to importers, 
gaining the possibility of better performance at the expense of tighter 
coupling (and thus stronger restrictions on evolution of the library 
code and/or more frequent recompiles for the client)."

But AFAICT that's not actually achieved by the wording, which primarily 
allows a module author to choose to avoid errors about exposing TU-local 
entities.

I don't see anything that precludes an implementation from making 
non-inline function bodies reachable to importers; compilers will often 
inline functions that are marked inline.  I suppose P1779 does avoid 
requiring that they be reachable under [basic.def.odr]/13, but does not 
prevent it.

Typically, it seems desirable for compilers to make reachable all 
non-inline function bodies that aren't exposures, for inlining/IPA of 
small functions without needing to resort to LTO.

For a function template, we generally need the implementation to be 
reachable regardless of whether it's inline, so that it can be 
instantiated by importers.

[module.global.frag] seems to say that anything in the global module 
used in the implementation of a function is decl-reachable, and 
therefore not discarded, regardless of whether the function is inline.

So, the actual effect of P1779 on reachability (and thus, what ends up 
in the BMI) seems to be pretty minimal.  Am I missing anything?

Jason

On Thu, Jul 18, 2024 at 02:33:56PM -0400, Jason Merrill wrote:
> On 7/18/24 12:33 AM, Nathaniel Shead wrote:
> > On Wed, Jul 17, 2024 at 11:36:26PM -0400, Jason Merrill wrote:
> > > On 7/17/24 11:04 PM, Nathaniel Shead wrote:
> > > > On Wed, Jul 17, 2024 at 01:12:34PM -0400, Jason Merrill wrote:
> > > > > On 5/1/24 11:27 AM, Jason Merrill wrote:
> > > > > > On 5/1/24 07:11, Patrick Palka wrote:
> > > > > > > On Wed, 1 May 2024, Nathaniel Shead wrote:
> > > > > > > 
> > > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > > > > > > 
> > > > > > > > -- >8 --
> > > > > > > > 
> > > > > > > > When calling instantiate_pending_templates at end of parsing, any new
> > > > > > > > functions that are instantiated from this point have their module
> > > > > > > > purview set based on the current value of module_kind.
> > > > > > > > 
> > > > > > > > This is unideal, however, as the modules code will then treat these
> > > > > > > > instantiations as reachable and cause large swathes of the GMF to be
> > > > > > > > emitted into the module CMI, despite no code in the actual module
> > > > > > > > purview referencing it.
> > > > > > > > 
> > > > > > > > This patch fixes this by also remembering the value of module_kind when
> > > > > > > > the instantiation was deferred, and restoring it when doing this
> > > > > > > > deferred instantiation.  That way newly instantiated declarations
> > > > > > > > appropriately get a DECL_MODULE_PURVIEW_P appropriate for where the
> > > > > > > > instantiation was required, meaning that GMF entities won't be counted
> > > > > > > > as reachable unless referenced by an actually reachable entity.
> > > > > > > > 
> > > > > > > > Note that purviewness and attachment etc. is generally only determined
> > > > > > > > by the base template: this is purely for determining whether a
> > > > > > > > specialisation was declared in the module purview and hence whether it
> > > > > > > > should be streamed out.  See the comment on 'set_instantiating_module'.
> > > > > > > > 
> > > > > > > >       PR c++/114630
> > > > > > > >       PR c++/114795
> > > > > > > > 
> > > > > > > > gcc/cp/ChangeLog:
> > > > > > > > 
> > > > > > > >       * cp-tree.h (struct tinst_level): Add field for tracking
> > > > > > > >       module_kind.
> > > > > > > >       * pt.cc (push_tinst_level_loc): Cache module_kind in new_level.
> > > > > > > >       (reopen_tinst_level): Restore module_kind from level.
> > > > > > > >       (instantiate_pending_templates): Save and restore module_kind so
> > > > > > > >       it isn't affected by reopen_tinst_level.
> > > > > > > 
> > > > > > > LGTM.  Another approach is to instantiate all so-far deferred
> > > > > > > instantiations and vtables once we reach the start of the module
> > > > > > > purview, but your approach is much cleaner.
> > > > > > 
> > > > > > Hmm, I actually think I like the approach of instantiating at that point
> > > > > > better, so that instantiations in the GMF can't depend on things in
> > > > > > module purview.
> > > > > > 
> > > > > > And probably do the same again when switching to the private module
> > > > > > fragment, when that's implemented.
> > > > > 
> > > > > Patrick mentioned these patches again today; I still think this is the right
> > > > > approach, particularly for the private module fragment.
> > > > > 
> > > > > That said, could we just clear module_kind at EOF?  As you say, only the
> > > > > purviewness of the template matters for implicit instantiations.  It is
> > > > > seeming to me like "set_instantiating_module" should only apply for explicit
> > > > > instantiations, at the point of the explicit instantiation declaration.
> > > > > 
> > > > > Jason
> > > > > 
> > > > 
> > > > Yup, I agree that this is the right approach, since as you say we'll
> > > > need to do this for private module fragment anyway; I've just left this
> > > > on the backburner because it didn't look particularly easy to do neatly
> > > > and have been focussing on crashes instead for now.
> > > > 
> > > > That sounds like a good idea, I've given that a shot and it succeeds
> > > > bootstrap + regtest (so far just modules.exp) on x86_64-pc-linux-gnu.
> > > > Here's the patch, OK for trunk if full regtest succeeds?
> > > 
> > > Is there a test that an explicit instantiation in module purview is not
> > > discarded?  OK if so.
> > 
> > Ah, there doesn't appear to be; I created a new test but it looks like
> > all explicit instantiations are deferred, which means that they then
> > aren't marked as purview.  So it doesn't look like this solution by
> > itself actually works.
> > 
> >    module;
> >    template <typename T> void foo() {}
> >    export module M;
> >    template void foo<int>();
> > 
> >    // impl.cpp
> >    module M;
> >    void bar() { foo<int>(); }  // error, foo has been discarded
> > 
> > As I was writing this I also did a bit more testing and there are other
> > things that we don't currently detect as reachable even when they should
> > be; for instance, according to [module.global.frag] p3.1 the following
> > should probably work, but currently does not:
> > 
> >    module;
> >    inline int x = 123;
> >    export module M;
> >    int y = x;  // x is decl-reachable from y
> > 
> >    // impl.cpp
> >    module M;
> >    int z = x;  // currently errors because 'x' has been discarded
> 
> I would expect that error whether or not x is discarded; x is not exported
> or attached to M, so it's not found by name lookup under
> https://eel.is/c++draft/basic#lookup.general-2.3
> 

Ah right, of course; here's a different testcase then:

  module;
  inline int x = 123;  // #1
  export module M;
  int y = x;

  // impl.cpp
  module M;
  extern "C++" double x;  // #2

Since by http://eel.is/c++draft/basic.link#11 this requires a
diagnostic.  The declarations have the same name, correspond, and both
declare names with external linkage, and so declare the same entity, but
with different types.  And as mentioned earlier, #1 should not be
discarded (it's decl-reachable from y) and so is reachable from #2.

That said this is probably a fairly low-priority issue to fix.

> > So there's probably a fair bit of work to do here.
> > 
> > It also strikes me that the standard requires us to keep decls reachable
> > when referred to from even non-inline function bodies, whereas our
> > current implementation quite deliberately tries to prevent this from
> > occurring.
> 
> Hmm, I agree.  That surprises me, so I just sent this to CWG:
> 
> --
> The rationale for P1779 included
> "The inline keyword therefore allows a module author to choose, for each
> exposed function, whether to expose its implementation to importers, gaining
> the possibility of better performance at the expense of tighter coupling
> (and thus stronger restrictions on evolution of the library code and/or more
> frequent recompiles for the client)."
> 
> But AFAICT that's not actually achieved by the wording, which primarily
> allows a module author to choose to avoid errors about exposing TU-local
> entities.
> 
> I don't see anything that precludes an implementation from making non-inline
> function bodies reachable to importers; compilers will often inline
> functions that are marked inline.  I suppose P1779 does avoid requiring that
> they be reachable under [basic.def.odr]/13, but does not prevent it.
> 
> Typically, it seems desirable for compilers to make reachable all non-inline
> function bodies that aren't exposures, for inlining/IPA of small functions
> without needing to resort to LTO.
> 
> For a function template, we generally need the implementation to be
> reachable regardless of whether it's inline, so that it can be instantiated
> by importers.
> 
> [module.global.frag] seems to say that anything in the global module used in
> the implementation of a function is decl-reachable, and therefore not
> discarded, regardless of whether the function is inline.
> 
> So, the actual effect of P1779 on reachability (and thus, what ends up in
> the BMI) seems to be pretty minimal.  Am I missing anything?
> 
> Jason
>