Re: [PATCH] c++: access for hidden friend of nested class template [PR100502]

[Jason Merrill <jason@redhat.com>]

On 5/26/21 10:31 AM, Patrick Palka wrote:
> On Tue, 25 May 2021, Jason Merrill wrote:
> 
>> On 5/25/21 10:50 AM, Patrick Palka wrote:
>>> On Mon, 24 May 2021, Jason Merrill wrote:
>>>
>>>> On 5/21/21 4:35 PM, Patrick Palka wrote:
>>>>> Here, during ahead of time access checking for the private member
>>>>> EnumeratorRange::end_reached_ in the hidden friend f, we're triggering
>>>>> the the assert in enforce_access that verifies we're not trying to add a
>>>>> dependent access check to TI_DEFERRED_ACCESS_CHECKS.
>>>>>
>>>>> The special thing about this class member access expression is that it's
>>>>> considered to be non-type-dependent (so finish_class_member_access_expr
>>>>> doesn't exit early at template parse time), and then accessible_p
>>>>> rejects the access (so we don't exit early from enforce access either,
>>>>> and end up triggering the assert).  I think we're correct to reject it
>>>>> because a hidden friend is not a member function, so [class.access.nest]
>>>>> doesn't apply, and also a hidden friend of a nested class is not a
>>>>> friend of the enclosing class.  (Note that Clang accepts the testcase
>>>>> and MSVC and ICC reject it.)
>>>>
>>>> Hmm, I think you're right, but that seems inconsistent with the change
>>>> (long
>>>> ago) to give nested classes access to members of the enclosing class.
>>>
>>> I guess the question is whether a hidden friend is considered to be a
>>> class member for sake of access checking.  Along that note, I noticed
>>> Clang/GCC/MSVC/ICC all accept the access of A::f in:
>>>
>>>     struct A {
>>>     protected:
>>>       static void f();
>>>     };
>>>
>>>     struct B : A {
>>>       friend void g() { A::f(); }
>>>     };
>>>
>>> But arguably this is valid iff g is considered to be a member of B.
>>>
>>> If we adjust the above example to define the friend g at namespace
>>> scope:
>>>
>>>     struct A {
>>>     protected:
>>>       static void f();
>>>     };
>>>
>>>     struct B : A {
>>>       friend void g();
>>>     };
>>>
>>>     void g() { A::f(); }
>>>
>>> then GCC/MSVC/ICC accept and Clang rejects.  But this second example is
>>> definitely invalid since it's just a special case of the example in
>>> [class.protected], which says:
>>>
>>>     void fr() {
>>>       ...
>>>       B::j = 5;                     // error: not a friend of naming class B
>>>       ...
>>>     }
>>>
>>>>
>>>>> This patch relaxes the problematic assert in enforce_access to check
>>>>> dependent_scope_p instead of uses_template_parms, which is the more
>>>>> accurate notion of dependence we care about.
>>>>
>>>> Agreed.
>>>>
>>>>> This change alone is
>>>>> sufficient to fix the ICE, but we now end up diagnosing each access
>>>>> twice, once at substitution time and again from
>>>>> TI_DEFERRED_ACCESS_CHECKS.
>>>>> So this patch additionally disables ahead of time access checking
>>>>> during the call to lookup_member from finish_class_member_access_expr;
>>>>> we're going to check the same access again at substitution time anyway.
>>>>
>>>> That seems undesirable; it's better to diagnose when parsing if we can.
>>>> Why is
>>>> it going on TI_DEFERRED_ACCESS_CHECKS after we already checked it?
>>>
>>> At parse time, a negative accessible_p answer only means "maybe not
>>> accessible" rather than "definitely not accessible", since access
>>> may still be granted to some specialization of the current template
>>> via a friend declaration.  I think we'd need to beef up accessible_p a
>>> bit before we can begin diagnosing accesses at template parse time.
>>> This probably wouldn't be too hairy to implement; I'll look into it.
>>
>> Ah, I missed that you were saying twice at substitution time.  You're right
>> that in general we can't diagnose at parse time.
>>
>>> For now, would the assert relaxation in enforce_access be OK for
>>> trunk/11?
>>
>> Yes.  And the other hunk is OK for trunk.
> 
> Thanks a lot.  Sadly it appears the assert relaxation is only sufficient
> to suppress the ICE, and with it alone we still don't accept the
> access37a.C testcase below, so backporting just the assert change
> wouldn't fully fix the regression.
> 
> The reason is that without the finish_class_member_access_expr hunk, we still
> push an access check for the dependent member EnumeratorRange<T>::end_reached_
> onto TI_DEFERRED_ACCESS_CHECKS.  And because
> perform_instantiation_time_access_checks substitutes only into the scope
> of the member and not also into the member itself, we effectively end up
> checking whether EnumeratorRange<T>::end_reached_ is accessible from
> EnumeratorRange<int>, which'll always fail when the member is private.
> Perhaps we could also substitute into the declaration here, but that
> seems wrong because it'd create a new declaration.
> 
> So in light of this, it seems we _shouldn't_ relax the assert in
> enforce_access because it currently accurately mirrors the assumptions
> of perform_instantiation_time_access_checks.  And it seems it's
> necessary to backport the finish_class_member_access_expr hunk.
> Sorry, I should have realized this sooner..
> 
> Would then the following be OK for trunk/11?
> 
> -- >8 --
> 
> 	PR c++/100502
> 
> gcc/cp/ChangeLog:
> 
> 	* typeck.c (finish_class_member_access_expr): Disable ahead
> 	of time access checking during the member lookup.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/template/access37.C: New test.
> 	* g++.dg/template/access37a.C: New test.
> ---
>   gcc/cp/typeck.c                           |  6 ++++++
>   gcc/testsuite/g++.dg/template/access37.C  | 26 +++++++++++++++++++++++
>   gcc/testsuite/g++.dg/template/access37a.C |  6 ++++++
>   3 files changed, 38 insertions(+)
>   create mode 100644 gcc/testsuite/g++.dg/template/access37.C
>   create mode 100644 gcc/testsuite/g++.dg/template/access37a.C
> 
> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> index 703ddd3cc7a..86cf26b9c84 100644
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -3201,9 +3201,15 @@ finish_class_member_access_expr (cp_expr object, tree name, bool template_p,
>   	{
>   	  /* Look up the member.  */
>   	  access_failure_info afi;
> +	  if (processing_template_decl)
> +	    /* We're going to redo this member lookup at instantiation
> +	       time, so don't bother checking access ahead of time here.  */

Please reword the comment now that we know this is needed for 
correctness.  OK with that change.

> +	    push_deferring_access_checks (dk_no_check);
>   	  member = lookup_member (access_path, name, /*protect=*/1,
>   				  /*want_type=*/false, complain,
>   				  &afi);
> +	  if (processing_template_decl)
> +	    pop_deferring_access_checks ();
>   	  afi.maybe_suggest_accessor (TYPE_READONLY (object_type));
>   	  if (member == NULL_TREE)
>   	    {
> diff --git a/gcc/testsuite/g++.dg/template/access37.C b/gcc/testsuite/g++.dg/template/access37.C
> new file mode 100644
> index 00000000000..92fed3e97cb
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/access37.C
> @@ -0,0 +1,26 @@
> +// PR c++/100502
> +
> +template <class>
> +struct EnumeratorRange {
> +  struct Iterator {
> +    EnumeratorRange range_;
> +
> +    friend void f(Iterator i) {
> +      i.range_.end_reached_; // { dg-error "private" }
> +      i.range_.EnumeratorRange::end_reached_; // { dg-error "private" }
> +      &i.range_.end_reached_; // { dg-error "private" }
> +      &i.range_.EnumeratorRange::end_reached_; // { dg-error "private" }
> +    }
> +  };
> +
> + private:
> +  bool end_reached_;
> +#if DECLARE_FRIEND
> +  friend void f(Iterator);
> +#endif
> +};
> +
> +int main() {
> +  EnumeratorRange<int>::Iterator i;
> +  f(i);
> +}
> diff --git a/gcc/testsuite/g++.dg/template/access37a.C b/gcc/testsuite/g++.dg/template/access37a.C
> new file mode 100644
> index 00000000000..4ce1b2718a0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/access37a.C
> @@ -0,0 +1,6 @@
> +// PR c++/100502
> +// { dg-additional-options "-DDECLARE_FRIEND -Wno-non-template-friend" }
> +
> +// Verify that access37.C is accepted if the appropriate friend relation
> +// is declared (controlled by the macro DECLARE_FRIEND).
> +#include "access37.C"
>