| Message ID | 20240213164907.3648858-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: implicitly_declare_fn and access checks [PR113908] | expand |
On 2/13/24 11:49, Patrick Palka wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, are one of > both of these fixes OK for trunk? > > -- >8 -- > > Here during ahead of time checking of the non-dependent new-expr we > synthesize B's copy constructor, which should be defined as deleted > due to A's inaccessible copy constructor. But enforce_access incorrectly > decides to defer the (silent) access check for A::A(const A&) during > synthesization since current_template_parms is still set (before r14-557 > it checked processing_template_decl which got cleared from > implicitly_declare_fn), which leads to the access check leaking out to > the template context that needed the synthesization. > > This patch narrowly fixes this regression in two sufficient ways: > > 1. Clear current_template_parms alongside processing_template_decl > in implicitly_declare_fn so that it's more independent of context. Hmm, perhaps it or synthesized_method_walk should use maybe_push_to_top_level? > 2. Don't defer a silent access check when in a template context, > since such deferred checks will be replayed noisily at instantiation > time which may not be what the caller intended. True, but returning a possibly incorrect 'false' is probably also not what the caller intended. It would be better to see that we never call enforce_access with tf_none in a template. If that's not feasible, I think we should still conservatively return true. Jason
On Tue, 13 Feb 2024, Jason Merrill wrote: > On 2/13/24 11:49, Patrick Palka wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, are one of > > both of these fixes OK for trunk? > > > > -- >8 -- > > > > Here during ahead of time checking of the non-dependent new-expr we > > synthesize B's copy constructor, which should be defined as deleted > > due to A's inaccessible copy constructor. But enforce_access incorrectly > > decides to defer the (silent) access check for A::A(const A&) during > > synthesization since current_template_parms is still set (before r14-557 > > it checked processing_template_decl which got cleared from > > implicitly_declare_fn), which leads to the access check leaking out to > > the template context that needed the synthesization. > > > > This patch narrowly fixes this regression in two sufficient ways: > > > > 1. Clear current_template_parms alongside processing_template_decl > > in implicitly_declare_fn so that it's more independent of context. > > Hmm, perhaps it or synthesized_method_walk should use maybe_push_to_top_level? That works nicely, and also fixes the other regression PR113332. There the lambda context triggering synthesization of a default ctor was causing maybe_dummy_object to misbehave during overload resolution of one of its member's default ctors, and now synthesization is context independent. > > > 2. Don't defer a silent access check when in a template context, > > since such deferred checks will be replayed noisily at instantiation > > time which may not be what the caller intended. > > True, but returning a possibly incorrect 'false' is probably also not what the > caller intended. It would be better to see that we never call enforce_access > with tf_none in a template. If that's not feasible, I think we should still > conservatively return true. Makes sense, I can experiment with that enforce_access access change as a follow-up. Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for trunk? -- >8 -- Subject: [PATCH] c++: synthesized_method_walk context independence [PR113908] PR c++/113908 PR c++/113332 gcc/cp/ChangeLog: * method.cc (synthesized_method_walk): Use maybe_push_to_top_level. gcc/testsuite/ChangeLog: * g++.dg/template/non-dependent31.C: New test. * g++.dg/template/non-dependent32.C: New test. --- gcc/cp/method.cc | 2 ++ .../g++.dg/template/non-dependent31.C | 18 +++++++++++++++++ .../g++.dg/template/non-dependent32.C | 20 +++++++++++++++++++ 3 files changed, 40 insertions(+) create mode 100644 gcc/testsuite/g++.dg/template/non-dependent31.C create mode 100644 gcc/testsuite/g++.dg/template/non-dependent32.C diff --git a/gcc/cp/method.cc b/gcc/cp/method.cc index 957496d3e18..98c10e6a8b5 100644 --- a/gcc/cp/method.cc +++ b/gcc/cp/method.cc @@ -2760,6 +2760,7 @@ synthesized_method_walk (tree ctype, special_function_kind sfk, bool const_p, return; } + bool push_to_top = maybe_push_to_top_level (TYPE_NAME (ctype)); ++cp_unevaluated_operand; ++c_inhibit_evaluation_warnings; push_deferring_access_checks (dk_no_deferred); @@ -2857,6 +2858,7 @@ synthesized_method_walk (tree ctype, special_function_kind sfk, bool const_p, pop_deferring_access_checks (); --cp_unevaluated_operand; --c_inhibit_evaluation_warnings; + maybe_pop_from_top_level (push_to_top); } /* DECL is a defaulted function whose exception specification is now diff --git a/gcc/testsuite/g++.dg/template/non-dependent31.C b/gcc/testsuite/g++.dg/template/non-dependent31.C new file mode 100644 index 00000000000..3fa68f40fe1 --- /dev/null +++ b/gcc/testsuite/g++.dg/template/non-dependent31.C @@ -0,0 +1,18 @@ +// PR c++/113908 +// { dg-do compile { target c++11 } } + +struct A { + A(); +private: + A(const A&); +}; + +struct B { + A a; + + template<class T> + static void f() { new B(); } +}; + +template void B::f<int>(); +static_assert(!__is_constructible(B, const B&), ""); diff --git a/gcc/testsuite/g++.dg/template/non-dependent32.C b/gcc/testsuite/g++.dg/template/non-dependent32.C new file mode 100644 index 00000000000..246654c5b50 --- /dev/null +++ b/gcc/testsuite/g++.dg/template/non-dependent32.C @@ -0,0 +1,20 @@ +// PR c++/113332 +// { dg-do compile { target c++11 } } + +struct tuple { + template<class _Tp> + static constexpr bool __is_implicitly_default_constructible() { return true; } + + template<class _Tp = void, + bool = __is_implicitly_default_constructible<_Tp>()> + tuple(); +}; + +struct DBusStruct { +private: + tuple data_; +}; + +struct IBusService { + int m = [] { DBusStruct{}; return 42; }(); +};
On 2/14/24 08:46, Patrick Palka wrote: > On Tue, 13 Feb 2024, Jason Merrill wrote: > >> On 2/13/24 11:49, Patrick Palka wrote: >>> Bootstrapped and regtested on x86_64-pc-linux-gnu, are one of >>> both of these fixes OK for trunk? >>> >>> -- >8 -- >>> >>> Here during ahead of time checking of the non-dependent new-expr we >>> synthesize B's copy constructor, which should be defined as deleted >>> due to A's inaccessible copy constructor. But enforce_access incorrectly >>> decides to defer the (silent) access check for A::A(const A&) during >>> synthesization since current_template_parms is still set (before r14-557 >>> it checked processing_template_decl which got cleared from >>> implicitly_declare_fn), which leads to the access check leaking out to >>> the template context that needed the synthesization. >>> >>> This patch narrowly fixes this regression in two sufficient ways: >>> >>> 1. Clear current_template_parms alongside processing_template_decl >>> in implicitly_declare_fn so that it's more independent of context. >> >> Hmm, perhaps it or synthesized_method_walk should use maybe_push_to_top_level? > > That works nicely, and also fixes the other regression PR113332. There > the lambda context triggering synthesization of a default ctor was > causing maybe_dummy_object to misbehave during overload resolution of > one of its member's default ctors, and now synthesization is context > independent. > >> >>> 2. Don't defer a silent access check when in a template context, >>> since such deferred checks will be replayed noisily at instantiation >>> time which may not be what the caller intended. >> >> True, but returning a possibly incorrect 'false' is probably also not what the >> caller intended. It would be better to see that we never call enforce_access >> with tf_none in a template. If that's not feasible, I think we should still >> conservatively return true. > > Makes sense, I can experiment with that enforce_access access change as > a follow-up. > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > trunk? OK. > -- >8 -- > > Subject: [PATCH] c++: synthesized_method_walk context independence [PR113908] > > PR c++/113908 > PR c++/113332 > > gcc/cp/ChangeLog: > > * method.cc (synthesized_method_walk): Use maybe_push_to_top_level. > > gcc/testsuite/ChangeLog: > > * g++.dg/template/non-dependent31.C: New test. > * g++.dg/template/non-dependent32.C: New test. > --- > gcc/cp/method.cc | 2 ++ > .../g++.dg/template/non-dependent31.C | 18 +++++++++++++++++ > .../g++.dg/template/non-dependent32.C | 20 +++++++++++++++++++ > 3 files changed, 40 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/template/non-dependent31.C > create mode 100644 gcc/testsuite/g++.dg/template/non-dependent32.C > > diff --git a/gcc/cp/method.cc b/gcc/cp/method.cc > index 957496d3e18..98c10e6a8b5 100644 > --- a/gcc/cp/method.cc > +++ b/gcc/cp/method.cc > @@ -2760,6 +2760,7 @@ synthesized_method_walk (tree ctype, special_function_kind sfk, bool const_p, > return; > } > > + bool push_to_top = maybe_push_to_top_level (TYPE_NAME (ctype)); > ++cp_unevaluated_operand; > ++c_inhibit_evaluation_warnings; > push_deferring_access_checks (dk_no_deferred); > @@ -2857,6 +2858,7 @@ synthesized_method_walk (tree ctype, special_function_kind sfk, bool const_p, > pop_deferring_access_checks (); > --cp_unevaluated_operand; > --c_inhibit_evaluation_warnings; > + maybe_pop_from_top_level (push_to_top); > } > > /* DECL is a defaulted function whose exception specification is now > diff --git a/gcc/testsuite/g++.dg/template/non-dependent31.C b/gcc/testsuite/g++.dg/template/non-dependent31.C > new file mode 100644 > index 00000000000..3fa68f40fe1 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/non-dependent31.C > @@ -0,0 +1,18 @@ > +// PR c++/113908 > +// { dg-do compile { target c++11 } } > + > +struct A { > + A(); > +private: > + A(const A&); > +}; > + > +struct B { > + A a; > + > + template<class T> > + static void f() { new B(); } > +}; > + > +template void B::f<int>(); > +static_assert(!__is_constructible(B, const B&), ""); > diff --git a/gcc/testsuite/g++.dg/template/non-dependent32.C b/gcc/testsuite/g++.dg/template/non-dependent32.C > new file mode 100644 > index 00000000000..246654c5b50 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/non-dependent32.C > @@ -0,0 +1,20 @@ > +// PR c++/113332 > +// { dg-do compile { target c++11 } } > + > +struct tuple { > + template<class _Tp> > + static constexpr bool __is_implicitly_default_constructible() { return true; } > + > + template<class _Tp = void, > + bool = __is_implicitly_default_constructible<_Tp>()> > + tuple(); > +}; > + > +struct DBusStruct { > +private: > + tuple data_; > +}; > + > +struct IBusService { > + int m = [] { DBusStruct{}; return 42; }(); > +};
diff --git a/gcc/cp/method.cc b/gcc/cp/method.cc index 957496d3e18..17d42143299 100644 --- a/gcc/cp/method.cc +++ b/gcc/cp/method.cc @@ -3088,6 +3088,7 @@ implicitly_declare_fn (special_function_kind kind, tree type, tree this_parm; tree name; HOST_WIDE_INT saved_processing_template_decl; + tree saved_current_template_parms; bool deleted_p = false; bool constexpr_p = false; tree inherited_ctor = (kind == sfk_inheriting_constructor @@ -3132,6 +3133,10 @@ implicitly_declare_fn (special_function_kind kind, tree type, when not in a template. */ saved_processing_template_decl = processing_template_decl; processing_template_decl = 0; + /* Also clear CURRENT_TEMPLATE_PARMS so enforce_access immediately checks + access even in a template context. */ + saved_current_template_parms = current_template_parms; + current_template_parms = NULL_TREE; type = TYPE_MAIN_VARIANT (type); @@ -3341,8 +3346,9 @@ implicitly_declare_fn (special_function_kind kind, tree type, set_constraints (fn, new_ci); } - /* Restore PROCESSING_TEMPLATE_DECL. */ + /* Restore PROCESSING_TEMPLATE_DECL and CURRENT_TEMPLATE_PARMS. */ processing_template_decl = saved_processing_template_decl; + current_template_parms = saved_current_template_parms; if (inherited_ctor && TREE_CODE (inherited_ctor) == TEMPLATE_DECL) fn = add_inherited_template_parms (fn, inherited_ctor); diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc index c4292c49ce2..29120995449 100644 --- a/gcc/cp/parser.cc +++ b/gcc/cp/parser.cc @@ -4350,8 +4350,10 @@ cp_parser_make_typename_type (cp_parser *parser, tree id, tree result; if (identifier_p (id)) { + push_deferring_access_checks (dk_deferred); result = make_typename_type (parser->scope, id, typename_type, /*complain=*/tf_none); + pop_to_parent_deferring_access_checks (); if (result == error_mark_node) cp_parser_diagnose_invalid_type_name (parser, id, id_location); return result; diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 3299e270446..0a18eaf9c2e 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -346,6 +346,7 @@ enforce_access (tree basetype_path, tree decl, tree diag_decl, tree cs = current_scope (); if (in_template_context + && (complain & tf_error) && (CLASS_TYPE_P (cs) || TREE_CODE (cs) == FUNCTION_DECL)) if (tree template_info = get_template_info (cs)) { diff --git a/gcc/testsuite/g++.dg/template/non-dependent31.C b/gcc/testsuite/g++.dg/template/non-dependent31.C new file mode 100644 index 00000000000..3fa68f40fe1 --- /dev/null +++ b/gcc/testsuite/g++.dg/template/non-dependent31.C @@ -0,0 +1,18 @@ +// PR c++/113908 +// { dg-do compile { target c++11 } } + +struct A { + A(); +private: + A(const A&); +}; + +struct B { + A a; + + template<class T> + static void f() { new B(); } +}; + +template void B::f<int>(); +static_assert(!__is_constructible(B, const B&), "");