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 <nathanielosh...@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
>
