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
>>>>>>>>>>> <si...@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.
It actually feels like having check_special_function_return_type return
error_mark_node in case of error makes sense. If I amend my patch to do
so (see attachment), then we fix both PR108306 and PR108304, and full
testing is successful on x86_64-pc-linux-gnu (modulo tweaking a couple
of tests that were expecting what we can consider spurious error
messages).
Am I missing anything, or should we go ahead with this approach and
merge this updated patch to GCC15?
SimonFrom 192fab4034a4f457b8a9e93016e6b68a30671c6c Mon Sep 17 00:00:00 2001
From: Simon Martin <si...@nasilyan.com>
Date: Wed, 15 Jan 2025 09:54:37 +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 returns
error_mark_node when processing a cdtor or a conversion operator with no
return type specified but also 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 (indirect_ref_declarator_p): New.
(check_special_function_return_type): Take declarator
as input. Call indirect_ref_declarator_p and reject return type
specifiers with only a * or &. Return error_mark_node in case of
error.
(grokdeclarator): Update call to
check_special_function_return_type.
gcc/testsuite/ChangeLog:
* g++.dg/parse/friend7.C: Adjust test expectation.
* g++.old-deja/g++.brendan/crash16.C: Ditto.
* g++.old-deja/g++.jason/operator.C: Ditto.
* g++.old-deja/g++.law/ctors5.C: Ditto.
* 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 | 71 ++++++++++++++-----
gcc/testsuite/g++.dg/parse/constructor4.C | 54 ++++++++++++++
gcc/testsuite/g++.dg/parse/constructor5.C | 14 ++++
gcc/testsuite/g++.dg/parse/conv_op2.C | 10 +++
gcc/testsuite/g++.dg/parse/default_to_int.C | 37 ++++++++++
gcc/testsuite/g++.dg/parse/friend7.C | 2 -
.../g++.old-deja/g++.brendan/crash16.C | 5 +-
.../g++.old-deja/g++.jason/operator.C | 1 -
gcc/testsuite/g++.old-deja/g++.law/ctors5.C | 1 -
9 files changed, 171 insertions(+), 24 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
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index b7af33b3231..9db1bd1cf42 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -101,7 +101,8 @@ static void end_cleanup_fn (void);
static tree cp_make_fname_decl (location_t, tree, int);
static void initialize_predefined_identifiers (void);
static tree check_special_function_return_type
- (special_function_kind, tree, tree, int, const location_t*);
+ (special_function_kind, tree, tree, int, const cp_declarator*,
+ const location_t*);
static tree push_cp_library_fn (enum tree_code, tree, int);
static tree build_cp_library_fn (tree, enum tree_code, tree, int);
static void store_parm_decls (tree);
@@ -12427,10 +12428,19 @@ smallest_type_location (const cp_decl_specifier_seq
*declspecs)
return smallest_type_location (type_quals, declspecs->locations);
}
-/* Check that it's OK to declare a function with the indicated TYPE
- and TYPE_QUALS. 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 whether DECLARATOR represents a pointer or a reference. */
+
+static bool
+indirect_ref_declarator_p (const cp_declarator* declarator)
+{
+ return declarator && (declarator->kind == cdk_pointer
+ || declarator->kind == cdk_reference);
+}
+
+/* Check that it's OK to declare a function 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. */
@@ -12439,16 +12449,32 @@ check_special_function_return_type
(special_function_kind sfk,
tree type,
tree optype,
int type_quals,
+ const cp_declarator* declarator,
const location_t* locations)
{
+#define ERROR_AT(...) \
+ do { \
+ has_error = true; \
+ error_at (__VA_ARGS__); \
+ } while (0)
+#define ERROR(...) \
+ do { \
+ has_error = true; \
+ error (__VA_ARGS__); \
+ } while (0)
+
+ bool has_error = false;
switch (sfk)
{
case sfk_constructor:
if (type)
- error_at (smallest_type_location (type_quals, locations),
+ ERROR_AT (smallest_type_location (type_quals, locations),
+ "return type specification for constructor invalid");
+ else if (indirect_ref_declarator_p (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),
+ ERROR_AT (smallest_type_quals_location (type_quals, locations),
"qualifiers are not allowed on constructor declaration");
if (targetm.cxx.cdtor_returns_this ())
@@ -12459,10 +12485,13 @@ check_special_function_return_type
(special_function_kind sfk,
case sfk_destructor:
if (type)
- error_at (smallest_type_location (type_quals, locations),
+ ERROR_AT (smallest_type_location (type_quals, locations),
+ "return type specification for destructor invalid");
+ else if (indirect_ref_declarator_p (declarator))
+ ERROR_AT (declarator->id_loc,
"return type specification for destructor invalid");
else if (type_quals != TYPE_UNQUALIFIED)
- error_at (smallest_type_quals_location (type_quals, locations),
+ ERROR_AT (smallest_type_quals_location (type_quals, locations),
"qualifiers are not allowed on destructor declaration");
/* We can't use the proper return type here because we run into
@@ -12475,10 +12504,13 @@ check_special_function_return_type
(special_function_kind sfk,
case sfk_conversion:
if (type)
- error_at (smallest_type_location (type_quals, locations),
+ ERROR_AT (smallest_type_location (type_quals, locations),
+ "return type specified for %<operator %T%>", optype);
+ else if (indirect_ref_declarator_p (declarator))
+ ERROR_AT (declarator->id_loc,
"return type specified for %<operator %T%>", optype);
else if (type_quals != TYPE_UNQUALIFIED)
- error_at (smallest_type_quals_location (type_quals, locations),
+ ERROR_AT (smallest_type_quals_location (type_quals, locations),
"qualifiers are not allowed on declaration of "
"%<operator %T%>", optype);
@@ -12487,31 +12519,35 @@ check_special_function_return_type
(special_function_kind sfk,
case sfk_deduction_guide:
if (type)
- error_at (smallest_type_location (type_quals, locations),
+ ERROR_AT (smallest_type_location (type_quals, locations),
+ "return type specified for deduction guide");
+ else if (indirect_ref_declarator_p (declarator))
+ ERROR_AT (declarator->id_loc,
"return type specified for deduction guide");
else if (type_quals != TYPE_UNQUALIFIED)
- error_at (smallest_type_quals_location (type_quals, locations),
+ ERROR_AT (smallest_type_quals_location (type_quals, locations),
"qualifiers are not allowed on declaration of "
"deduction guide");
if (TREE_CODE (optype) == TEMPLATE_TEMPLATE_PARM)
{
- error ("template template parameter %qT in declaration of "
+ ERROR ("template template parameter %qT in declaration of "
"deduction guide", optype);
- type = error_mark_node;
}
else
type = make_template_placeholder (CLASSTYPE_TI_TEMPLATE (optype));
for (int i = 0; i < ds_last; ++i)
if (i != ds_explicit && locations[i])
- error_at (locations[i],
+ ERROR_AT (locations[i],
"%<decl-specifier%> in declaration of deduction guide");
break;
default:
gcc_unreachable ();
}
+#undef ERROR_AT
+#undef ERROR
- return type;
+ return !has_error ? type : error_mark_node;
}
/* A variable or data member (whose unqualified name is IDENTIFIER)
@@ -13167,6 +13203,7 @@ grokdeclarator (const cp_declarator *declarator,
type = check_special_function_return_type (sfk, type,
ctor_return_type,
type_quals,
+ declarator,
declspecs->locations);
type_quals = TYPE_UNQUALIFIED;
}
diff --git a/gcc/testsuite/g++.dg/parse/constructor4.C
b/gcc/testsuite/g++.dg/parse/constructor4.C
new file mode 100644
index 00000000000..f7e4cace451
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/constructor4.C
@@ -0,0 +1,54 @@
+// PR c++/118306
+// { dg-do "compile" }
+
+// Constructors.
+struct A {
+ *A (); // { dg-error "return type specification" }
+};
+struct B {
+ **B (); // { dg-error "return type specification" }
+};
+struct C {
+ ***C (); // { dg-error "return type specification" }
+};
+struct D {
+ &D (); // { dg-error "return type specification|reference to" }
+};
+struct E {
+ *&E (); // { dg-error "return type specification|reference to" }
+};
+struct F {
+ **&F (); // { dg-error "return type specification|reference to" }
+};
+struct G {
+ *G (const G&); // { dg-error "return type specification" }
+};
+struct H {
+ **H (const H&); // { dg-error "return type specification" }
+};
+struct I {
+ &I (const I&); // { dg-error "return type specification|reference to" }
+};
+struct J {
+ const J(); // { dg-error "expected unqualified-id" }
+};
+
+// Destructors.
+struct K {
+ * ~K (); // { dg-error "return type specification" }
+};
+struct L {
+ ** ~L (); // { dg-error "return type specification" }
+};
+struct M {
+ & ~M (); // { dg-error "return type specification|reference to" }
+};
+struct N {
+ virtual * ~N (); // { dg-error "return type specification" }
+};
+struct O {
+ virtual & ~O (); // { dg-error "return type specification|reference to" }
+};
+struct P {
+ volatile ~P(); // { dg-error "qualifiers are not allowed" }
+};
diff --git a/gcc/testsuite/g++.dg/parse/constructor5.C
b/gcc/testsuite/g++.dg/parse/constructor5.C
new file mode 100644
index 00000000000..34de286fb67
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/constructor5.C
@@ -0,0 +1,14 @@
+// PR c++/118304
+// { dg-do "compile" { target c++11 } }
+
+struct A {
+ *A() = default; // { dg-error "return type specification" }
+};
+
+struct B {
+ &B (const B&) = default; // { dg-error "return type specification|reference
to" }
+};
+
+struct C {
+ & ~C () = default; // { dg-error "return type specification|reference to" }
+};
diff --git a/gcc/testsuite/g++.dg/parse/conv_op2.C
b/gcc/testsuite/g++.dg/parse/conv_op2.C
new file mode 100644
index 00000000000..f2719135e00
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/conv_op2.C
@@ -0,0 +1,10 @@
+// PR c++/118306
+// { dg-do "compile" }
+
+struct K {
+ char operator int(); // { dg-error "return type specified for" }
+ * operator short(); // { dg-error "return type specified for" }
+ ** operator float(); // { dg-error "return type specified for" }
+ &* operator double(); // { dg-error "return type specified
for|pointer to 'double&'" }
+ & operator long(); // { dg-error "return type specified for" }
+};
diff --git a/gcc/testsuite/g++.dg/parse/default_to_int.C
b/gcc/testsuite/g++.dg/parse/default_to_int.C
new file mode 100644
index 00000000000..681298ce634
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/default_to_int.C
@@ -0,0 +1,37 @@
+// PR c++/118306 - "Document" various behaviours wrt. defaulting types to int.
+// { dg-do "compile" }
+// { dg-additional-options "-fpermissive" }
+
+// Members.
+struct K {
+ * mem1; // { dg-warning "forbids declaration" }
+ * mem2; // { dg-warning "forbids declaration" }
+ const * mem3; // { dg-warning "forbids declaration" }
+ const ** mem4; // { dg-warning "forbids declaration" }
+ & mem5; // { dg-warning "forbids declaration" }
+ volatile & mem6; // { dg-warning "forbids declaration" }
+
+ void foo (const& permissive_fine, // { dg-warning "forbids
declaration" }
+ volatile* permissive_fine_as_well); // { dg-warning "forbids
declaration" }
+
+ * bar () { return 0; } // { dg-warning "forbids declaration" }
+ const& baz (); // { dg-warning "forbids declaration" }
+
+ void bazz () {
+ try {}
+ catch (const *i) {} // { dg-warning "forbids" }
+ catch (const &i) {} // { dg-warning "forbids" }
+ }
+};
+
+// Template parameters.
+template<const *i, const &j> // { dg-warning "forbids" }
+void baz() {}
+
+// Functions.
+foo(int) { return 42; } // { dg-warning "forbids
declaration" }
+*bar(int) { return 0; } // { dg-warning "forbids
declaration" }
+**bazz(int) { return 0; } // { dg-warning "forbids declaration" }
+*&bazzz(int) { return 0; } // { dg-warning "forbids declaration|bind
non-const" }
+const bazzzz (int) { return 0; } // { dg-warning "forbids declaration" }
+const* bazzzzz (int) { return 0; } // { dg-warning "forbids declaration" }
diff --git a/gcc/testsuite/g++.dg/parse/friend7.C
b/gcc/testsuite/g++.dg/parse/friend7.C
index 19e31795c24..d78c1cf94cc 100644
--- a/gcc/testsuite/g++.dg/parse/friend7.C
+++ b/gcc/testsuite/g++.dg/parse/friend7.C
@@ -20,7 +20,6 @@ struct C
{
friend int C ();
friend int ~C (); // { dg-error "10:return type" }
- // { dg-error "14:expected qualified name in friend decl" "" { target *-*-*
} .-1 }
friend int C (const C &);
};
@@ -28,7 +27,6 @@ struct D
{
friend int D () {}
friend int ~D () {} // { dg-error "10:return type" }
- // { dg-error "14:expected qualified name in friend decl" "" { target *-*-*
} .-1 }
friend int D (const D &) {}
};
diff --git a/gcc/testsuite/g++.old-deja/g++.brendan/crash16.C
b/gcc/testsuite/g++.old-deja/g++.brendan/crash16.C
index 2edb2a8711b..97d810f14dd 100644
--- a/gcc/testsuite/g++.old-deja/g++.brendan/crash16.C
+++ b/gcc/testsuite/g++.old-deja/g++.brendan/crash16.C
@@ -6,10 +6,9 @@ class Graph { // { dg-error "1:new types|1: note: \\(perhaps" }
// { dg-error "1:return type" "" { target *-*-* } .-1 }
public:
unsigned char N;
- Graph(void) {} // { dg-message "7:'Graph" }
+ Graph(void) {}
}
-Graph::Graph(void) // { dg-error "1:redefinition" }
+Graph::Graph(void)
{ N = 10;
}
-
diff --git a/gcc/testsuite/g++.old-deja/g++.jason/operator.C
b/gcc/testsuite/g++.old-deja/g++.jason/operator.C
index c18790190b5..ad02f262d6e 100644
--- a/gcc/testsuite/g++.old-deja/g++.jason/operator.C
+++ b/gcc/testsuite/g++.old-deja/g++.jason/operator.C
@@ -29,4 +29,3 @@ void * operator new (A a); // { dg-error ".operator new.
takes type .size_t." }
void operator delete (A a); // { dg-error ".operator delete. takes type
.void\\*. as first parameter" }
char * operator char * (int); // { dg-error "return type" "ret" }
-// { dg-error "8:.operator char\\*\\*\\(int\\). must be a non-static member
function" "mem" { target *-*-* } .-1 }
diff --git a/gcc/testsuite/g++.old-deja/g++.law/ctors5.C
b/gcc/testsuite/g++.old-deja/g++.law/ctors5.C
index 492c429ab17..d892414ce7c 100644
--- a/gcc/testsuite/g++.old-deja/g++.law/ctors5.C
+++ b/gcc/testsuite/g++.old-deja/g++.law/ctors5.C
@@ -24,7 +24,6 @@ class Y // { dg-error "1:new types may not be defined in a
return type" "err" }
Y();
}
X::X( int xi )
-// { dg-message "1:X::X|candidate expects" "match candidate text" { target
*-*-* } .-1 }
{
x = xi;
}
--
2.44.0
