On Wed, Feb 26, 2025 at 10:29:59AM -0500, Jason Merrill wrote:
> On 2/21/25 6:05 AM, Nathaniel Shead wrote:
> > After seeing PR c++/118964 I'm coming back around to this [1] patch
> > series, since it appears that this can cause errors on otherwise valid
> > code by instantiations coming into module purview that reference
> > TU-local entities.
> >
> > [1]: https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650324.html
> >
> > We'd previously discussed that the best way to solve this in general
> > would be to perform all deferred instantiations at the end of the GMF to
> > ensure that they would not leak into the module purview. I still
> > tentatively agree that this would be the "nicer" way to go (though I've
> > since come across https://eel.is/c++draft/temp.point#7 and wonder if
> > this may not be strictly conforming according to the current wording?).
>
> Hmm, interesting point.
>
> > I've not yet managed to implement this however, and even if I had, at
> > this stage it would definitely not be appropriate for GCC15; would the
> > approach described below be appropriate for GCC15 as a stop-gap to
> > reduce these issues?
>
> As I suggested earlier in this discussion, it seems to me that the
> purviewness of an implicit instantiation shouldn't matter, they should all
> be treated as discardable.
>
So looking further I believe that currently they are not, in that
implicit instantiations themselves are only ever emitted if they are
reached from elsewhere (regardless of their purviewness), so the issues
are mostly just with deferred explicit instantiations etc. However...
> Could we instead set DECL_MODULE_PURVIEW_P as appropriate when we see an
> explicit instantiation and use that + DECL_EXPLICIT_INSTANTIATION to
> recompute module_kind?
>
> Or go with this patch but only look at the saved module_kind if
> DECL_EXPLICIT_INSTANTIATION, but that seems a bit of a waste of space. Might
> be safer at this point, though.
>
Part of the issue is handling things that are "implicit instantiations"
that we don't track were as such; the obvious case here is friend decls,
which get instantiated with the current purviewness but might only have
been instantiated due to a deferred implicit instantiation, and for
which we remove any evidence that they were created due to an implicit
instantiation.
So maybe looking at the problem from that side would also work, but I'm
not sure exactly how best to do that just yet.
> > Another approach would be to maybe lower the error to a permerror, so
> > that users that 'know better' could compile such modules anyway (at risk
> > of various link errors and ODR issues), though I would also need to
> > adjust the streaming logic to handle this better. Thoughts?
>
> This also sounds desirable. With a warning flag so it can be disabled for
> certain deliberate cases like the gthr stuff.
>
> Jason
>
OK, I'll try and took a look if I get a chance (unfortunately life has
interrupted me again for the time being...). I'm not sure that this
would work for the gthr case though (since the error occurs at the point
of use not at the point of declaration); for that we would probably need
some kind of attribute I guess?
> > On Wed, May 01, 2024 at 08:00:22PM +1000, 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.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > * g++.dg/modules/gmf-3.C: New test.
> > >
> > > Signed-off-by: Nathaniel Shead <nathanielosh...@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
> > >
> >
>
