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

[Jason Merrill <jason@redhat.com>]

On 12/14/18 3:42 PM, Jason Merrill wrote:
> On 12/6/18 7:23 PM, Alexandre Oliva wrote:
>> 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 the accesses there was easy, but I missed 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.
>>
>> Regstrapped on x86_64- and i686-linux-gnu, mistakenly along with a patch
>> with a known regression, and got only that known regression.  Retesting
>> without it.  Ok to install?
>>
>>
>> for  gcc/cp/ChangeLog
>>
>>     PR c++/88146
>>     * method.c (do_build_copy_constructor): Do not crash with
>>     ellipsis-only parm list.
>>     (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                         |    9 +
>>   gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C |  229 
>> +++++++++++++++++++++++++++++++
>>   2 files changed, 234 insertions(+), 4 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..41d609fb1de6 100644
>> --- a/gcc/cp/method.c
>> +++ b/gcc/cp/method.c
>> @@ -643,7 +643,7 @@ do_build_copy_constructor (tree fndecl)
>>     bool trivial = trivial_fn_p (fndecl);
>>     tree inh = DECL_INHERITED_CTOR (fndecl);
>> -  if (!inh)
>> +  if (parm && !inh)
>>       parm = convert_from_reference (parm);
> 
> If inh is false, we're a copy constructor, which always has a parm, so 
> this hunk seems unnecessary.
> 
>>     if (trivial)
>> @@ -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 = parm ? cp_type_quals (TREE_TYPE (parm)) : 0;
> 
> This could also check !inh.

And in the existing code, while I'm looking at it:

>       for (; fields; fields = DECL_CHAIN (fields))
>         {
>           tree field = fields;
>           tree expr_type;
> 
>           if (TREE_CODE (field) != FIELD_DECL)
>             continue;
>           if (inh)
>             continue;

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.

Jason