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 [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id 3CE773861019 for ; Thu, 7 Jan 2021 22:47:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3CE773861019 Received: from mail-il1-f197.google.com (mail-il1-f197.google.com [209.85.166.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-254-Vy29WmvLOcCp9dNnRmHWdg-1; Thu, 07 Jan 2021 17:47:52 -0500 X-MC-Unique: Vy29WmvLOcCp9dNnRmHWdg-1 Received: by mail-il1-f197.google.com with SMTP id z8so8257868ilq.21 for ; Thu, 07 Jan 2021 14:47:52 -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=to8QZmBrf2GKmAeTsG4GMmoHkA2igLH5Uoa2aIM+Dg4=; b=huwVnOfmXlYUX0AEGXMHaE9PTDzOTKL+ENb+Gg91H4E7eCK1Xf0Vqnh1Tx0L4XhLTc EzngK2yMDJVx5vl3DBkBvLpa7beysxIrlaoqtZUa/gBmO8XEohcT6+cf3oOf6VKhaBhu UdHQWMQzb3vsMrQFPXRsrVRw0Kj1yDaqk+RdCgc72t7y3SUrghMPw03gkIOrtng8WZfb lSNszJdEerPo2JjJ8PfXCscZo1VWvkPJNHq2tbQIZ8Vdh3kk55fvqZXG/pqm0iBRAE35 OENqtjdr9l1PJhO+s6sNUo3bOREdDJLkQOU4/A7gWggyUioUmGaUjcx9EYHObrhlhmXd Ulhw== X-Gm-Message-State: AOAM530i99d/gd2JG0Ubf0hUso24xOGU2iq2AbWIQTfnUjvnHC0yzNjl 5KWHmXz39I4MlGHV7A07AZa63VnCnfnZlTVpKNj/5QCfWdoBxgxwOREK37koJphSFnbGP4Zw/CE KVkKEJw0I37eQhMsOww== X-Received: by 2002:a92:9806:: with SMTP id l6mr1104230ili.304.1610059671276; Thu, 07 Jan 2021 14:47:51 -0800 (PST) X-Google-Smtp-Source: ABdhPJztDRQDru+nkxD89HVLCfDLauOHUJT0aMEA1HqTkBdw6Ugmsn1xJz1w6ebxNZjSPR8rwGwWUg== X-Received: by 2002:a92:9806:: with SMTP id l6mr1104216ili.304.1610059671041; Thu, 07 Jan 2021 14:47:51 -0800 (PST) Received: from [192.168.1.130] (ool-457d493a.dyn.optonline.net. [69.125.73.58]) by smtp.gmail.com with ESMTPSA id t26sm3973211ioi.11.2021.01.07.14.47.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Jan 2021 14:47:50 -0800 (PST) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Thu, 7 Jan 2021 17:47:48 -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: Message-ID: <30ad8ec-9fd-fe-b5b1-3fac7e3e6fb@idea> References: <20210106181932.1393432-1-ppalka@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_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: Thu, 07 Jan 2021 22:47:55 -0000 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. 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? -- >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; -- 2.30.0