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
