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 DDB1D39730D9 for ; Fri, 8 Jan 2021 15:39:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DDB1D39730D9 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-483-ZmRpvzHhNyKFc0UTDe6ylA-1; Fri, 08 Jan 2021 10:39:14 -0500 X-MC-Unique: ZmRpvzHhNyKFc0UTDe6ylA-1 Received: by mail-qt1-f200.google.com with SMTP id w17so4529324qta.20 for ; Fri, 08 Jan 2021 07:39:14 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:mime-version; bh=KJC13EDNJo5n3KOwsavVFhzFAlgWP5Eh7mRj5iF/u00=; b=q+ahmyAurV4DZvraraihqn5yVc1/xsxv9gRsTfyh/rF7uC+GZl9k0wVrALryT8HDzD I8H6PB6O+V+mT6yQO4hw3pLjkTA0P+0sFHVjhkOninDNckeKgJ5Z5FLdSX+MiNpGwE6/ lX/n0ynA21XE86PFjLFed3I/dADUVk8NrPd032e6npL6J9Bjum7HTuNr4IO+ViyiQXyu Yegj+6HoBBUdPdyzqtQ9TgVH2obZxH5TCnp0WvwV7TQ69qFq5PFskCEYdjeEx/ZyV7KK Nh9ybqqjcReQx26KteLEc5cA720gBun8vekwJWJmzfSn9vs7At2PGY0dDomhKF8VKn14 vP6w== X-Gm-Message-State: AOAM532Kwou4tj36sSxMJkyjNizNDV1mvM5Y+HTgBZ9/gJacecapP4lu DDRY/z9ZSSlNCyyrTVjox/Y0kCCqRL3XHLYVl35BAJo6VH9WnS7Au1KThOUMjGREji8PmWeN80L ZuiN6QU97D8tUxpdgSw== X-Received: by 2002:a05:6214:d05:: with SMTP id 5mr4049640qvh.54.1610120354226; Fri, 08 Jan 2021 07:39:14 -0800 (PST) X-Google-Smtp-Source: ABdhPJxk/yMuK7lwrziburD44w7U7G8reGRtVsZf1Ssi21KuNeLqkC30dMrLboHjHZaHFqriktCj0w== X-Received: by 2002:a05:6214:d05:: with SMTP id 5mr4049614qvh.54.1610120353873; Fri, 08 Jan 2021 07:39:13 -0800 (PST) Received: from [192.168.1.130] (ool-457d493a.dyn.optonline.net. [69.125.73.58]) by smtp.gmail.com with ESMTPSA id c20sm4478833qtj.29.2021.01.08.07.39.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 08 Jan 2021 07:39:13 -0800 (PST) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Fri, 8 Jan 2021 10:39:11 -0500 (EST) To: Jason Merrill cc: Patrick Palka , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++: Fix access checking of scoped non-static member [PR98515] In-Reply-To: <6f105a0f-c591-5995-82c3-a1d4a9d4109e@redhat.com> Message-ID: <15aa4b24-b80-d5f7-a9c-dbf098529888@idea> References: <20210106181932.1393432-1-ppalka@redhat.com> <30ad8ec-9fd-fe-b5b1-3fac7e3e6fb@idea> <6f105a0f-c591-5995-82c3-a1d4a9d4109e@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-16.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, 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: Fri, 08 Jan 2021 15:39:18 -0000 On Thu, 7 Jan 2021, Jason Merrill wrote: > On 1/7/21 5:47 PM, Patrick Palka wrote: > > On Thu, 7 Jan 2021, Jason Merrill wrote: > > > > > On 1/6/21 1:19 PM, Patrick Palka wrote: > > > > In the first testcase below, we incorrectly reject the use of the > > > > protected non-static member A::var0 from C::g() because > > > > check_accessibility_of_qualified_id, at template parse time, determines > > > > that the access doesn't go through 'this'. (This happens because the > > > > dependent base B of C doesn't have a binfo object, so it appears > > > > to DERIVED_FROM_P that A is not an indirect base of C.) From there > > > > we create the corresponding deferred access check, which we then > > > > perform at instantiation time and which (unsurprisingly) fails. > > > > > > > > The problem ultimately seems to be that we can't, in general, know > > > > whether a use of a scoped non-static member goes through 'this' until > > > > instantiation time, as the second testcase below demonstrates. So this > > > > patch makes check_accessibility_of_qualified_id punt in this situation. > > > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK to > > > > commit? > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > PR c++/98515 > > > > * semantics.c (check_accessibility_of_qualified_id): Punt if > > > > we're checking the access of a scoped non-static member at > > > > class template parse time. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > PR c++/98515 > > > > * g++.dg/template/access32.C: New test. > > > > * g++.dg/template/access33.C: New test. > > > > --- > > > > gcc/cp/semantics.c | 20 +++++++++++++++----- > > > > gcc/testsuite/g++.dg/template/access32.C | 8 ++++++++ > > > > gcc/testsuite/g++.dg/template/access33.C | 9 +++++++++ > > > > 3 files changed, 32 insertions(+), 5 deletions(-) > > > > create mode 100644 gcc/testsuite/g++.dg/template/access32.C > > > > create mode 100644 gcc/testsuite/g++.dg/template/access33.C > > > > > > > > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c > > > > index b448efe024a..f52b2e4d1e7 100644 > > > > --- a/gcc/cp/semantics.c > > > > +++ b/gcc/cp/semantics.c > > > > @@ -2107,14 +2107,24 @@ check_accessibility_of_qualified_id (tree decl, > > > > /* If the reference is to a non-static member of the > > > > current class, treat it as if it were referenced through > > > > `this'. */ > > > > - tree ct; > > > > if (DECL_NONSTATIC_MEMBER_P (decl) > > > > - && current_class_ptr > > > > - && DERIVED_FROM_P (scope, ct = current_nonlambda_class_type ())) > > > > - qualifying_type = ct; > > > > + && current_class_ptr) > > > > + { > > > > + if (dependent_type_p (TREE_TYPE (current_class_ptr))) > > > > > > This should also look at current_nonlambda_class_type. > > > > Ah, ack. But it seems to me we really only need to be checking > > dependence of current_nonlambda_class_type here. > > Yes, that's what I meant, sorry about the ambiguous use of "also". :) Oops, I see what you had meant now, sorry about the confusion :) > > > IIUC, dependence of > > these two types should coincide except in the case where we're inside a > > generic lambda within a non-template class (in which case > > current_class_ptr will dependent and current_nonlambda_class_type won't). > > But in this case we have enough information to be able to resolve the > > access check at parse time, I think (and so we shouldn't punt). > > > > The below patch, which seems to pass 'make check-c++', checks the > > dependence of current_nonlambda_class_type instead of that of > > current_class_ptr. Does this approach seem right? > > OK. Thanks. > > > -- >8 -- > > > > Subject: [PATCH] c++: Fix access checking of scoped non-static member > > [PR98515] > > > > In the first testcase below, we incorrectly reject the use of the > > protected non-static member A::var0 from C::g() because > > check_accessibility_of_qualified_id, at template parse time, determines > > that the access doesn't go through 'this'. (This happens because the > > dependent base B of C doesn't have a binfo object, so it appears > > to DERIVED_FROM_P that A is not an indirect base of C.) From there > > we create the corresponding deferred access check, which we then > > perform at instantiation time and which (unsurprisingly) fails. > > > > The problem ultimately seems to be that we can't in general determine > > whether a use of a scoped non-static member goes through 'this' until > > instantiation time, as the second testcase below illustrates. So this > > patch makes check_accessibility_of_qualified_id punt in such situations > > to avoid creating a bogus deferred access check. > > > > gcc/cp/ChangeLog: > > > > PR c++/98515 > > * semantics.c (check_accessibility_of_qualified_id): Punt if > > we're checking access of a scoped non-static member inside a > > class template. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/98515 > > * g++.dg/template/access32.C: New test. > > * g++.dg/template/access33.C: New test. > > --- > > gcc/cp/semantics.c | 22 +++++++++++++++++----- > > gcc/testsuite/g++.dg/template/access32.C | 8 ++++++++ > > gcc/testsuite/g++.dg/template/access33.C | 9 +++++++++ > > 3 files changed, 34 insertions(+), 5 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/template/access32.C > > create mode 100644 gcc/testsuite/g++.dg/template/access33.C > > > > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c > > index b448efe024a..51f7c114b03 100644 > > --- a/gcc/cp/semantics.c > > +++ b/gcc/cp/semantics.c > > @@ -2107,14 +2107,26 @@ check_accessibility_of_qualified_id (tree decl, > > /* If the reference is to a non-static member of the > > current class, treat it as if it were referenced through > > `this'. */ > > - tree ct; > > if (DECL_NONSTATIC_MEMBER_P (decl) > > - && current_class_ptr > > - && DERIVED_FROM_P (scope, ct = current_nonlambda_class_type ())) > > - qualifying_type = ct; > > + && current_class_ptr) > > + { > > + if (tree current = current_nonlambda_class_type ()) > > + { > > + if (dependent_type_p (current)) > > + /* In general we can't know whether this access goes through > > + `this' until instantiation time. Punt now, or else we might > > + create a deferred access check that's not relative to 'this' > > + when it ought to be. We'll check this access again after > > + substitution, e.g. from tsubst_qualified_id. */ > > + return true; > > + > > + if (DERIVED_FROM_P (scope, current)) > > + qualifying_type = current; > > + } > > + } > > /* Otherwise, use the type indicated by the > > nested-name-specifier. */ > > - else > > + if (!qualifying_type) > > qualifying_type = nested_name_specifier; > > } > > else > > diff --git a/gcc/testsuite/g++.dg/template/access32.C > > b/gcc/testsuite/g++.dg/template/access32.C > > new file mode 100644 > > index 00000000000..08faa9f0f97 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/template/access32.C > > @@ -0,0 +1,8 @@ > > +// PR c++/98515 > > +// { dg-do compile } > > + > > +struct A { protected: int var0; }; > > +template struct B : public A { }; > > +template struct C : public B { void g(); }; > > +template void C::g() { A::var0++; } > > +template class C; > > diff --git a/gcc/testsuite/g++.dg/template/access33.C > > b/gcc/testsuite/g++.dg/template/access33.C > > new file mode 100644 > > index 00000000000..9fb9b9a1236 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/template/access33.C > > @@ -0,0 +1,9 @@ > > +// PR c++/98515 > > +// { dg-do compile } > > + > > +struct A { protected: int var0; }; > > +template struct B : public A { }; > > +template struct C : public B { void g(); }; > > +template void C::g() { A::var0++; } // { dg-error > > "protected|invalid" } > > +template <> struct B { }; > > +template class C; > > > >