diff mbox series

[v3] c++: Don't crash when mangling member with anonymous union or template types [PR100632, PR109790]

Message ID 01020191fb28da70-3603abf7-c078-4d44-a9c4-a13085850bc4-000000@eu-west-1.amazonses.com
State New
Headers show
Series [v3] c++: Don't crash when mangling member with anonymous union or template types [PR100632, PR109790] | expand

Commit Message

Simon Martin Sept. 16, 2024, 2:07 p.m. UTC 
Hi Jason,

On 14 Sep 2024, at 18:44, Simon Martin wrote:

> Hi Jason,
>
> On 14 Sep 2024, at 18:11, Jason Merrill wrote:
>
>> On 9/13/24 11:06 AM, Simon Martin wrote:
>>> Hi Jason,
>>>
>>> On 12 Sep 2024, at 16:48, Jason Merrill wrote:
>>>
>>>> On 9/12/24 7:23 AM, Simon Martin wrote:
>>>>> Hi,
>>>>>
>>>>> While looking at more open PRs, I have discovered that the problem

>>>>> reported in PR109790 is very similar to that in PR100632, so I’m
>>>>> combining both in a single patch attached here. The fix is similar

>>>>> to
>>>>> the one I initially submitted, only more general and I believe
>>>>> better.
>>>>
>>>>> We currently crash upon mangling members that have an anonymous
>>>>> union
>>>>> or a template type.
>>>>>
>>>>> The problem is that before calling write_unqualified_name,
>>>>> write_member_name has an assert that assumes that it has an
>>>>> IDENTIFIER_NODE in its hand. However it's incorrect: it has an
>>>>> anonymous union in PR100632, and a template in PR109790.
>>>>
>>>> The assert does not assume it has an IDENTIFIER_NODE; it assumes it

>>>> has a _DECL, and expects its DECL_NAME to be an IDENTIFIER_NODE.
>>>>
>>>> !identifier_p will always be true for a _DECL, making the assert
>>>> useless.
>>> Indeed, my bad. Thanks for catching and explaining this!
>>>>
>>>> How about checking !DECL_NAME (member) instead of !identifier_p?
>>> Unfortunately it does not fix PR100632, that actually involves
>>> legitimate operators.
>>>
>>> I checked why the assert was added in the first place (via r11-6301),
>>>
>>> and the idea was to catch any case where we’d be missing the

>>> “on”
>>> marker - PR100632 contains such cases.
>>
>> I assume you mean 109790?
> Yes :-/
>>
>>> So I took the approach to refactor write_member_name a bit to first
>>> write the marker in all the cases required, and then actually write
>>> the
>>> member name; and the assert is not needed anymore there.
>>
>> Refactoring code in mangle.cc is tricky given the intent to retain
>> backward bug-compatibility.
>>
>> Specifically, adding the "on" in ABI v11 is wrong since GCC 10 (ABI
>> v10) didn't emit it for the 109790 testcase; we can add it for v16,
>> since GCC 11 ICEd on the testcase.
>>
>> I would prefer to fix the bug locally rather than refactor.
> Understood, that makes sense.
>
> I’ll work on a more local patch and resubmit (by the way you can also
> ignore
> https://gcc.gnu.org/pipermail/gcc-patches/2024-September/662496.html,
> that is also “too wide”).
I’m attaching a revised version of the patch, that lets members
with an anonymous union type go through, and for operators, introduces
a new ABI version under which it adds the missing "on”.

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

Thanks, Simon
From c51f080a1844a5330f785c2c529ae5afc9e611b8 Mon Sep 17 00:00:00 2001
From: Simon Martin <simon@nasilyan.com>
Date: Mon, 16 Sep 2024 13:45:32 +0200
Subject: [PATCH] c++: Don't crash when mangling member with anonymous union or template type [PR100632, PR109790]

We currently crash upon mangling members that have an anonymous union or
a template operator type.

The problem is that before calling write_unqualified_name,
write_member_name asserts that it has a declaration whose DECL_NAME is
an identifier node that is not that of an operator. This is wrong:
 - In PR100632, it's an anonymous union declaration, hence a 0 DECL_NAME
 - In PR109790, it's a legitimate template declaration for an operator
   (this was accepted up to GCC 10)

This assert was added via r11-6301, to be sure that we do write the "on"
marker for operator members.

This patch removes that assert and instead
 - Lets members with an anonymous union type go through.
 - For operators,
   - Introduces a new ABI version, and under it, adds the missing "on"
     marker.
   - Adds tests checking the mangled name both for the previous ABI
     version (checking it's what we got with GCC 10) and the new one.

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/109790
	PR c++/100632

gcc/c-family/ChangeLog:

	* c-opts.cc (c_common_post_options): Bump latest_abi_version.

gcc/ChangeLog:

	* common.opt: Document ABI version 20.

gcc/cp/ChangeLog:

	* mangle.cc (write_member_name): Handle members whose type is an
	anonymous union member. Write missing "on" marker for operators
	when ABI version is at least 20.

gcc/testsuite/ChangeLog:

	* g++.dg/abi/macro0.C: Adjust due to latest_abi_version bump.
	* g++.dg/cpp0x/decltype83.C: New test.
	* g++.dg/cpp0x/decltype83a.C: New test.
	* g++.dg/cpp1y/lambda-ice3.C: New test.
	* g++.dg/cpp1y/lambda-ice3a.C: New test.
	* g++.dg/cpp2a/nontype-class67.C: New test.

---
 gcc/c-family/c-opts.cc                       |  2 +-
 gcc/common.opt                               |  3 +++
 gcc/cp/mangle.cc                             | 10 +++++++++-
 gcc/testsuite/g++.dg/abi/macro0.C            |  2 +-
 gcc/testsuite/g++.dg/cpp0x/decltype83.C      | 20 ++++++++++++++++++++
 gcc/testsuite/g++.dg/cpp0x/decltype83a.C     | 18 ++++++++++++++++++
 gcc/testsuite/g++.dg/cpp1y/lambda-ice3.C     | 19 +++++++++++++++++++
 gcc/testsuite/g++.dg/cpp1y/lambda-ice3a.C    | 17 +++++++++++++++++
 gcc/testsuite/g++.dg/cpp2a/nontype-class67.C |  9 +++++++++
 9 files changed, 97 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/decltype83.C
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/decltype83a.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/lambda-ice3.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/lambda-ice3a.C
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/nontype-class67.C
diff mbox series

Patch

diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc
index bd397ff6407..1e75d47d461 100644
--- a/gcc/c-family/c-opts.cc
+++ b/gcc/c-family/c-opts.cc
@@ -1031,7 +1031,7 @@  c_common_post_options (const char **pfilename)
 
   /* Change flag_abi_version to be the actual current ABI level, for the
      benefit of c_cpp_builtins, and to make comparison simpler.  */
-  const int latest_abi_version = 19;
+  const int latest_abi_version = 20;
   /* Generate compatibility aliases for ABI v13 (8.2) by default.  */
   const int abi_compat_default = 13;
 
diff --git a/gcc/common.opt b/gcc/common.opt
index d270e524ff4..ccfd81551ef 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1034,6 +1034,9 @@  Driver Undocumented
 ;     Mangles constraints on function templates.
 ;     Default in G++ 14.
 ;
+; 20: Adds missing 'on' in mangling of operator names in some cases.
+;     Default in G++ 15.
+;
 ; Additional positive integers will be assigned as new versions of
 ; the ABI become the default version of the ABI.
 fabi-version=
diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc
index 46dc6923add..bbe5056599a 100644
--- a/gcc/cp/mangle.cc
+++ b/gcc/cp/mangle.cc
@@ -3255,7 +3255,15 @@  write_member_name (tree member)
     }
   else if (DECL_P (member))
     {
-      gcc_assert (!DECL_OVERLOADED_OPERATOR_P (member));
+      if (ANON_AGGR_TYPE_P (TREE_TYPE (member)))
+	;
+      else if (DECL_OVERLOADED_OPERATOR_P (member))
+	{
+	  if (abi_check (20))
+	    write_string ("on");
+	}
+      else
+	gcc_assert (identifier_p (DECL_NAME (member)));
       write_unqualified_name (member);
     }
   else if (TREE_CODE (member) == TEMPLATE_ID_EXPR)
diff --git a/gcc/testsuite/g++.dg/abi/macro0.C b/gcc/testsuite/g++.dg/abi/macro0.C
index 183184e0f0a..f6a57c11ae7 100644
--- a/gcc/testsuite/g++.dg/abi/macro0.C
+++ b/gcc/testsuite/g++.dg/abi/macro0.C
@@ -1,6 +1,6 @@ 
 // This testcase will need to be kept in sync with c_common_post_options.
 // { dg-options "-fabi-version=0" }
 
-#if __GXX_ABI_VERSION != 1019
+#if __GXX_ABI_VERSION != 1020
 #error "Incorrect value of __GXX_ABI_VERSION"
 #endif
diff --git a/gcc/testsuite/g++.dg/cpp0x/decltype83.C b/gcc/testsuite/g++.dg/cpp0x/decltype83.C
new file mode 100644
index 00000000000..a362cc91ae2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/decltype83.C
@@ -0,0 +1,20 @@ 
+// PR c++/109790
+// This used to work until GCC 10; force the usage of ABI 19 (in use when the
+// regression was fixed) and assert that the mangling (actually wrong; see 
+// decltype83a.C) matches that of GCC 10.
+
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-fabi-version=19" }
+
+struct A {
+  template<class T> void operator+(T);
+};
+
+template<class T>
+decltype(&A::operator+<T>) f();
+
+int main() {
+  f<int>();
+}
+
+// { dg-final { scan-assembler "_Z1fIiEDTadsr1AplIT_EEv" } }
diff --git a/gcc/testsuite/g++.dg/cpp0x/decltype83a.C b/gcc/testsuite/g++.dg/cpp0x/decltype83a.C
new file mode 100644
index 00000000000..27c363651f1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/decltype83a.C
@@ -0,0 +1,18 @@ 
+// PR c++/109790
+// Up to GCC 10, the mangling would be missing the "on" marker, hence be wrong.
+// Check that this is fixed with the latest ABI.
+
+// { dg-do compile { target c++11 } }
+
+struct A {
+  template<class T> void operator+(T);
+};
+
+template<class T>
+decltype(&A::operator+<T>) f();
+
+int main() {
+  f<int>();
+}
+
+// { dg-final { scan-assembler "_Z1fIiEDTadsr1AonplIT_EEv" } } 
diff --git a/gcc/testsuite/g++.dg/cpp1y/lambda-ice3.C b/gcc/testsuite/g++.dg/cpp1y/lambda-ice3.C
new file mode 100644
index 00000000000..49f9c120c2a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/lambda-ice3.C
@@ -0,0 +1,19 @@ 
+// PR c++/109790
+// This used to work until GCC 10; force the usage of ABI 19 (in use when the
+// regression was fixed) and assert that the mangling (actually wrong; see 
+// lambda-ice3a.C) matches that of GCC 10.
+
+// { dg-do compile { target c++14 } }
+// { dg-additional-options "-fabi-version=19" }
+
+auto ll = [](auto ... ){};
+template <class _Impl, class _Args>
+  void mm(void (_Impl::*__p)(_Args) const);
+template <class _Ts>
+using __impl_for = decltype(mm(&decltype(ll)::operator()<_Ts>));
+template <class _Ts> __impl_for<_Ts> f() { }
+void aaa() {
+  f<int>();
+}
+
+// { dg-final { scan-assembler "_Z1fIiEDTcl2mmadsrN2llMUlDpT_E_EclIT_EEEv" } }
diff --git a/gcc/testsuite/g++.dg/cpp1y/lambda-ice3a.C b/gcc/testsuite/g++.dg/cpp1y/lambda-ice3a.C
new file mode 100644
index 00000000000..6394e6c4914
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/lambda-ice3a.C
@@ -0,0 +1,17 @@ 
+// PR c++/109790
+// Up to GCC 10, the mangling would be missing the "on" marker, hence be wrong.
+// Check that this is fixed with the latest ABI.
+
+// { dg-do compile { target c++14 } }
+
+auto ll = [](auto ... ){};
+template <class _Impl, class _Args>
+  void mm(void (_Impl::*__p)(_Args) const);
+template <class _Ts>
+using __impl_for = decltype(mm(&decltype(ll)::operator()<_Ts>));
+template <class _Ts> __impl_for<_Ts> f() { }
+void aaa() {
+  f<int>();
+}
+
+// { dg-final { scan-assembler "_Z1fIiEDTcl2mmadsrN2llMUlDpT_E_EonclIT_EEEv" } } 
diff --git a/gcc/testsuite/g++.dg/cpp2a/nontype-class67.C b/gcc/testsuite/g++.dg/cpp2a/nontype-class67.C
new file mode 100644
index 00000000000..accf4284883
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/nontype-class67.C
@@ -0,0 +1,9 @@ 
+// PR c++/100632
+// { dg-do compile { target c++20 } }
+
+struct B { const int* p; };
+template<B> void f() {}
+
+struct Nested { union { int k; }; } nested;
+
+template void f<B{&nested.k}>();