From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id E05DD3857C5F for ; Wed, 26 May 2021 15:29:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E05DD3857C5F Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-38-gmnQjYRDPo2BI9c0TCf0ug-1; Wed, 26 May 2021 11:29:15 -0400 X-MC-Unique: gmnQjYRDPo2BI9c0TCf0ug-1 Received: by mail-qv1-f69.google.com with SMTP id z93-20020a0ca5e60000b02901ec19d8ff47so1506776qvz.8 for ; Wed, 26 May 2021 08:29:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=KztYxRH18/ua75yH45tXzP9xiMpWrK6XIT45ppAtH3U=; b=fGO93VdJ2XiBwt5knz2AYKGDHnGqHchzTAuhCNZwn4kSySIfWpseicRAi0MqL2spCW VrGRXltLnF3omPcbV1eCP/DO3zz9AOq43jeMl8BbWyynzCLkUzVjlchPaKsfb+N1qV1u SbmQPSvFVYFLPHyf3607d1/RFvBRvuRfmPTiyArj2GhdvKr6qO/5Bcpm4A8OST5vL+7O wKnJKx/HTwfEcPkBFv26g/O2NSI2MXl7C+URWkbUTDsB29ND+ouYGBTGPfRKOT5+j7Y6 h3g2hCUp7SxtIbUjn9MxclI5WrloeRiJQmAlZXk7K3YPbjviy7m9FfrQpS7G5oVxZmAm x8tg== X-Gm-Message-State: AOAM531FSg0wFQ2PtgtWc6KnYt3Bu602Idz3KDZgKGLE4kiy7ChlEgr7 oIergT+qJ8wrOuJkDAhMQmZ0vftdIphS7tv9wm5djT6Z4nUdcB+RuNeBRXGX32Ry0vZKkrnfYpQ +3ALa1kZDq0yucQThGym49xCV21K9VFaPGztM+8/sPUcMcGcCBjO4c2tXkYbUwQFkUw== X-Received: by 2002:a37:482:: with SMTP id 124mr39381807qke.373.1622042953667; Wed, 26 May 2021 08:29:13 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxBksioEKMe4XrFdrPf4fMbAWLIahYHB15zWCy73/b1Dd00KkYvGoOCuwiq0XWhHZS+Dtv2kg== X-Received: by 2002:a37:482:: with SMTP id 124mr39381751qke.373.1622042952955; Wed, 26 May 2021 08:29:12 -0700 (PDT) Received: from [192.168.1.148] ([130.44.159.43]) by smtp.gmail.com with ESMTPSA id t25sm1713075qkt.62.2021.05.26.08.29.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 May 2021 08:29:11 -0700 (PDT) Subject: Re: [PATCH] c++: access for hidden friend of nested class template [PR100502] To: Patrick Palka Cc: gcc-patches@gcc.gnu.org References: <20210521203542.3815790-1-ppalka@redhat.com> <02cf0d83-e362-687c-6619-e8b81a7ad872@redhat.com> <4505f20-6034-9712-937e-2acb3c4fbea@idea> From: Jason Merrill Message-ID: <3c7910d4-c383-0c13-b2ab-f72af50c41af@redhat.com> Date: Wed, 26 May 2021 11:29:10 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 May 2021 15:29:18 -0000 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::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::end_reached_ is accessible from > EnumeratorRange, 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 > +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::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" >