Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...)

[Jason Merrill <jason@redhat.com>]

On 12/14/18 5:33 PM, Alexandre Oliva wrote:
> On Dec 14, 2018, Jason Merrill <jason@redhat.com> wrote:
> 
>>> If inh is false, we're a copy constructor, which always has a parm,
>>> so this hunk seems unnecessary.
> 
> ack
> 
>>>> -      int cvquals = cp_type_quals (TREE_TYPE (parm));
>>>> +      int cvquals = parm ? cp_type_quals (TREE_TYPE (parm)) : 0;
>>>
>>> This could also check !inh.
> 
> *nod*
> 
>> And in the existing code, while I'm looking at it:
> 
>> The "if (inh) continue" is odd, there's no reason to iterate through
>> the fields ignoring all of them when we could skip the loop entirely.
> 
> Heh, funny, an earlier version of the patch that added an if (inh) to
> print an error on zero-args had an 'else fields = NULL;'.  That
> improvement went away along with my course change.  But look!, it's back
> in the version below ;-)
> 
> Testing...  Ok to install if it passes?
> 
> 
> [PR c++/88146] do not crash synthesizing inherited ctor(...)
> 
> This patch started out from the testcase in PR88146, that attempted to
> synthesize an inherited ctor without any args before a varargs
> ellipsis and crashed while at that, because of the unguarded
> dereferencing of the parm type list, that usually contains a
> terminator.  The terminator is not there for varargs functions,
> however, and without any other args, we ended up dereferencing a NULL
> pointer.  Oops.
> 
> Guarding accesses to parm would be easy, but not necessary.  In
> do_build_copy_constructor, non-inherited ctors are copy-ctors, that
> always have at least one parm, so parm needs not be guarded when we
> know the access will only take place when we're dealing with an
> inherited ctor.  The only other problematic use was in the cvquals
> initializer, a variable only used in a loop over fields, that we
> skipped individually in inherited ctors.  I've arranged to skip the
> entire loop over fields for inherited ctors, and to only initialize
> cvquals otherwise.
> 
> Avoiding the crash from unguarded accesses was easy, but I thought we
> should still produce the sorry message we got in other testcases that
> passed arguments through the ellipsis in inherited ctors.  I put a
> check in, and noticed the inherited ctors were synthesized with the
> location assigned to the class name, although they were initially
> assigned the location of the using declaration.  I decided the latter
> was better, and arranged for the better location to be retained.
> 
> Further investigation revealed the lack of a sorry message had to do
> with the call being in a non-evaluated context, in this case, a
> noexcept expression.  The sorry would be correctly reported in other
> contexts, so I rolled back the check I'd added, but retained the
> source location improvement.
> 
> I was still concerned about issuing sorry messages while instantiating
> template ctors even in non-evaluated contexts, e.g., if a template
> ctor had a base initializer that used an inherited ctor with enough
> arguments that they'd go through an ellipsis.  I wanted to defer the
> instantiation of such template ctors, but that would have been wrong
> for constexpr template ctors, and already done for non-constexpr ones.
> So, I just consolidated multiple test variants into a single testcase
> that explores and explains various of the possibilities I thought of.
> 
> 
> for  gcc/cp/ChangeLog
> 
> 	PR c++/88146
> 	* method.c (do_build_copy_constructor): Skip iteration over
> 	fields for inherited ctors, and initialize cvquals otherwise.
> 	(synthesize_method): Retain location of inherited ctor.
> 
> for  gcc/testsuite/ChangeLog
> 
> 	PR c++/88146
> 	* g++.dg/cpp0x/inh-ctor32.C: New.
> ---
>   gcc/cp/method.c                         |   14 +-
>   gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C |  229 +++++++++++++++++++++++++++++++
>   2 files changed, 238 insertions(+), 5 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C
> 
> diff --git a/gcc/cp/method.c b/gcc/cp/method.c
> index fd023e200538..4cbdadbe3d26 100644
> --- a/gcc/cp/method.c
> +++ b/gcc/cp/method.c
> @@ -677,7 +677,7 @@ do_build_copy_constructor (tree fndecl)
>       {
>         tree fields = TYPE_FIELDS (current_class_type);
>         tree member_init_list = NULL_TREE;
> -      int cvquals = cp_type_quals (TREE_TYPE (parm));
> +      int cvquals;
>         int i;
>         tree binfo, base_binfo;
>         tree init;
> @@ -704,6 +704,11 @@ do_build_copy_constructor (tree fndecl)
>   						inh, member_init_list);
>   	}
>   
> +      if (!inh)
> +	cvquals = cp_type_quals (TREE_TYPE (parm));
> +      else
> +	fields = NULL;

Let's move the initialization of "fields" inside the 'then' block here 
with the initialization of "cvquals", rather than clear it in the 
'else'.  OK with that change.

Jason