Re: [PATCH] c++: implicitly_declare_fn and access checks [PR113908]
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 000..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 + static void f() { new B(); } +}; + +template void B::f(); +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 000..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 + static constexpr bool __is_implicitly_default_constructible() { return true; } + + template()> + tuple(); +}; + +struct DBusStruct { +private: + tuple data_; +}; + +struct IBusService { + int m = [] { DBusStruct{}; return 42; }(); +};
Re: [PATCH] c++: implicitly_declare_fn and access checks [PR113908]
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 000..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 + static void f() { new B(); } +}; + +template void B::f(); +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 000..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 + static constexpr bool __is_implicitly_default_constructible() { return true; } + + template()> + tuple(); +}; + +struct DBusStruct { +private: + tuple data_; +}; + +struct IBusService { + int m = [] { DBusStruct{}; return 42; }(); +}; -- 2.44.0.rc0.46.g2996f11c1d
Re: [PATCH] c++: implicitly_declare_fn and access checks [PR113908]
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