| 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 |
On 9/16/24 4:07 PM, Simon Martin wrote: > 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 >>> v14-15) 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”. We can add the missing "on" in v16, since previous releases of v16-19 ICEd on that case. > @@ -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))); This last else is redundant; checking DECL_OVERLOADED_OPERATOR_P already asserts that it's an identifier. Jason
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}>();