[v4] c++: Reject cdtors and conversion operators with a single * as return type [PR118306]

  Hi Jason,

On 7 Feb 2025, at 23:10, Jason Merrill wrote:

> On 2/7/25 4:04 PM, Simon Martin wrote:
>> Hi Jason,
>>
>> On 7 Feb 2025, at 14:21, Jason Merrill wrote:
>>
>>> On 2/6/25 3:05 PM, Simon Martin wrote:
>>>> Hi Jason,
>>>>
>>>> On 6 Feb 2025, at 16:48, Jason Merrill wrote:
>>>>
>>>>> On 2/5/25 2:21 PM, Simon Martin wrote:
>>>>>> Hi Jason,
>>>>>>
>>>>>> On 4 Feb 2025, at 21:23, Jason Merrill wrote:
>>>>>>
>>>>>>> On 2/4/25 3:03 PM, Jason Merrill wrote:
>>>>>>>> On 2/4/25 11:45 AM, Simon Martin wrote:
>>>>>>>>> On 4 Feb 2025, at 17:17, Jason Merrill wrote:
>>>>>>>>>
>>>>>>>>>> On 2/4/25 10:56 AM, Simon Martin wrote:
>>>>>>>>>>> Hi Jason,
>>>>>>>>>>>
>>>>>>>>>>> On 4 Feb 2025, at 16:39, Jason Merrill wrote:
>>>>>>>>>>>
>>>>>>>>>>>> On 1/15/25 9:56 AM, Jason Merrill wrote:
>>>>>>>>>>>>> On 1/15/25 7:24 AM, Simon Martin wrote:
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 14 Jan 2025, at 23:31, Jason Merrill wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 1/14/25 2:13 PM, Simon Martin wrote:
>>>>>>>>>>>>>>>> On 10 Jan 2025, at 19:10, Andrew Pinski wrote:
>>>>>>>>>>>>>>>>> On Fri, Jan 10, 2025 at 3:18 AM Simon Martin
>>>>>>>>>>>>>>>>> <simon@nasilyan.com>
>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> We currently accept the following invalid code (EDG 
>>>>>>>>>>>>>>>>>> and
>>
>>>>>>>>>>>>>>>>>> MSVC
>>>>>>>>>>>>>>>>>> do
>>>>>>>>>>>>>>>>>> as
>>>>>>>>>>>>>>>>>> well)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> clang does too:
>>>>>>>>>>>>>>>>> https://github.com/llvm/llvm-project/issues/121706 .
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Note it might be useful if a testcase with multiply 

>>>>>>>>>>>>>>>>> `*`
>>>>>>>>>>>>>>>>> is
>>>>>>>>>>>>>>>>> included
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> too:
>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>> struct A {
>>>>>>>>>>>>>>>>>          ****A ();
>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>> Thanks, makes sense to add those. Done in the attached
>>>>>>>>>>>>>>>> updated
>>>>>>>>>>>>>>>> revision,
>>>>>>>>>>>>>>>> successfully tested on x86_64-pc-linux-gnu.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> +/* Check that it's OK to declare a function at ID_LOC
>>
>>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>> indicated TYPE,
>>>>>>>>>>>>>>>> +   TYPE_QUALS and DECLARATOR.  SFK indicates the 
>>>>>>>>>>>>>>>> kind
>>>>>>>>>>>>>>>> of
>>>>>>>>>>>>>>>> special
>>>>>>>>>>>>>>>> function (if
>>>>>>>>>>>>>>>> +   any) that this function is.  OPTYPE is the type
>>>>>>>>>>>>>>>> given
>>>>>>>>>>>>>>>> in
>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>> conversion
>>>>>>>>>>>>>>>>           operator declaration, or the class type 
>>>>>>>>>>>>>>>> for
>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>> constructor/destructor.
>>>>>>>>>>>>>>>>           Returns the actual return type of the
>>>>>>>>>>>>>>>> function;
>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>> may
>>>>>>>>>>>>>>>> be
>>>>>>>>>>>>>>>> different
>>>>>>>>>>>>>>>>           than TYPE if an error occurs, or for
>>>>>>>>>>>>>>>> certain
>>>>>>>>>>>>>>>> special
>>>>>>>>>>>>>>>> functions.
>>>>>>>>>>>>>>>> */
>>>>>>>>>>>>>>>> @@ -12361,8 +12362,19 @@
>>>>>>>>>>>>>>>> check_special_function_return_type
>>>>>>>>>>>>>>>> (special_function_kind sfk,
>>>>>>>>>>>>>>>>                            tree
>>>>>>>>>>>>>>>> type,
>>>>>>>>>>>>>>>>                            tree
>>>>>>>>>>>>>>>> optype,
>>>>>>>>>>>>>>>>                            int
>>>>>>>>>>>>>>>> type_quals,
>>>>>>>>>>>>>>>> +                    const
>>>>>>>>>>>>>>>> cp_declarator
>>>>>>>>>>>>>>>> *declarator,
>>>>>>>>>>>>>>>> +                    location_t
>>>>>>>>>>>>>>>> id_loc,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> id_loc should be the same as declarator->id_loc?
>>>>>>>>>>>>>> You’re right.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>                            const
>>>>>>>>>>>>>>>> location_t*
>>>>>>>>>>>>>>>> locations)
>>>>>>>>>>>>>>>>        {
>>>>>>>>>>>>>>>> +  /* If TYPE is unspecified, DECLARATOR, if set, 
>>>>>>>>>>>>>>>> should
>>>>>>>>>>>>>>>> not
>>>>>>>>>>>>>>>> represent a pointer
>>>>>>>>>>>>>>>> +     or a reference type.  */

>>>>>>>>>>>>>>>> +  if (type == NULL_TREE
>>>>>>>>>>>>>>>> +      && declarator
>>>>>>>>>>>>>>>> +      && (declarator->kind == cdk_pointer
>>>>>>>>>>>>>>>> +      || declarator->kind == cdk_reference))
>>>>>>>>>>>>>>>> +    error_at (id_loc, "expected unqualified-id 
>>>>>>>>>>>>>>>> before
>>>>>>>>>>>>>>>> %qs
>>>>>>>>>>>>>>>> token",
>>>>>>>>>>>>>>>> +          declarator->kind == cdk_pointer ? 
>>>>>>>>>>>>>>>> "*"
>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>> "&");
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ...and id_loc isn't the location of the ptr-operator, 

>>>>>>>>>>>>>>> it's
>>
>>>>>>>>>>>>>>> the
>>>>>>
>>>>>>>>>>>>>>> location of the identifier, so this indicates the wrong
>>>>>>>>>>>>>>> column.
>>>>>>>>>>>>>>> I
>>>>>>>>>>>>>>> think using declarator->id_loc makes sense, just not
>>>>>>>>>>>>>>> pretending
>>>>>>>>>>>>>>> it's
>>>>>>>>>>>>>>> the location of the *.
>>>>>>>>>>>>>> Good catch, thanks.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Let's give diagnostics more like the others later in the

>>>>>>>>>>>>>>> function
>>>>>>>>>>>>>>> instead of trying to emulate cp_parser_error.
>>>>>>>>>>>>>> Makes sense. This is what the updated patch does,
>>>>>>>>>>>>>> successfully
>>>>>>>>>>>>>> tested on
>>>>>>>>>>>>>> x86_64-pc-linux-gnu. OK for GCC 16?
>>>>>>>>>>>>>
>>>>>>>>>>>>> OK.
>>>>>>>>>>>>
>>>>>>>>>>>> Does this also fix 118304?  If so, let's go ahead and 
>>>>>>>>>>>> apply
>>>>>>>>>>>> it
>>>>>>>>>>>> to
>>>>>>>>>>>> GCC
>>>>>>>>>>>> 15.
>>>>>>>>>>> I have checked just now, and we still ICE for 118304’s
>>
>>>>>>>>>>> testcase
>>>>>>>>>>> with
>>>>>>>>>>> that fix.
>>>>>>>>>>
>>>>>>>>>> Why doesn't the preeexisting
>>>>>>>>>>
>>>>>>>>>> type = void_type_node;
>>>>>>>>>>
>>>>>>>>>> in check_special_function_return_type fix the return type and

>>>>>>>>>> avoid
>>>>>>
>>>>>>>>>> the ICE?
>>>>>>>>
>>>>>>>>> We hit the gcc_assert at method.cc:3593, that Marek’s fix
>>>>>>
>>>>>>>>> bypasses.
>>>>>>>>
>>>>>>>> Yes, but why doesn't check_special_function_return_type prevent

>>>>>>>> that?
>>>>>>>
>>>>>>> Ah, because we call it before walking the declarator.  We need 
>>>>>>> to
>>>>>>> check again later, perhaps in grokfndecl, that the type is
>>>>>>> correct.
>>>>>>> Perhaps instead of your patch.
>>>>>> One “issue” with adding another check in or close to 
>>>>>> grokfndecl
>>>>>> is
>>>>>> that DECLARATOR will have “been moved to the ID”, and the 
>>>>>> fact
>>>>>> that
>>>>>> we had a CDK_POINTER kind is “lost”. We could obviously 
>>>>>> somehow
>>>>>> propagate this information, but there might be something easier.
>>>>>
>>>>> The information isn't lost: it's now reflected in the (wrong) 
>>>>> return
>>
>>>>> type.  One place it would make sense to check would be
>>>>>
>>>>>>               if (ctype && (sfk == sfk_constructor
>>>>>>                             || sfk == sfk_destructor))
>>>>>>                 {
>>>>>>                   /* We are within a class's scope. If our
>>>>>> declarator
>>>>>> name
>>>>>>                 is the same as the class name, and we are 
>>>>>> defining
>>>>>>                                                                                a
>>>>>> function, then it is a constructor/destructor, and
>>>>>>                                                                therefore
>>>>>> returns a void type.  */
>>>>>
>>>>> Here 'type' is still the return type, we haven't gotten to
>>>>> build_function_type yet.
>>>> That’s true. However, doesn’t it make sense to cram all the
>>>> checks
>>>> about the return type of special functions in
>>>> check_special_function_return_type, and return an error if that
>>>> return
>>>> type is invalid?
>>>
>>> This error seems easily recoverable since we know what the type 
>>> needs
>>> to be, there's no need for error return from grokdeclarator.
>> ACK.
>>
>>> However, an alternative to my suggestion above would be to build on
>>> your patch by making check_special_function_return_type actually 
>>> strip
>>> the invalid declarators, not just complain about them.
>
>> Thanks for the suggestion, I like that it keeps everything within
>> check_special_function_return_type. Hence the updated attached patch,

>>
>> successfully tested on 86_64-pc-linux-gnu. OK for trunk?
>
>>      case sfk_constructor:
>>        if (type)
>>  	error_at (smallest_type_location (type_quals, locations),
>>  		  "return type specification for constructor invalid");
>> +      else if (maybe_strip_indirect_ref (declarator))
>> +	error_at ((*declarator)->id_loc,
>> +		  "return type specification for constructor invalid");
>>        else if (type_quals != TYPE_UNQUALIFIED)
>>  	error_at (smallest_type_quals_location (type_quals, locations),
>>  		  "qualifiers are not allowed on constructor declaration");
>
> This looks like if someone writes e.g.
>
> int *A();
>
> then we'll complain about the 'int' but leave the * alone.
Argh you’re right, the return type is not fully groked at this
point, so check_special_function_return_type should check first
for indirect refs and then for type if needed. This is what the
updated attached patch does

Successfully tested on x86_64-pc-linux-gnu. OK for trunk?

Thanks, Simon
From 140bd5fa4f98fcb2ac32b99e4403dd4875afe9c5 Mon Sep 17 00:00:00 2001
From: Simon Martin <simon@nasilyan.com>
Date: Sun, 9 Feb 2025 20:38:43 +0100
Subject: [PATCH] c++: Reject cdtors and conversion operators with a single *
 as return type [PR118304, PR118306]

We currently accept the following constructor declaration (clang, EDG
and MSVC do as well), and ICE on the destructor declaration

=== cut here ===
struct A {
  *A ();
  ~A () = default;
};
=== cut here ===

The problem is that we end up in grokdeclarator with a cp_declarator of
kind cdk_pointer but no type, and we happily go through (if we have a
reference instead we eventually error out trying to form a reference to
void).

This patch makes sure that grokdeclarator errors out and strips the
invalid declarator when processing a cdtor (or a conversion operator
with no return type specified) with a declarator representing a pointer
or a reference type.

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/118306
	PR c++/118304

gcc/cp/ChangeLog:

	* decl.cc (maybe_strip_indirect_ref): New.
	(check_special_function_return_type): Take declarator as input.
	Call maybe_strip_indirect_ref and error out if it returns true.
	(grokdeclarator): Update call to
	check_special_function_return_type.

gcc/testsuite/ChangeLog:

	* g++.old-deja/g++.jason/operator.C: Adjust bogus test
	expectation (char** vs char*).
	* g++.dg/parse/constructor4.C: New test.
	* g++.dg/parse/constructor5.C: New test.
	* g++.dg/parse/conv_op2.C: New test.
	* g++.dg/parse/default_to_int.C: New test.

---
 gcc/cp/decl.cc                                | 50 ++++++++++++-----
 gcc/testsuite/g++.dg/parse/constructor4.C     | 54 +++++++++++++++++++
 gcc/testsuite/g++.dg/parse/constructor5.C     | 48 +++++++++++++++++
 gcc/testsuite/g++.dg/parse/conv_op2.C         | 10 ++++
 gcc/testsuite/g++.dg/parse/default_to_int.C   | 37 +++++++++++++
 .../g++.old-deja/g++.jason/operator.C         |  2 +-
 6 files changed, 187 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/parse/constructor4.C
 create mode 100644 gcc/testsuite/g++.dg/parse/constructor5.C
 create mode 100644 gcc/testsuite/g++.dg/parse/conv_op2.C
 create mode 100644 gcc/testsuite/g++.dg/parse/default_to_int.C

-- 
2.44.0
  

On 2/10/25 12:09 PM, Simon Martin wrote:
> Hi Jason,
> 
> On 7 Feb 2025, at 23:10, Jason Merrill wrote:
> 
>> On 2/7/25 4:04 PM, Simon Martin wrote:
>>> Hi Jason,
>>>
>>> On 7 Feb 2025, at 14:21, Jason Merrill wrote:
>>>
>>>> On 2/6/25 3:05 PM, Simon Martin wrote:
>>>>> Hi Jason,
>>>>>
>>>>> On 6 Feb 2025, at 16:48, Jason Merrill wrote:
>>>>>
>>>>>> On 2/5/25 2:21 PM, Simon Martin wrote:
>>>>>>> Hi Jason,
>>>>>>>
>>>>>>> On 4 Feb 2025, at 21:23, Jason Merrill wrote:
>>>>>>>
>>>>>>>> On 2/4/25 3:03 PM, Jason Merrill wrote:
>>>>>>>>> On 2/4/25 11:45 AM, Simon Martin wrote:
>>>>>>>>>> On 4 Feb 2025, at 17:17, Jason Merrill wrote:
>>>>>>>>>>
>>>>>>>>>>> On 2/4/25 10:56 AM, Simon Martin wrote:
>>>>>>>>>>>> Hi Jason,
>>>>>>>>>>>>
>>>>>>>>>>>> On 4 Feb 2025, at 16:39, Jason Merrill wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> On 1/15/25 9:56 AM, Jason Merrill wrote:
>>>>>>>>>>>>>> On 1/15/25 7:24 AM, Simon Martin wrote:
>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 14 Jan 2025, at 23:31, Jason Merrill wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 1/14/25 2:13 PM, Simon Martin wrote:
>>>>>>>>>>>>>>>>> On 10 Jan 2025, at 19:10, Andrew Pinski wrote:
>>>>>>>>>>>>>>>>>> On Fri, Jan 10, 2025 at 3:18 AM Simon Martin
>>>>>>>>>>>>>>>>>> <simon@nasilyan.com>
>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> We currently accept the following invalid code (EDG
>>>>>>>>>>>>>>>>>>> and
>>>
>>>>>>>>>>>>>>>>>>> MSVC
>>>>>>>>>>>>>>>>>>> do
>>>>>>>>>>>>>>>>>>> as
>>>>>>>>>>>>>>>>>>> well)
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> clang does too:
>>>>>>>>>>>>>>>>>> https://github.com/llvm/llvm-project/issues/121706 .
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Note it might be useful if a testcase with multiply
> 
>>>>>>>>>>>>>>>>>> `*`
>>>>>>>>>>>>>>>>>> is
>>>>>>>>>>>>>>>>>> included
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> too:
>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>> struct A {
>>>>>>>>>>>>>>>>>>           ****A ();
>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>> Thanks, makes sense to add those. Done in the attached
>>>>>>>>>>>>>>>>> updated
>>>>>>>>>>>>>>>>> revision,
>>>>>>>>>>>>>>>>> successfully tested on x86_64-pc-linux-gnu.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> +/* Check that it's OK to declare a function at ID_LOC
>>>
>>>>>>>>>>>>>>>>> with
>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>> indicated TYPE,
>>>>>>>>>>>>>>>>> +   TYPE_QUALS and DECLARATOR.  SFK indicates the
>>>>>>>>>>>>>>>>> kind
>>>>>>>>>>>>>>>>> of
>>>>>>>>>>>>>>>>> special
>>>>>>>>>>>>>>>>> function (if
>>>>>>>>>>>>>>>>> +   any) that this function is.  OPTYPE is the type
>>>>>>>>>>>>>>>>> given
>>>>>>>>>>>>>>>>> in
>>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>>> conversion
>>>>>>>>>>>>>>>>>            operator declaration, or the class type
>>>>>>>>>>>>>>>>> for
>>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>>> constructor/destructor.
>>>>>>>>>>>>>>>>>            Returns the actual return type of the
>>>>>>>>>>>>>>>>> function;
>>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>>> may
>>>>>>>>>>>>>>>>> be
>>>>>>>>>>>>>>>>> different
>>>>>>>>>>>>>>>>>            than TYPE if an error occurs, or for
>>>>>>>>>>>>>>>>> certain
>>>>>>>>>>>>>>>>> special
>>>>>>>>>>>>>>>>> functions.
>>>>>>>>>>>>>>>>> */
>>>>>>>>>>>>>>>>> @@ -12361,8 +12362,19 @@
>>>>>>>>>>>>>>>>> check_special_function_return_type
>>>>>>>>>>>>>>>>> (special_function_kind sfk,
>>>>>>>>>>>>>>>>>                             tree
>>>>>>>>>>>>>>>>> type,
>>>>>>>>>>>>>>>>>                             tree
>>>>>>>>>>>>>>>>> optype,
>>>>>>>>>>>>>>>>>                             int
>>>>>>>>>>>>>>>>> type_quals,
>>>>>>>>>>>>>>>>> +                    const
>>>>>>>>>>>>>>>>> cp_declarator
>>>>>>>>>>>>>>>>> *declarator,
>>>>>>>>>>>>>>>>> +                    location_t
>>>>>>>>>>>>>>>>> id_loc,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> id_loc should be the same as declarator->id_loc?
>>>>>>>>>>>>>>> You’re right.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>                             const
>>>>>>>>>>>>>>>>> location_t*
>>>>>>>>>>>>>>>>> locations)
>>>>>>>>>>>>>>>>>         {
>>>>>>>>>>>>>>>>> +  /* If TYPE is unspecified, DECLARATOR, if set,
>>>>>>>>>>>>>>>>> should
>>>>>>>>>>>>>>>>> not
>>>>>>>>>>>>>>>>> represent a pointer
>>>>>>>>>>>>>>>>> +     or a reference type.  */
> 
>>>>>>>>>>>>>>>>> +  if (type == NULL_TREE
>>>>>>>>>>>>>>>>> +      && declarator
>>>>>>>>>>>>>>>>> +      && (declarator->kind == cdk_pointer
>>>>>>>>>>>>>>>>> +      || declarator->kind == cdk_reference))
>>>>>>>>>>>>>>>>> +    error_at (id_loc, "expected unqualified-id
>>>>>>>>>>>>>>>>> before
>>>>>>>>>>>>>>>>> %qs
>>>>>>>>>>>>>>>>> token",
>>>>>>>>>>>>>>>>> +          declarator->kind == cdk_pointer ?
>>>>>>>>>>>>>>>>> "*"
>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>> "&");
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> ...and id_loc isn't the location of the ptr-operator,
> 
>>>>>>>>>>>>>>>> it's
>>>
>>>>>>>>>>>>>>>> the
>>>>>>>
>>>>>>>>>>>>>>>> location of the identifier, so this indicates the wrong
>>>>>>>>>>>>>>>> column.
>>>>>>>>>>>>>>>> I
>>>>>>>>>>>>>>>> think using declarator->id_loc makes sense, just not
>>>>>>>>>>>>>>>> pretending
>>>>>>>>>>>>>>>> it's
>>>>>>>>>>>>>>>> the location of the *.
>>>>>>>>>>>>>>> Good catch, thanks.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Let's give diagnostics more like the others later in the
> 
>>>>>>>>>>>>>>>> function
>>>>>>>>>>>>>>>> instead of trying to emulate cp_parser_error.
>>>>>>>>>>>>>>> Makes sense. This is what the updated patch does,
>>>>>>>>>>>>>>> successfully
>>>>>>>>>>>>>>> tested on
>>>>>>>>>>>>>>> x86_64-pc-linux-gnu. OK for GCC 16?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> OK.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Does this also fix 118304?  If so, let's go ahead and
>>>>>>>>>>>>> apply
>>>>>>>>>>>>> it
>>>>>>>>>>>>> to
>>>>>>>>>>>>> GCC
>>>>>>>>>>>>> 15.
>>>>>>>>>>>> I have checked just now, and we still ICE for 118304’s
>>>
>>>>>>>>>>>> testcase
>>>>>>>>>>>> with
>>>>>>>>>>>> that fix.
>>>>>>>>>>>
>>>>>>>>>>> Why doesn't the preeexisting
>>>>>>>>>>>
>>>>>>>>>>> type = void_type_node;
>>>>>>>>>>>
>>>>>>>>>>> in check_special_function_return_type fix the return type and
> 
>>>>>>>>>>> avoid
>>>>>>>
>>>>>>>>>>> the ICE?
>>>>>>>>>
>>>>>>>>>> We hit the gcc_assert at method.cc:3593, that Marek’s fix
>>>>>>>
>>>>>>>>>> bypasses.
>>>>>>>>>
>>>>>>>>> Yes, but why doesn't check_special_function_return_type prevent
> 
>>>>>>>>> that?
>>>>>>>>
>>>>>>>> Ah, because we call it before walking the declarator.  We need
>>>>>>>> to
>>>>>>>> check again later, perhaps in grokfndecl, that the type is
>>>>>>>> correct.
>>>>>>>> Perhaps instead of your patch.
>>>>>>> One “issue” with adding another check in or close to
>>>>>>> grokfndecl
>>>>>>> is
>>>>>>> that DECLARATOR will have “been moved to the ID”, and the
>>>>>>> fact
>>>>>>> that
>>>>>>> we had a CDK_POINTER kind is “lost”. We could obviously
>>>>>>> somehow
>>>>>>> propagate this information, but there might be something easier.
>>>>>>
>>>>>> The information isn't lost: it's now reflected in the (wrong)
>>>>>> return
>>>
>>>>>> type.  One place it would make sense to check would be
>>>>>>
>>>>>>>                if (ctype && (sfk == sfk_constructor
>>>>>>>                              || sfk == sfk_destructor))
>>>>>>>                  {
>>>>>>>                    /* We are within a class's scope. If our
>>>>>>> declarator
>>>>>>> name
>>>>>>>                  is the same as the class name, and we are
>>>>>>> defining
>>>>>>>                                                                                 a
>>>>>>> function, then it is a constructor/destructor, and
>>>>>>>                                                                 therefore
>>>>>>> returns a void type.  */
>>>>>>
>>>>>> Here 'type' is still the return type, we haven't gotten to
>>>>>> build_function_type yet.
>>>>> That’s true. However, doesn’t it make sense to cram all the
>>>>> checks
>>>>> about the return type of special functions in
>>>>> check_special_function_return_type, and return an error if that
>>>>> return
>>>>> type is invalid?
>>>>
>>>> This error seems easily recoverable since we know what the type
>>>> needs
>>>> to be, there's no need for error return from grokdeclarator.
>>> ACK.
>>>
>>>> However, an alternative to my suggestion above would be to build on
>>>> your patch by making check_special_function_return_type actually
>>>> strip
>>>> the invalid declarators, not just complain about them.
>>
>>> Thanks for the suggestion, I like that it keeps everything within
>>> check_special_function_return_type. Hence the updated attached patch,
> 
>>>
>>> successfully tested on 86_64-pc-linux-gnu. OK for trunk?
>>
>>>       case sfk_constructor:
>>>         if (type)
>>>   	error_at (smallest_type_location (type_quals, locations),
>>>   		  "return type specification for constructor invalid");
>>> +      else if (maybe_strip_indirect_ref (declarator))
>>> +	error_at ((*declarator)->id_loc,
>>> +		  "return type specification for constructor invalid");
>>>         else if (type_quals != TYPE_UNQUALIFIED)
>>>   	error_at (smallest_type_quals_location (type_quals, locations),
>>>   		  "qualifiers are not allowed on constructor declaration");
>>
>> This looks like if someone writes e.g.
>>
>> int *A();
>>
>> then we'll complain about the 'int' but leave the * alone.
> Argh you’re right, the return type is not fully groked at this
> point, so check_special_function_return_type should check first
> for indirect refs and then for type if needed. This is what the
> updated attached patch does
> 
> Successfully tested on x86_64-pc-linux-gnu. OK for trunk?

OK, thanks.

Jason