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
>
