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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id C99F6389853D for ; Tue, 25 May 2021 15:18:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C99F6389853D Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-403-mHc4Dl4lNzGTADW3fJRrSA-1; Tue, 25 May 2021 11:18:47 -0400 X-MC-Unique: mHc4Dl4lNzGTADW3fJRrSA-1 Received: by mail-qt1-f197.google.com with SMTP id i12-20020ac860cc0000b02901cb6d022744so26585551qtm.20 for ; Tue, 25 May 2021 08:18:47 -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=z3CPvpLSeonzY6RrFXYdkHWHu93+hRuUb3yi+/uqIyI=; b=r01RnGaEVATtJQ8Mfrcac9xvCjSRfA+R7BGNAwy/sQ5ShxTf4kw+M6VBoXGghKqPYv Q97WrkkuqLhSzDXiv2m3hd68mXgrB2Fcbqi3Laq8WgXcuX/3vKaHyby6NMpAP61t4GzX 9SiKOtv7qms+KQOQI0CGkMLIB1359ormmrR8po2MFajWsVl50OBC/9msNwKqg0ZV+0ka hKx4P0hxevDkpKzlt0xreao1movxIe/NMizfB1UZnHgjGwVRvmx8CgHSRSNh4lZH3vvS siAyyCD0xyMmnVUPv28go1nUIID04wp9dPLS8KUCy8yRI7QRxw+XJAlcQ9jqwc4cHZeN vDkw== X-Gm-Message-State: AOAM533lt+GhFqCjjPXzzGSkERBopZY03yaITMHDGKPay+gbHKI4zEDC qWr5m/N2lwqayX6bkK466ani/poaKS8tMyPe2N9Qxbq+qc/Jr/o4mCrOtJ00YcpCss80VmhchFT RTV7z71TAxqE3z9+8ujDMVU9D9vY+HIN4eFCD4y+CdFvb2WQlyEPz+RCW8ykeHF6lKw== X-Received: by 2002:a05:622a:1489:: with SMTP id t9mr4504656qtx.191.1621955925823; Tue, 25 May 2021 08:18:45 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxUjvyXM45NN5ylskKagtV+4qvbaUkGoggB4NQsCCbkoZtvwj4M/c5IB3/DUpyZl0rrzef7wQ== X-Received: by 2002:a05:622a:1489:: with SMTP id t9mr4504468qtx.191.1621955923934; Tue, 25 May 2021 08:18:43 -0700 (PDT) Received: from [192.168.1.148] ([130.44.159.43]) by smtp.gmail.com with ESMTPSA id 196sm7491740qkk.130.2021.05.25.08.18.42 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 25 May 2021 08:18:43 -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: Date: Tue, 25 May 2021 11:18:42 -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: <4505f20-6034-9712-937e-2acb3c4fbea@idea> 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: Tue, 25 May 2021 15:18:53 -0000 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. >>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for >>> trunk? For GCC 11, should we just backport the enforce_access hunk? >>> >>> PR c++/100502 >>> >>> gcc/cp/ChangeLog: >>> >>> * semantics.c (enforce_access): Relax assert about the type >>> depedence of the DECL_CONTEXT of the declaration. >>> * 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/semantics.c | 2 +- >>> gcc/cp/typeck.c | 6 ++++++ >>> gcc/testsuite/g++.dg/template/access37.C | 26 +++++++++++++++++++++++ >>> gcc/testsuite/g++.dg/template/access37a.C | 6 ++++++ >>> 4 files changed, 39 insertions(+), 1 deletion(-) >>> 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/semantics.c b/gcc/cp/semantics.c >>> index 0d590c318fb..0de14316bba 100644 >>> --- a/gcc/cp/semantics.c >>> +++ b/gcc/cp/semantics.c >>> @@ -365,7 +365,7 @@ enforce_access (tree basetype_path, tree decl, tree >>> diag_decl, >>> check here. */ >>> gcc_assert (!uses_template_parms (decl)); >>> if (TREE_CODE (decl) == FIELD_DECL) >>> - gcc_assert (!uses_template_parms (DECL_CONTEXT (decl))); >>> + gcc_assert (!dependent_scope_p (DECL_CONTEXT (decl))); >>> /* Defer this access check until instantiation time. */ >>> deferred_access_check access_check; >>> 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. */ >>> + 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" >>> >> >> >