| Message ID | 010201921e881e69-d4d3c52e-d003-4852-b882-cf33518b4af5-000000@eu-west-1.amazonses.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] c++: Don't crash when mangling member with anonymous union or template types [PR100632, PR109790] | expand |
On 9/23/24 12:58 PM, Simon Martin wrote: > Hi Jason, > > On 20 Sep 2024, at 18:06, Jason Merrill wrote: > >> 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. > Duh you’re right; amended in the latest revision of the patch. >> >>> @@ -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. > Thanks for pointing this out, fixed in the attached revision, > successfully tested on x86_64-pc-linux-gnu. OK for trunk? OK. Jason
diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc index 46dc6923add..17988d69e1e 100644 --- a/gcc/cp/mangle.cc +++ b/gcc/cp/mangle.cc @@ -3255,7 +3255,13 @@ 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 (16)) + write_string ("on"); + } 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..b71a302d5eb --- /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 15 (the highest +// usable in GCC 10) and check that the mangling (actually wrong; see +// decltyp83a.C) matches that of GCC 10's default ABI version (14). + +// { dg-do compile { target c++11 } } +// { dg-additional-options "-fabi-version=15" } + +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..b6a2056724f --- /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 15 (the highest +// usable in GCC 10) and check that the mangling (actually wrong; see +// lambda-ice3a.C) matches that of GCC 10's default ABI version (14). + +// { dg-do compile { target c++14 } } +// { dg-additional-options "-fabi-version=15" } + +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}>();