| Message ID | 20190208172136.GL3884@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | C++ PATCH for c++/89212 - ICE converting nullptr to pointer-to-member-function | expand |
On 2/8/19 12:21 PM, Marek Polacek wrote: > r256999 removed early bailout for pointer-to-member-function types, so we > now try to tsubst each element of a pointer-to-member-function CONSTRUCTOR. > > That's fine but the problem here is that we end up converting a null pointer > to pointer-to-member-function type and that crashes in fold_convert: > > 16035 case INTEGER_CST: > 16039 { > 16040 /* Instantiate any typedefs in the type. */ > 16041 tree type = tsubst (TREE_TYPE (t), args, complain, in_decl); > 16042 r = fold_convert (type, t); > > It seems obvious to use cp_fold_convert which handles TYPE_PTRMEM_P, but > that then ICEs too, the infamous "canonical types differ for identical types": > type is > > struct > { > void A::<T344> (struct A *) * __pfn; > long int __delta; > } > > and the type of "0" is "void A::<T344> (struct A *) *". These types are > structurally equivalent but have different canonical types. (What's up > with that, anyway? It seems OK that the canonical type of the struct is > the struct itself and that the canonical type of the pointer is the pointer > itself.) > > That could be handled in cp_fold_convert: add code to convert an integer_zerop to > TYPE_PTRMEMFUNC_P. Unfortunately the 0 is not null_ptr_cst_p because it's got > a pointer type. > > Or just don't bother substituting null_member_pointer_value_p and avoid the > above. > > Bootstrapped/regtested on x86_64-linux, ok for trunk and 8? > > 2019-02-08 Marek Polacek <polacek@redhat.com> > > PR c++/89212 - ICE converting nullptr to pointer-to-member-function. > * pt.c (tsubst_copy_and_build) <case CONSTRUCTOR>: Return early for > null member pointer value. > > * g++.dg/cpp0x/nullptr40.C: New test. > > diff --git gcc/cp/pt.c gcc/cp/pt.c > index b8fbf4046f0..acc2d8f1feb 100644 > --- gcc/cp/pt.c > +++ gcc/cp/pt.c > @@ -19251,6 +19251,9 @@ tsubst_copy_and_build (tree t, > looked up by digest_init. */ > process_index_p = !(type && MAYBE_CLASS_TYPE_P (type)); > > + if (null_member_pointer_value_p (t)) > + RETURN (t); I would expect this to do the wrong thing if type is different from TREE_TYPE (t). Can we get here for a dependent PMF type like T (A::*)()? If not, let's assert that they're the same. Otherwise, maybe cp_convert (type, nullptr_node)? Jason
On Fri, Feb 08, 2019 at 05:37:00PM -0500, Jason Merrill wrote: > On 2/8/19 12:21 PM, Marek Polacek wrote: > > r256999 removed early bailout for pointer-to-member-function types, so we > > now try to tsubst each element of a pointer-to-member-function CONSTRUCTOR. > > > > That's fine but the problem here is that we end up converting a null pointer > > to pointer-to-member-function type and that crashes in fold_convert: > > > > 16035 case INTEGER_CST: > > 16039 { > > 16040 /* Instantiate any typedefs in the type. */ > > 16041 tree type = tsubst (TREE_TYPE (t), args, complain, in_decl); > > 16042 r = fold_convert (type, t); > > > > It seems obvious to use cp_fold_convert which handles TYPE_PTRMEM_P, but > > that then ICEs too, the infamous "canonical types differ for identical types": > > type is > > > > struct > > { > > void A::<T344> (struct A *) * __pfn; > > long int __delta; > > } > > > > and the type of "0" is "void A::<T344> (struct A *) *". These types are > > structurally equivalent but have different canonical types. (What's up > > with that, anyway? It seems OK that the canonical type of the struct is > > the struct itself and that the canonical type of the pointer is the pointer > > itself.) > > > > That could be handled in cp_fold_convert: add code to convert an integer_zerop to > > TYPE_PTRMEMFUNC_P. Unfortunately the 0 is not null_ptr_cst_p because it's got > > a pointer type. > > > > Or just don't bother substituting null_member_pointer_value_p and avoid the > > above. > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk and 8? > > > > 2019-02-08 Marek Polacek <polacek@redhat.com> > > > > PR c++/89212 - ICE converting nullptr to pointer-to-member-function. > > * pt.c (tsubst_copy_and_build) <case CONSTRUCTOR>: Return early for > > null member pointer value. > > > > * g++.dg/cpp0x/nullptr40.C: New test. > > > > diff --git gcc/cp/pt.c gcc/cp/pt.c > > index b8fbf4046f0..acc2d8f1feb 100644 > > --- gcc/cp/pt.c > > +++ gcc/cp/pt.c > > @@ -19251,6 +19251,9 @@ tsubst_copy_and_build (tree t, > > looked up by digest_init. */ > > process_index_p = !(type && MAYBE_CLASS_TYPE_P (type)); > > + if (null_member_pointer_value_p (t)) > > + RETURN (t); > > I would expect this to do the wrong thing if type is different from > TREE_TYPE (t). Can we get here for a dependent PMF type like T (A::*)()? > If not, let's assert that they're the same. Otherwise, maybe cp_convert > (type, nullptr_node)? Yup, that's a concern. But I'm not seeing any ICEs with the assert added and a dependent PMF as in the new testcase. And it seems we get a conversion error if the types of the PMFs don't match. If I'm wrong, this would be easy to fix anyway. Bootstrapped/regtested on x86_64-linux, ok for trunk? 2019-02-11 Marek Polacek <polacek@redhat.com> PR c++/89212 - ICE converting nullptr to pointer-to-member-function. * pt.c (tsubst_copy_and_build) <case CONSTRUCTOR>: Return early for null member pointer value. * g++.dg/cpp0x/nullptr40.C: New test. * g++.dg/cpp0x/nullptr41.C: New test. diff --git gcc/cp/pt.c gcc/cp/pt.c index b8fbf4046f0..2682c68dcfa 100644 --- gcc/cp/pt.c +++ gcc/cp/pt.c @@ -19251,6 +19251,12 @@ tsubst_copy_and_build (tree t, looked up by digest_init. */ process_index_p = !(type && MAYBE_CLASS_TYPE_P (type)); + if (null_member_pointer_value_p (t)) + { + gcc_assert (same_type_p (type, TREE_TYPE (t))); + RETURN (t); + } + n = vec_safe_copy (CONSTRUCTOR_ELTS (t)); newlen = vec_safe_length (n); FOR_EACH_VEC_SAFE_ELT (n, idx, ce) diff --git gcc/testsuite/g++.dg/cpp0x/nullptr40.C gcc/testsuite/g++.dg/cpp0x/nullptr40.C new file mode 100644 index 00000000000..21c188bdb5e --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/nullptr40.C @@ -0,0 +1,19 @@ +// PR c++/89212 +// { dg-do compile { target c++11 } } + +template <int, typename T> using enable_if_t = int; + +template<class X, void(X::*foo)() = nullptr> +struct p +{ + template<void(X::*fun)() = foo, typename T = enable_if_t<nullptr == fun, int>> + p(T) { } + p() = default; +}; + +struct A +{ + p<A> i = 1; + void bar(); + p<A, &A::bar> j; +}; diff --git gcc/testsuite/g++.dg/cpp0x/nullptr41.C gcc/testsuite/g++.dg/cpp0x/nullptr41.C new file mode 100644 index 00000000000..54e66af2095 --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/nullptr41.C @@ -0,0 +1,19 @@ +// PR c++/89212 +// { dg-do compile { target c++11 } } + +template <int, typename T> using enable_if_t = int; + +template<typename U, typename W, typename Y, class X, W(X::*foo)() = nullptr> +struct p +{ + template<U(Y::*fun)() = foo, typename T = enable_if_t<nullptr == fun, int>> + p(T) { } + p() = default; +}; + +struct A +{ + p<void, void, A, A> i = 1; + void bar(); + p<void, void, A, A, &A::bar> j; +};
On 2/11/19 2:21 PM, Marek Polacek wrote: > On Fri, Feb 08, 2019 at 05:37:00PM -0500, Jason Merrill wrote: >> On 2/8/19 12:21 PM, Marek Polacek wrote: >>> r256999 removed early bailout for pointer-to-member-function types, so we >>> now try to tsubst each element of a pointer-to-member-function CONSTRUCTOR. >>> >>> That's fine but the problem here is that we end up converting a null pointer >>> to pointer-to-member-function type and that crashes in fold_convert: >>> >>> 16035 case INTEGER_CST: >>> 16039 { >>> 16040 /* Instantiate any typedefs in the type. */ >>> 16041 tree type = tsubst (TREE_TYPE (t), args, complain, in_decl); >>> 16042 r = fold_convert (type, t); >>> >>> It seems obvious to use cp_fold_convert which handles TYPE_PTRMEM_P, but >>> that then ICEs too, the infamous "canonical types differ for identical types": >>> type is >>> >>> struct >>> { >>> void A::<T344> (struct A *) * __pfn; >>> long int __delta; >>> } >>> >>> and the type of "0" is "void A::<T344> (struct A *) *". These types are >>> structurally equivalent but have different canonical types. (What's up >>> with that, anyway? It seems OK that the canonical type of the struct is >>> the struct itself and that the canonical type of the pointer is the pointer >>> itself.) >>> >>> That could be handled in cp_fold_convert: add code to convert an integer_zerop to >>> TYPE_PTRMEMFUNC_P. Unfortunately the 0 is not null_ptr_cst_p because it's got >>> a pointer type. >>> >>> Or just don't bother substituting null_member_pointer_value_p and avoid the >>> above. >>> >>> Bootstrapped/regtested on x86_64-linux, ok for trunk and 8? >>> >>> 2019-02-08 Marek Polacek <polacek@redhat.com> >>> >>> PR c++/89212 - ICE converting nullptr to pointer-to-member-function. >>> * pt.c (tsubst_copy_and_build) <case CONSTRUCTOR>: Return early for >>> null member pointer value. >>> >>> * g++.dg/cpp0x/nullptr40.C: New test. >>> >>> diff --git gcc/cp/pt.c gcc/cp/pt.c >>> index b8fbf4046f0..acc2d8f1feb 100644 >>> --- gcc/cp/pt.c >>> +++ gcc/cp/pt.c >>> @@ -19251,6 +19251,9 @@ tsubst_copy_and_build (tree t, >>> looked up by digest_init. */ >>> process_index_p = !(type && MAYBE_CLASS_TYPE_P (type)); >>> + if (null_member_pointer_value_p (t)) >>> + RETURN (t); >> >> I would expect this to do the wrong thing if type is different from >> TREE_TYPE (t). Can we get here for a dependent PMF type like T (A::*)()? >> If not, let's assert that they're the same. Otherwise, maybe cp_convert >> (type, nullptr_node)? > > Yup, that's a concern. But I'm not seeing any ICEs with the assert added and a > dependent PMF as in the new testcase. And it seems we get a conversion error > if the types of the PMFs don't match. If I'm wrong, this would be easy to > fix anyway. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? OK. Jason
diff --git gcc/cp/pt.c gcc/cp/pt.c index b8fbf4046f0..acc2d8f1feb 100644 --- gcc/cp/pt.c +++ gcc/cp/pt.c @@ -19251,6 +19251,9 @@ tsubst_copy_and_build (tree t, looked up by digest_init. */ process_index_p = !(type && MAYBE_CLASS_TYPE_P (type)); + if (null_member_pointer_value_p (t)) + RETURN (t); + n = vec_safe_copy (CONSTRUCTOR_ELTS (t)); newlen = vec_safe_length (n); FOR_EACH_VEC_SAFE_ELT (n, idx, ce) diff --git gcc/testsuite/g++.dg/cpp0x/nullptr40.C gcc/testsuite/g++.dg/cpp0x/nullptr40.C new file mode 100644 index 00000000000..21c188bdb5e --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/nullptr40.C @@ -0,0 +1,19 @@ +// PR c++/89212 +// { dg-do compile { target c++11 } } + +template <int, typename T> using enable_if_t = int; + +template<class X, void(X::*foo)() = nullptr> +struct p +{ + template<void(X::*fun)() = foo, typename T = enable_if_t<nullptr == fun, int>> + p(T) { } + p() = default; +}; + +struct A +{ + p<A> i = 1; + void bar(); + p<A, &A::bar> j; +};
r256999 removed early bailout for pointer-to-member-function types, so we now try to tsubst each element of a pointer-to-member-function CONSTRUCTOR. That's fine but the problem here is that we end up converting a null pointer to pointer-to-member-function type and that crashes in fold_convert: 16035 case INTEGER_CST: 16039 { 16040 /* Instantiate any typedefs in the type. */ 16041 tree type = tsubst (TREE_TYPE (t), args, complain, in_decl); 16042 r = fold_convert (type, t); It seems obvious to use cp_fold_convert which handles TYPE_PTRMEM_P, but that then ICEs too, the infamous "canonical types differ for identical types": type is struct { void A::<T344> (struct A *) * __pfn; long int __delta; } and the type of "0" is "void A::<T344> (struct A *) *". These types are structurally equivalent but have different canonical types. (What's up with that, anyway? It seems OK that the canonical type of the struct is the struct itself and that the canonical type of the pointer is the pointer itself.) That could be handled in cp_fold_convert: add code to convert an integer_zerop to TYPE_PTRMEMFUNC_P. Unfortunately the 0 is not null_ptr_cst_p because it's got a pointer type. Or just don't bother substituting null_member_pointer_value_p and avoid the above. Bootstrapped/regtested on x86_64-linux, ok for trunk and 8? 2019-02-08 Marek Polacek <polacek@redhat.com> PR c++/89212 - ICE converting nullptr to pointer-to-member-function. * pt.c (tsubst_copy_and_build) <case CONSTRUCTOR>: Return early for null member pointer value. * g++.dg/cpp0x/nullptr40.C: New test.