On Mon, Feb 27, 2023 at 06:21:13PM -0500, Jason Merrill wrote:
> On 2/23/23 10:54, Marek Polacek wrote:
> > On Thu, Feb 23, 2023 at 10:17:22AM -0500, Patrick Palka wrote:
> > > On Wed, 22 Feb 2023, Marek Polacek wrote:
> > >
> > > > In this test, we get a bogus error because we failed to deduce the auto
> > > > in
> > > > constexpr auto is_pointer_v = is_pointer<Tp>::value;
> > > > to bool. Then ensure_literal_type_for_constexpr_object thinks the
> > > > object
> > > > isn't literal and an error is reported.
> > > >
> > > > This is another case of the interaction between tf_partial and 'auto',
> > > > where the auto was not reduced so the deduction failed. In more detail:
> > > > we have
> > > >
> > > > Wrap1<int>()
> > > >
> > > > in the code and we need to perform OR -> fn_type_unification. The targ
> > > > list is incomplete, so we do
> > > > tsubst_flags_t ecomplain = complain | tf_partial |
> > > > tf_fndecl_type;
> > > > fntype = tsubst (TREE_TYPE (fn), explicit_targs, ecomplain,
> > > > NULL_TREE);
> > > > where TREE_TYPE (fn) is struct integral_constant <T402> (void). Then
> > > > we substitute the return type, which results in tsubsting
> > > > is_pointer_v<int>.
> > > > is_pointer_v is a variable template with a placeholder type:
> > > >
> > > > template <class Tp>
> > > > constexpr auto is_pointer_v = is_pointer<Tp>::value;
> > > >
> > > > so we find ourselves in lookup_and_finish_template_variable.
> > > > tf_partial is
> > > > still set, so finish_template_variable -> instantiate_template -> tsubst
> > > > won't reduce the level of auto. But then we do mark_used which
> > > > eventually
> > > > calls do_auto_deduction which clears tf_partial, because we want to
> > > > replace
> > > > the auto now. But we hadn't reduced auto's level so this fails. And
> > > > since we're not in an immediate context, we emit a hard error.
> > > >
> > > > I suppose that when we reach lookup_and_finish_template_variable it's
> > > > probably time to clear tf_partial. (I added an assert and our testsuite
> > > > doesn't have a test whereby we get to
> > > > lookup_and_finish_template_variable
> > > > while tf_partial is still active.)
> > > >
> > > > Does this make *any* sense to you?
> > > >
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > >
> > > > PR c++/108550
> > > >
> > > > gcc/cp/ChangeLog:
> > > >
> > > > * pt.cc (lookup_and_finish_template_variable): Clear tf_partial.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > * g++.dg/cpp1y/var-templ70.C: New test.
> > > > * g++.dg/cpp1y/var-templ71.C: New test.
> > > > ---
> > > > gcc/cp/pt.cc | 6 ++++++
> > > > gcc/testsuite/g++.dg/cpp1y/var-templ70.C | 25 +++++++++++++++++++++++
> > > > gcc/testsuite/g++.dg/cpp1y/var-templ71.C | 26 ++++++++++++++++++++++++
> > > > 3 files changed, 57 insertions(+)
> > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/var-templ70.C
> > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/var-templ71.C
> > > >
> > > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > > > index 1a071e95004..f636bac5413 100644
> > > > --- a/gcc/cp/pt.cc
> > > > +++ b/gcc/cp/pt.cc
> > > > @@ -10355,6 +10355,12 @@ lookup_and_finish_template_variable (tree
> > > > templ, tree targs,
> > > > if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (templ)) == 1
> > > > && !any_dependent_template_arguments_p (targs))
> > > > {
> > > > + /* We may be called while doing a partial substitution, but the
> > > > + type of the variable template may be auto, in which case we
> > > > + will call do_auto_deduction in mark_used (which clears
> > > > tf_partial)
> > > > + and the auto must be properly reduced at that time for the
> > > > + deduction to work. */
> > > > + complain &= ~tf_partial;
> > >
> > > LGTM. I can't think of a reason to keep tf_partial set at this point
> > > since we know there's only a single level of template arguments left to
> > > substitute and the args we have are non-dependent, so the substitution
> > > is in no way partial.
> >
> > Right, once we get to finish_template_variable I suppose we're really
> > committed to the var tmpl. So the fix should be safe, albeit it feels
> > a bit ad hoc. FWIW, I'd tested adding alias templates to the mix and
> > it still worked.
> > > Though I wonder if ideally we should clear tf_partial more generally
> > > from instantiate_template? Modulo diagnostics, the result of
> > > instantiate_template shouldn't depend on complain, in particular because
> > > we cache the result in the specializations table is which keyed off of
> > > only the given tmpl and args. Not sure if that'd be a suitable change
> > > for stage4/backports though...
> >
> > This makes sense; I guess anytime you have the full set of targs and are
> > going to instantiate, tf_partial should be cleared otherwise it can
> > wreak havoc. I'd be nervous about applying such a patch now but maybe
> > we could experiment with that in GCC 14.
>
> This sounds right to me; it would never make sense to have tf_partial set in
> instantiate_template, so we should be able to clear it there. We might even
> do
>
> complain = complain & tf_warning_or_error
>
> since none of the other flags are relevant at that level either.
I filed an internal-improvement PR and assigned to self for GCC 14:
<https://gcc.gnu.org/PR108960>.
> But this patch is OK for GCC 13.
Thanks,
Marek
