| Message ID | 01020191e5f95f25-ce2839c1-94cf-4be4-9963-612271b0a468-000000@eu-west-1.amazonses.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] c++: Don't crash when mangling member with anonymous union or template types [PR100632, PR109790] | expand |
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. How about checking !DECL_NAME (member) instead of !identifier_p? > This patch fixes this by relaxing the assert to accept members that are > not identifiers, that are handled by write_unqualified_name just fine.
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. 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. Successfully tested on x86_64-pc-linux-gnu. OK? Thanks again, Simon > >> This patch fixes this by relaxing the assert to accept members that >> are >> not identifiers, that are handled by write_unqualified_name just >> fine. From c5985057d6769ae2d54ebffd703d5508624479dd Mon Sep 17 00:00:00 2001 From: Simon Martin <simon@nasilyan.com> Date: Mon, 9 Sep 2024 09:31:10 +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 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, is a legitimate template declaration for an operator This assert was added via r11-6301, to be sure that we do write the "on" marker for operator members. This patch refactors write_member_name to add this marker whenever it's needed (including cases where it was missing; see decltype83.C), and removes the assert altogether since it's not needed anymore. Successfully tested on x86_64-pc-linux-gnu. PR c++/109790 PR c++/100632 gcc/cp/ChangeLog: * mangle.cc (write_member_name): Add "on" marker for all operators. Remove now unnecessary gcc_assert. gcc/testsuite/ChangeLog: * g++.dg/cpp0x/decltype83.C: New test. * g++.dg/cpp1y/lambda-ice3.C: New test. * g++.dg/cpp2a/nontype-class67.C: New test. --- gcc/cp/mangle.cc | 23 ++++++++++---------- gcc/testsuite/g++.dg/cpp0x/decltype83.C | 18 +++++++++++++++ gcc/testsuite/g++.dg/cpp1y/lambda-ice3.C | 12 ++++++++++ gcc/testsuite/g++.dg/cpp2a/nontype-class67.C | 9 ++++++++ 4 files changed, 51 insertions(+), 11 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/decltype83.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/lambda-ice3.C create mode 100644 gcc/testsuite/g++.dg/cpp2a/nontype-class67.C diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc index 46dc6923add..d2548d69991 100644 --- a/gcc/cp/mangle.cc +++ b/gcc/cp/mangle.cc @@ -3244,20 +3244,21 @@ write_template_args (tree args, tree parms /*= NULL_TREE*/) static void write_member_name (tree member) { - if (identifier_p (member)) + /* Make sure to add the "on" marker if MEMBER is an operator. */ + if ((identifier_p (member) || DECL_DECLARES_FUNCTION_P (member)) + && abi_check (11)) { - if (IDENTIFIER_ANY_OP_P (member)) - { - if (abi_check (11)) - write_string ("on"); - } - write_unqualified_id (member); + tree name = member; + if (DECL_DECLARES_FUNCTION_P (name)) + name = DECL_NAME (name); + if (IDENTIFIER_ANY_OP_P (name)) + write_string ("on"); } + + if (identifier_p (member)) + write_unqualified_id (member); else if (DECL_P (member)) - { - gcc_assert (!DECL_OVERLOADED_OPERATOR_P (member)); - write_unqualified_name (member); - } + write_unqualified_name (member); else if (TREE_CODE (member) == TEMPLATE_ID_EXPR) { tree name = TREE_OPERAND (member, 0); diff --git a/gcc/testsuite/g++.dg/cpp0x/decltype83.C b/gcc/testsuite/g++.dg/cpp0x/decltype83.C new file mode 100644 index 00000000000..75e9c081b27 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/decltype83.C @@ -0,0 +1,18 @@ +// PR c++/109790 +// { dg-do compile { target c++11 } } +// This would ICE due to an assert, and if the assert were removed, it'd still +// be wrongly mangling "decltype (&(A::operator+<int>)) f<int>()": the "on" +// marker for operators would be missing. + +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..49261aaabb7 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/lambda-ice3.C @@ -0,0 +1,12 @@ +// PR c++/109790 +// { 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>(); +} 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}>();
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? > 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. Jason
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”). Thanks, Simon
diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc index 46dc6923add..a63ae9f7ac6 100644 --- a/gcc/cp/mangle.cc +++ b/gcc/cp/mangle.cc @@ -3255,7 +3255,8 @@ write_member_name (tree member) } else if (DECL_P (member)) { - gcc_assert (!DECL_OVERLOADED_OPERATOR_P (member)); + gcc_assert (!identifier_p (member) + || !DECL_OVERLOADED_OPERATOR_P (member)); write_unqualified_name (member); } else if (TREE_CODE (member) == TEMPLATE_ID_EXPR) diff --git a/gcc/testsuite/g++.dg/cpp0x/decltype83.C b/gcc/testsuite/g++.dg/cpp0x/decltype83.C new file mode 100644 index 00000000000..db104f333aa --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/decltype83.C @@ -0,0 +1,13 @@ +// PR c++/109790 +// { 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>(); +} 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..49261aaabb7 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/lambda-ice3.C @@ -0,0 +1,12 @@ +// PR c++/109790 +// { 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>(); +} 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}>();