Hi Jason,
On 9 Jan 2025, at 22:55, Jason Merrill wrote:
> On 1/9/25 8:25 AM, Simon Martin wrote:
>> We segfault upon the following invalid code
>>
>> === cut here ===
>> template <int> struct S {
>> friend void foo (int a = []{}());
>> };
>> void foo (int a) {}
>> int main () {
>> S<0> t;
>> foo ();
>> }
>> === cut here ===
>>
>> The problem is that we end up with a LAMBDA_EXPR callee in
>> set_flags_from_callee, and dereference its NULL_TREE
>> TREE_TYPE (TREE_TYPE ( )).
>>
>> This patch simply sets the default argument to error_mark_node for
>> friend functions that do not meet the requirement in C++17 11.3.6/4.
>>
>> Successfully tested on x86_64-pc-linux-gnu.
>>
>> PR c++/118319
>>
>> gcc/cp/ChangeLog:
>>
>> * decl.cc (grokfndecl): Inspect all friend function parameters,
>> and set them to error_mark_node if invalid.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * g++.dg/parse/defarg18.C: New test.
>>
>> ---
>> gcc/cp/decl.cc | 13 +++++---
>> gcc/testsuite/g++.dg/parse/defarg18.C | 48
>> +++++++++++++++++++++++++++
>> 2 files changed, 57 insertions(+), 4 deletions(-)
>> create mode 100644 gcc/testsuite/g++.dg/parse/defarg18.C
>>
>> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
>> index 503ecd9387e..b2761c23d3e 100644
>> --- a/gcc/cp/decl.cc
>> +++ b/gcc/cp/decl.cc
>> @@ -11134,14 +11134,19 @@ grokfndecl (tree ctype,
>> expression, that declaration shall be a definition..." */
>> if (friendp && !funcdef_flag)
>> {
>> + bool has_permerrored = false;
>> for (tree t = FUNCTION_FIRST_USER_PARMTYPE (decl);
>> t && t != void_list_node; t = TREE_CHAIN (t))
>> if (TREE_PURPOSE (t))
>> {
>> - permerror (DECL_SOURCE_LOCATION (decl),
>> - "friend declaration of %qD specifies default "
>> - "arguments and isn%'t a definition", decl);
>> - break;
>> + if (!has_permerrored)
>> + {
>> + has_permerrored = true;
>> + permerror (DECL_SOURCE_LOCATION (decl),
>> + "friend declaration of %qD specifies default "
>> + "arguments and isn%'t a definition", decl);
>> + }
>> + TREE_PURPOSE (t) = error_mark_node;
>
> If we're going to unconditionally change TREE_PURPOSE, then permerror
> needs to strengthen to error. But I'd think we could leave the
> current state in a non-template class, only changing the template
> case.
Thanks. It’s true that setting the argument to error_mark_node is
contradictory with the fact that we accept the code with -fpermissive,
even if only under processing_template_decl, so I checked if there’s
not a better way of approaching this PR.
After a bit of investigation, I think that the real problem is that
duplicate_decls tries to merge the two declarations, even though they
don’t meet the constraint about friend functions and default
arguments.
So the updated patch changes check_no_redeclaration_friend_default_args
to return whether the two declarations can be merged, and to return
false if the constraint about friend declarations and default arguments
is not met (unless we’re in permissive mode [1]), and updates
duplicate_decls to check the return value.
Successfully tested on x86_64-pc-linux-gnu. OK for trunk?
Thanks, Simon
[1] It means that the PR case will still ICE under -fpermissive;
assuming that this patch is accepted, I’ll open a P5 PR to fix this
and reference it in the commit message when I merge.From 3de87095af7a08bbd1d61071744cf6c4c38e52fb Mon Sep 17 00:00:00 2001
From: Simon Martin <si...@nasilyan.com>
Date: Fri, 31 Jan 2025 13:38:40 +0100
Subject: [PATCH] c++: Don't merge friend declarations that specify default
arguments [PR118319]
We segfault upon the following invalid code
=== cut here ===
template <int> struct S {
friend void foo (int a = []{}());
};
void foo (int a) {}
int main () {
S<0> t;
foo ();
}
=== cut here ===
When instantiating S, we instantiate the friend function, and merge its
default arguments (without instantiating them) into the definition of
foo. When resolving the call to foo with no argument, we try to convert
the LAMBDA_EXPR to int, and crash while accessing its NULL_TREE
TREE_TYPE (TREE_TYPE (...)).
This patch fixes this by making sure that if two declarations do not
meet the final part of C++17 11.3.6/4 about friend declarations and
default arguments, duplicate_decls considers them as unrelated, unless
in permissive mode (it means that the initial test case will still ICE
in that mode, a very low priority issue for which I'll open a PR that I
will reference in the commit message when I merge this patch).
Successfully tested on x86_64-pc-linux-gnu.
PR c++/118319
gcc/cp/ChangeLog:
* decl.cc (check_no_redeclaration_friend_default_args): Unless
in permissive mode, return false if the friend declarations
don't meet the constraint about default arguments.
(duplicate_decls): Don't merge declarations if
check_no_redeclaration_friend_default_args returns false.
gcc/testsuite/ChangeLog:
* g++.dg/other/friend13.C: Adjust test expectation.
* g++.dg/parse/defarg18.C: New test.
* g++.dg/parse/defarg19.C: New test.
---
gcc/cp/decl.cc | 20 ++++++---
gcc/testsuite/g++.dg/other/friend13.C | 2 +-
gcc/testsuite/g++.dg/parse/defarg18.C | 65 +++++++++++++++++++++++++++
gcc/testsuite/g++.dg/parse/defarg19.C | 16 +++++++
4 files changed, 95 insertions(+), 8 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/parse/defarg18.C
create mode 100644 gcc/testsuite/g++.dg/parse/defarg19.C
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index cf5e055e146..b706b1bd24d 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -1521,13 +1521,15 @@ check_redeclaration_no_default_args (tree decl)
to enforce the final part of C++17 11.3.6/4, about a single declaration:
"If a friend declaration specifies a default argument expression, that
declaration shall be a definition and shall be the only declaration of
- the function or function template in the translation unit." */
+ the function or function template in the translation unit."
+ It returns false if this constraint is not met and we're not in permissive
+ mode. */
-static void
+static bool
check_no_redeclaration_friend_default_args (tree olddecl, tree newdecl)
{
if (!DECL_UNIQUE_FRIEND_P (olddecl) && !DECL_UNIQUE_FRIEND_P (newdecl))
- return;
+ return true;
for (tree t1 = FUNCTION_FIRST_USER_PARMTYPE (olddecl),
t2 = FUNCTION_FIRST_USER_PARMTYPE (newdecl);
@@ -1542,8 +1544,10 @@ check_no_redeclaration_friend_default_args (tree
olddecl, tree newdecl)
"arguments and isn%'t the only declaration", newdecl))
inform (DECL_SOURCE_LOCATION (olddecl),
"previous declaration of %q#D", olddecl);
- return;
+ if (!flag_permissive)
+ return false;
}
+ return true;
}
/* Merge tree bits that correspond to attributes noreturn, nothrow,
@@ -2271,7 +2275,8 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding,
bool was_hidden)
argument expression, that declaration... shall be the only
declaration of the function or function template in the
translation unit." */
- check_no_redeclaration_friend_default_args (olddecl, newdecl);
+ if (!check_no_redeclaration_friend_default_args (olddecl,
newdecl))
+ return NULL_TREE;
}
}
}
@@ -2495,8 +2500,9 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding,
bool was_hidden)
argument expression, that declaration... shall be the only
declaration of the function or function template in the
translation unit." */
- check_no_redeclaration_friend_default_args
- (old_result, new_result);
+ if (!check_no_redeclaration_friend_default_args
+ (old_result, new_result))
+ return NULL_TREE;
tree new_parms = DECL_INNERMOST_TEMPLATE_PARMS (newdecl);
tree old_parms = DECL_INNERMOST_TEMPLATE_PARMS (olddecl);
diff --git a/gcc/testsuite/g++.dg/other/friend13.C
b/gcc/testsuite/g++.dg/other/friend13.C
index 6cdb322f1df..f59a267e218 100644
--- a/gcc/testsuite/g++.dg/other/friend13.C
+++ b/gcc/testsuite/g++.dg/other/friend13.C
@@ -2,5 +2,5 @@
void f(int, int, int=0); // { dg-message "6:previous" }
class C {
- friend void f(int, int=0, int) {} // { dg-error "15:friend declaration" }
+ friend void f(int, int=0, int) {} // { dg-error "15:friend
declaration|missing" }
};
diff --git a/gcc/testsuite/g++.dg/parse/defarg18.C
b/gcc/testsuite/g++.dg/parse/defarg18.C
new file mode 100644
index 00000000000..689cd79fce4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/defarg18.C
@@ -0,0 +1,65 @@
+// PR c++/118319
+// { dg-do "compile" { target c++11 } }
+
+// Template case, that used to crash.
+
+template <int>
+struct S {
+ friend void foo1 (int a = []{}()); // { dg-error "a definition|only
declaration" }
+ friend void foo2 (int a, int b);
+ friend void foo3 (int a, // { dg-error "a definition|only
declaration" }
+ int b = []{}(),
+ int c = []{}());
+};
+
+void foo1 (int a) {}
+void foo2 (int a, int b = 42) {}
+void foo3 (int a, int b, int c) {}
+
+void hello () {
+ S<0> t;
+ foo1 (); // { dg-error "too few" }
+ foo3 (1, 2); // { dg-error "too few" }
+}
+
+
+// Template case involving a template friend function, that already worked.
+
+template <int>
+struct T {
+ template <class A> friend void foofoo (A a = []{}()); // { dg-error "a
definition" }
+};
+
+template<int> void foofoo (int a) {}
+
+void bonjour () {
+ T<42> t;
+ foofoo(); // { dg-error "no matching" }
+}
+
+
+// Template case, that already worked.
+
+template <int>
+struct U {
+ friend void bar (int a = []{}()); // { dg-error "a definition" }
+};
+
+void hallo () {
+ U<0> u;
+ bar (); // { dg-error "not declared" }
+}
+
+
+// Non template case, that already worked.
+
+struct NoTemplate {
+ friend void baz (int a = []{}()); // { dg-error "a definition|could not
convert" }
+};
+
+void baz (int a) {} // { dg-error "friend declaration" }
+
+void ola () {
+ NoTemplate t;
+ baz (); // { dg-error "too few" }
+}
diff --git a/gcc/testsuite/g++.dg/parse/defarg19.C
b/gcc/testsuite/g++.dg/parse/defarg19.C
new file mode 100644
index 00000000000..23fd2d82fdb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/defarg19.C
@@ -0,0 +1,16 @@
+// PR c++/118319 - Make sure that we don't start rejecting cases that are
+// "valid" in -fpermissive mode
+// { dg-do "run" { target c++11 } }
+// { dg-additional-options "-fpermissive" }
+
+template <int>
+struct S {
+ friend int foo1 (int a = 1); // { dg-warning "specifies default|only
declaration" }
+};
+
+int foo1 (int a) { return a; }
+
+int main () {
+ S<0> t;
+ return 1 - foo1 ();
+}
--
2.44.0
