On Fri, Sep 27, 2024 at 11:56:27AM -0400, Jason Merrill wrote:
> On 9/23/24 7:46 PM, Nathaniel Shead wrote:
> > Currently I just stream DECL_NAME in TU_LOCAL_ENTITYs for use in
> > diagnostics,
> > but this feels perhaps insufficient. Are there any better approached here?
> > Otherwise I don't think it matters too much, as which entity it is will also
> > be hopefully clear from the 'declared here' notes.
> >
> > I've put the new warning in Wextra, but maybe it would be better to just
> > leave it out of any of the normal warning groups since there's currently
> > no good way to work around the warnings it produces?
> >
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> >
> > -- >8 --
> >
> > [basic.link] p14 lists a number of circumstances where a declaration
> > naming a TU-local entity is not an exposure, notably the bodies of
> > non-inline templates and friend declarations in classes. This patch
> > ensures that these references do not error when exporting the module.
> >
> > We do need to still error on instantiation from a different module,
> > however, in case this refers to a TU-local entity. As such this patch
> > adds a new tree TU_LOCAL_ENTITY which is used purely as a placeholder to
> > poison any attempted template instantiations that refer to it.
> >
> > This is also streamed for friend decls so that merging (based on the
> > index of an entity into the friend decl list) doesn't break and to
> > prevent complicating the logic; I imagine this shouldn't ever come up
> > though.
> >
> > We also add a new warning, '-Wignored-exposures', to handle the case
> > where someone accidentally refers to a TU-local value from within a
> > non-inline function template. This will compile without errors as-is,
> > but any attempt to instantiate the decl will fail; this warning can be
> > used to ensure that this doesn't happen. Unfortunately the warning has
> > quite some false positives; for instance, a user could deliberately only
> > call explicit instantiations of the decl, or use 'if constexpr' to avoid
> > instantiating the TU-local entity from other TUs, neither of which are
> > currently detected.
>
> I disagree with the term "ignored exposure", in the warning name and in the
> rest of the patch; these references are not exposures. It's the naming of a
> TU-local entity that is ignored in basic.link/14.
>
> I like the warning, just would change the name. "-Wtemplate-names-tu-local"?
> "-Wtu-local-in-template"?
>
I wasn't too happy with "ignored exposures" either, I much prefer these
names. I'll use these in the next version of this patch.
> I'm not too concerned about false positives, as long as it can be
> effectively suppressed with #pragma GCC diagnostic ignored. If you only use
> explicit instantiations you don't need to have the template body in the
> interface anyway.
>
The main case I'm thinking of that would be annoying would be
static void tu_local_fn() { /* ... */ }
export template <typename T>
void foo() {
if constexpr (std::is_same_v<T, int>)
tu_local_fn();
// ...
}
template foo<int>();
But this should be able to silenced with #pragma so I suppose that's OK
for Wextra (and is probably fairly unusual code to begin with). And
even more so if the following is fixed...
> > + /* Ideally we would only warn in cases where there are no explicit
> > + instantiations of the template, but we don't currently track this
> > + in an easy-to-find way. */
>
> You can't walk DECL_TEMPLATE_INSTANTIATIONS checking
> DECL_EXPLICIT_INSTANTIATION?
>
I tried, but it looks like DECL_TEMPLATE_INSTANTIATIONS doesn't include
explicit instantiations for function templates definitions once we get
to module processing: it's only recorded for namespace-scope primary
function templates without definitions.
I think I would be able to update 'register_specialization' to more
eagerly include instantiations for every function template in this list,
but I wasn't sure if we wanted to do that just for this if it's not
already necessary, since that would potentially grow it quite a lot.
It strikes me as I write this though that the fact we currently
correctly mark GMF explicit instantiations as reachable is not because
looking through DECL_TEMPLATE_INSTANTIATIONS works, but because of the
issue we've discussed earlier where GMF instantiations are deferred
until the end of the TU and then get marked as purview... so I suppose
we do need to fix this anyway in anticipation of solving that issue.
See e.g.
module;
template <typename T> void foo();
template void foo<int>();
export module M;
with -fdump-lang-module-graph which shows that we emit the instantiation
despite never referencing it from purview.
> What happens with a non-templated, non-inline function body that names a
> TU-local entity? I don't see any specific handling or testcase. Should we
> just omit its definition, like you do in the previous patch for TU-local
> variable initializers?
>
Non-template, non-inline function bodies already have their definitions
omitted by has_definition, so this patch didn't need to do anything
special. There's a testcase for this in internal-5, see e.g.
// The function body for a non-inline function or function template
export void function() {
internal_t i {};
internal_tmpl_t<int> ii {};
internal_ovl(internal_x);
internal_tmpl<int>();
}
export template <typename T> void function_tmpl() { // { dg-warning "refers
to TU-local entity" }
internal_t i {};
internal_tmpl_t<T> ii {};
internal_ovl(internal_x);
internal_tmpl<T>();
}
template void function_tmpl<ok_inst_tag>();
and matching calls in the importer. I should probably add a test for
explicit specialisations too, however; I'll update that in the next
version.
Nathaniel
