diff mbox series

c++: Fix access checking of scoped non-static member [PR98515]

Message ID 20210106181932.1393432-1-ppalka@redhat.com
State New
Headers show
Series c++: Fix access checking of scoped non-static member [PR98515] | expand

Commit Message

Patrick Palka Jan. 6, 2021, 6:19 p.m. UTC
In the first testcase below, we incorrectly reject the use of the
protected non-static member A::var0 from C<int>::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<T> of C<T> doesn't have a binfo object, so it appears
to DERIVED_FROM_P that A is not an indirect base of C<T>.)  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

Comments

Jason Merrill Jan. 7, 2021, 9:37 p.m. UTC | #1
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<int>::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<T> of C<T> doesn't have a binfo object, so it appears
> to DERIVED_FROM_P that A is not an indirect base of C<T>.)  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.

> +	  /* In general we can't know whether this access goes through `this'
> +	     until instantiation of the current class.  Punt now, or else
> +	     we might create a deferred access check that's relative to the
> +	     wrong class.  We'll check this access again after substitution,
> +	     e.g. from tsubst_qualified_id.  */
> +	    return true;
> +
> +	  if (tree current = current_nonlambda_class_type ())
> +	    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 <class> struct B : public A { };
> +template <class T> struct C : public B<T> { void g(); };
> +template <class T> void C<T>::g() { A::var0++; }
> +template class C<int>;
> 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 <class> struct B : public A { };
> +template <class T> struct C : public B<T> { void g(); };
> +template <class T> void C<T>::g() { A::var0++; } // { dg-error "protected|invalid" }
> +template <> struct B<char> { };
> +template class C<char>;
>
Patrick Palka Jan. 7, 2021, 10:47 p.m. UTC | #2
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<int>::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<T> of C<T> doesn't have a binfo object, so it appears
> > to DERIVED_FROM_P that A is not an indirect base of C<T>.)  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<int>::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<T> of C<T> doesn't have a binfo object, so it appears
to DERIVED_FROM_P that A is not an indirect base of C<T>.)  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 <class> struct B : public A { };
+template <class T> struct C : public B<T> { void g(); };
+template <class T> void C<T>::g() { A::var0++; }
+template class C<int>;
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 <class> struct B : public A { };
+template <class T> struct C : public B<T> { void g(); };
+template <class T> void C<T>::g() { A::var0++; } // { dg-error "protected|invalid" }
+template <> struct B<char> { };
+template class C<char>;
Jason Merrill Jan. 8, 2021, 1:12 a.m. UTC | #3
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<int>::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<T> of C<T> doesn't have a binfo object, so it appears
>>> to DERIVED_FROM_P that A is not an indirect base of C<T>.)  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".  :)

>  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.

> -- >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<int>::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<T> of C<T> doesn't have a binfo object, so it appears
> to DERIVED_FROM_P that A is not an indirect base of C<T>.)  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 <class> struct B : public A { };
> +template <class T> struct C : public B<T> { void g(); };
> +template <class T> void C<T>::g() { A::var0++; }
> +template class C<int>;
> 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 <class> struct B : public A { };
> +template <class T> struct C : public B<T> { void g(); };
> +template <class T> void C<T>::g() { A::var0++; } // { dg-error "protected|invalid" }
> +template <> struct B<char> { };
> +template class C<char>;
>
Patrick Palka Jan. 8, 2021, 3:39 p.m. UTC | #4
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<int>::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<T> of C<T> doesn't have a binfo object, so it appears
> > > > to DERIVED_FROM_P that A is not an indirect base of C<T>.)  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<int>::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<T> of C<T> doesn't have a binfo object, so it appears
> > to DERIVED_FROM_P that A is not an indirect base of C<T>.)  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 <class> struct B : public A { };
> > +template <class T> struct C : public B<T> { void g(); };
> > +template <class T> void C<T>::g() { A::var0++; }
> > +template class C<int>;
> > 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 <class> struct B : public A { };
> > +template <class T> struct C : public B<T> { void g(); };
> > +template <class T> void C<T>::g() { A::var0++; } // { dg-error
> > "protected|invalid" }
> > +template <> struct B<char> { };
> > +template class C<char>;
> > 
> 
>
diff mbox series

Patch

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)))
+	  /* In general we can't know whether this access goes through `this'
+	     until instantiation of the current class.  Punt now, or else
+	     we might create a deferred access check that's relative to the
+	     wrong class.  We'll check this access again after substitution,
+	     e.g. from tsubst_qualified_id.  */
+	    return true;
+
+	  if (tree current = current_nonlambda_class_type ())
+	    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 <class> struct B : public A { };
+template <class T> struct C : public B<T> { void g(); };
+template <class T> void C<T>::g() { A::var0++; }
+template class C<int>;
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 <class> struct B : public A { };
+template <class T> struct C : public B<T> { void g(); };
+template <class T> void C<T>::g() { A::var0++; } // { dg-error "protected|invalid" }
+template <> struct B<char> { };
+template class C<char>;