Re: C++ PATCH for c++/88294 - ICE with non-constant noexcept-specifier

[Jason Merrill <jason@redhat.com>]

On Fri, Feb 22, 2019 at 1:17 PM Marek Polacek <polacek@redhat.com> wrote:
> On Thu, Feb 21, 2019 at 02:57:28PM -1000, Jason Merrill wrote:
> > On 2/21/19 1:34 PM, Marek Polacek wrote:
> > > On Thu, Feb 21, 2019 at 11:22:41AM -1000, Jason Merrill wrote:
> > > > On 2/21/19 10:56 AM, Marek Polacek wrote:
> > > > > On Wed, Feb 20, 2019 at 01:53:18PM -1000, Jason Merrill wrote:
> > > > > > On 2/20/19 10:31 AM, Marek Polacek wrote:
> > > > > > > Here we ICE when substituting a deferred noexcept-specifier, because it
> > > > > > > contains 'this', a PARM_DECL, in an evaluated context.  This is different
> > > > > > > from "noexcept(noexcept(this))" where the noexcept operator's operand is
> > > > > > > an unevaluated operand.  We crash within tsubst_copy's PARM_DECL handling
> > > > > > > of a 'this' PARM_DECL:
> > > > > > > 15488           gcc_assert (cp_unevaluated_operand != 0)
> > > > > > > It'd be wrong to mess with cp_unevaluated_operand (or current_class_*), and
> > > > > > > since we only check the expression's constness after substituting in
> > > > > > > maybe_instantiate_noexcept, one fix would be the following.
> > > > > > >
> > > > > > > This is not just about 'this', as the 87844 test shows: here we also have
> > > > > > > a parameter whose value we're trying to determine -- it's a template arg
> > > > > > > of a late-specified return type.  Returning the original value in tsubst_copy
> > > > > > > and leaving the later code to complain seems to work here as well.  Just
> > > > > > > removing the assert should work as well.
> > > > > >
> > > > > > I'm reluctant to mess with this assert, as it catches a lot of lambda bugs.
> > > > >
> > > > > Makes sense -- I wasn't aware of that.
> > > > >
> > > > > > I think we want to register_parameter_specializations when instantiating
> > > > > > deferred noexcept, so that tsubst_copy can find the right decls.
> > > > >
> > > > > Thanks for the suggestion -- that works with one catch: we need to replace the
> > > > > fake 'this' in the noexcept- specifier with a real 'this' (the template one),
> > > > > one that has DECL_CONTEXT set.  The fake one comes from inject_this_parameter,
> > > > > when we were parsing the noexcept-specifier.  The real one was set in grokfndecl.
> > > >
> > > > If you set current_class_ptr appropriately tsubst_copy will use it, so you
> > > > shouldn't need to do a walk_tree.
> > >
> > > Unfortunately that broke a lot of libstdc++ tests.  I can poke at it more if
> > > you feel stronly about it.
> >
> > Please do poke a bit more, that surprises me.
>
> Apparently *someone* needs to learn how to properly restore c_c_{ptr,ref}...
>
> noexcept35.C is a new test reduced from something from libstdc++.  Since it
> exercises codepaths that nothing in dg.exp does, I think it's worth adding.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2019-02-22  Marek Polacek  <polacek@redhat.com>
>
>         PR c++/88294 - ICE with non-constant noexcept-specifier.
>         * pt.c (maybe_instantiate_noexcept): Set up the list of local
>         specializations.  Set current_class_{ptr,ref}.

OK, thanks.

Jason