Hi all,

I tried both:

(SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))
and
(SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)),
parent_binfo))

but both caused around 80 regressions because it was returning false when
it should have been returning true. No idea why. In the end I used

(same_type_p (context_for_name_lookup (decl), BINFO_TYPE (parent_binfo)))

which works. New patch attached.

Anthony

On Sat, 6 Feb 2021 at 13:09, Anthony Sharp <anthonyshar...@gmail.com> wrote:

> I think at least something like "Improve private access checking." would
>> be better.  No need to be verbose in the ChangeLog.  :)
>
>
> That sounds like a good idea, I will change it.
>
> Yup, this one.
>
>
> Awesome.
>
> Yeah, that can be a pain.  Best if your editor does it for you.  If you
>> use vim, you can use gcc/contrib/vimrc and then vim will do most of the
>> formatting for you.
>
>
> Aah I didn't know that, thanks for the tip!
>
> Anthony
>
>
>
> On Fri, 5 Feb 2021 at 17:46, Marek Polacek <pola...@redhat.com> wrote:
>
>> On Fri, Feb 05, 2021 at 05:28:07PM +0000, Anthony Sharp wrote:
>> > Hi Marek,
>> >
>> > > Pedantically, "Altered function." is not very good, it should say what
>> > > in enforce_access changed.
>> >
>> > I would normally 100% agree, but the changes are a bit too complicated
>> > to be concisely (and helpfully) described there. I think the patch
>> > description covers it well enough; not sure what I would say without
>> > having to write a big paragraph there.
>>
>> I think at least something like "Improve private access checking." would
>> be better.  No need to be verbose in the ChangeLog.  :)
>>
>> > > Let's move the test into g++.dg/cpp1z and call it using9.C.
>> >
>> > Okie dokie - it's a bit hard to know where stuff's supposed to go in
>> > that folder. I'll put a comment in the testcase mentioning pr19377
>> > just in case there's ever a regression.
>>
>> Yeah, it's customary to start a test with
>> // PR c++/19377
>>
>> > > I don't understand the "generate a diagnostic decl location".  Maybe
>> just
>> > > "generate a diagnostic?"
>> >
>> > get_class_access_diagnostic_decl returns a decl which is used as the
>> > location that the error-producing code points to as the source of the
>> > problem. It could be better - I will change it to say "Examine certain
>> > special cases to find the decl that is the source of the problem" to
>> > make it a bit clearer.
>>
>> Oh, I see now.
>>
>> > > These two comments can be merged into one.
>> >
>> > Technically yes ... but I like how it is since in a very subtle way it
>> > creates a sense of separation between the first two paragraphs and the
>> > third. The first two paras are sort of an introduction and the third
>> > begins to describe the code.
>>
>> Fair enough.
>>
>> > > I think for comparing a binfo with a type we should use
>> SAME_BINFO_TYPE_P.
>> >
>> > Okay, I think that simplifies the code a bit aswell.
>> >
>> > > This first term doesn't need to be wrapped in ().
>> >
>> > I normally wouldn't use the (), but I think that's how Jason likes it
>> > so that's why I did it. I guess it makes it more readable.
>> >
>> > > This could be part of the if above.
>> >
>> > Oops - I forgot to change that when I modified the code.
>> >
>> > > Just "had" instead of "did had"?
>> >
>> > Good spot - that's a spelling mistake on my part. Should be "did have".
>> >
>> > > Looks like this line is indented too much (even in the newer patch).
>> >
>> > You're right (if you meant access_failure_reason = ak_private), I will
>> change.
>>
>> Yup, this one.
>>
>> > If you mean get_class_access_diagnostic_decl (parent_binfo, diag_decl)
>> > then I donno, because Linux Text Editor says both are on column 64.
>> >
>> > To be honest, I'm sure there is a way to do it, but I'm not really
>> > sure how to consistently align code. Every text/code editor/browser
>> > seems to give different column numbers (and display it differently)
>> > since they seem to all treat whitespace differently.
>>
>> Yeah, that can be a pain.  Best if your editor does it for you.  If you
>> use vim, you can use gcc/contrib/vimrc and then vim will do most of the
>> formatting for you.
>>
>> > > We usually use dg-do compile.
>> >
>> > True, but wouldn't that technically be slower? I would agree if it
>> > were more than just error-handling code.
>> >
>> > > Best to replace both with
>> > > // { dg-do compile { target c++17 } }
>> >
>> > Okie dokie. I did see both being used and I wasn't sure which to go for.
>> >
>> > I'll probably send another patch over tomorrow.
>>
>> Sounds good, thanks!
>>
>> > On Fri, 5 Feb 2021 at 16:06, Marek Polacek <pola...@redhat.com> wrote:
>> > >
>> > > Hi,
>> > >
>> > > a few comments:
>> > >
>> > > On Fri, Feb 05, 2021 at 03:39:25PM +0000, Anthony Sharp via
>> Gcc-patches wrote:
>> > > > 2021-02-05  Anthony Sharp  <anthonyshar...@gmail.com>
>> > > >
>> > > >       * semantics.c (get_class_access_diagnostic_decl): New
>> function.
>> > > >       (enforce_access): Altered function.
>> > >
>> > > Pedantically, "Altered function." is not very good, it should say what
>> > > in enforce_access changed.
>> > >
>> > > > gcc/testsuite/ChangeLog:
>> > > >
>> > > > 2021-02-05  Anthony Sharp  <anthonyshar...@gmail.com>
>> > > >
>> > > >       * g++.dg/pr19377.C: New test.
>> > >
>> > > Let's move the test into g++.dg/cpp1z and call it using9.C.
>> > >
>> > > >  gcc/cp/semantics.c             | 89
>> ++++++++++++++++++++++++++--------
>> > > >  gcc/testsuite/g++.dg/pr19377.C | 28 +++++++++++
>> > > >  2 files changed, 98 insertions(+), 19 deletions(-)
>> > > >  create mode 100644 gcc/testsuite/g++.dg/pr19377.C
>> > > >
>> > > > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
>> > > > index 73834467fca..ffb627f08ea 100644
>> > > > --- a/gcc/cp/semantics.c
>> > > > +++ b/gcc/cp/semantics.c
>> > > > @@ -256,6 +256,58 @@ pop_to_parent_deferring_access_checks (void)
>> > > >      }
>> > > >  }
>> > > >
>> > > > +/* Called from enforce_access.  A class has attempted (but failed)
>> to access
>> > > > +   DECL.  It is already established that a baseclass of that class,
>> > > > +   PARENT_BINFO, has private access to DECL.  Examine certain
>> special cases to
>> > > > +   generate a diagnostic decl location.  If no special cases are
>> found, simply
>> > >
>> > > I don't understand the "generate a diagnostic decl location".  Maybe
>> just
>> > > "generate a diagnostic?"
>> > >
>> > > > +   return DECL.  */
>> > > > +
>> > > > +static tree
>> > > > +get_class_access_diagnostic_decl (tree parent_binfo, tree decl)
>> > > > +{
>> > > > +  /* When a class is denied access to a decl in a baseclass, most
>> of the
>> > > > +     time it is because the decl itself was declared as private at
>> the point
>> > > > +     of declaration.  So, by default, DECL is at fault.
>> > > > +
>> > > > +     However, in C++, there are (at least) two situations in which
>> a decl
>> > > > +     can be private even though it was not originally defined as
>> such.  */
>> > > > +
>> > > > +  /* These two situations only apply if a baseclass had private
>> access to
>> > > > +     DECL (this function is only called if that is the case).  We
>> must however
>> > > > +     also ensure that the reason the parent had private access
>> wasn't simply
>> > > > +     because it was declared as private in the parent.  */
>> > >
>> > > These two comments can be merged into one.
>> > >
>> > > > +  if (context_for_name_lookup (decl) == BINFO_TYPE (parent_binfo))
>> > > > +    return decl;
>> > >
>> > > I think for comparing a binfo with a type we should use
>> SAME_BINFO_TYPE_P.
>> > >
>> > > > +  /* 1.  If the "using" keyword is used to inherit DECL within a
>> baseclass,
>> > > > +     this may cause DECL to be private, so we return the using
>> statement as
>> > > > +     the source of the problem.
>> > > > +
>> > > > +     Scan the fields of PARENT_BINFO and see if there are any
>> using decls.  If
>> > > > +     there are, see if they inherit DECL.  If they do, that's
>> where DECL was
>> > > > +     truly declared private.  */
>> > > > +  for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (parent_binfo));
>> > > > +      parent_field;
>> > > > +      parent_field = DECL_CHAIN (parent_field))
>> > > > +    {
>> > > > +      if ((TREE_CODE (parent_field) == USING_DECL)
>> > >
>> > > This first term doesn't need to be wrapped in ().
>> > >
>> > > > +      && TREE_PRIVATE (parent_field))
>> > > > +     {
>> > > > +       if (DECL_NAME (decl) == DECL_NAME (parent_field))
>> > >
>> > > This could be part of the if above.  And then we can drop the braces
>> (in the
>> > > if and for both).
>> > >
>> > > > +         return parent_field;
>> > > > +     }
>> > > > +    }
>> > > > +
>> > > > +  /* 2.  If decl was privately inherited by a baseclass of the
>> current class,
>> > > > +     then decl will be inaccessible, even though it may originally
>> have
>> > > > +     been accessible to deriving classes.  In that case, the fault
>> lies with
>> > > > +     the baseclass that used a private inheritance, so we return
>> the
>> > > > +     baseclass type as the source of the problem.
>> > > > +
>> > > > +     Since this is the last check, we just assume it's true.  */
>> > > > +  return TYPE_NAME (BINFO_TYPE (parent_binfo));
>> > > > +}
>> > > > +
>> > > >  /* If the current scope isn't allowed to access DECL along
>> > > >     BASETYPE_PATH, give an error, or if we're parsing a function or
>> class
>> > > >     template, defer the access check to be performed at
>> instantiation time.
>> > > > @@ -317,34 +369,33 @@ enforce_access (tree basetype_path, tree
>> decl, tree diag_decl,
>> > > >       diag_decl = strip_inheriting_ctors (diag_decl);
>> > > >        if (complain & tf_error)
>> > > >       {
>> > > > -       /* We will usually want to point to the same place as
>> > > > -          diag_decl but not always.  */
>> > > > +       access_kind access_failure_reason = ak_none;
>> > > > +
>> > > > +       /* By default, using the decl as the source of the problem
>> will
>> > > > +          usually give correct results.  */
>> > > >         tree diag_location = diag_decl;
>> > > > -       access_kind parent_access = ak_none;
>> > > >
>> > > > -       /* See if any of BASETYPE_PATH's parents had private access
>> > > > -          to DECL.  If they did, that will tell us why we don't.
>> */
>> > > > +       /* However, if a parent of BASETYPE_PATH had private access
>> to decl,
>> > > > +          then it actually might be the case that the source of
>> the problem
>> > > > +          is not DECL.  */
>> > > >         tree parent_binfo = get_parent_with_private_access (decl,
>> > > > -
>>  basetype_path);
>> > > > +
>> basetype_path);
>> > > >
>> > > > -       /* If a parent had private access, then the diagnostic
>> > > > -          location DECL should be that of the parent class, since
>> it
>> > > > -          failed to give suitable access by using a private
>> > > > -          inheritance.  But if DECL was actually defined in the
>> parent,
>> > > > -          it wasn't privately inherited, and so we don't need to do
>> > > > -          this, and complain_about_access will figure out what to
>> > > > -          do.  */
>> > > > -       if (parent_binfo != NULL_TREE
>> > > > -           && (context_for_name_lookup (decl)
>> > > > -               != BINFO_TYPE (parent_binfo)))
>> > > > +       /* So if a parent did had private access, then we need to
>> do special
>> > >
>> > > Just "had" instead of "did had"?
>> > >
>> > > > +          checks to obtain the best diagnostic location decl.  */
>> > > > +       if (parent_binfo != NULL_TREE)
>> > > >           {
>> > > > -           diag_location = TYPE_NAME (BINFO_TYPE (parent_binfo));
>> > > > -           parent_access = ak_private;
>> > > > +           diag_location = get_class_access_diagnostic_decl
>> (parent_binfo,
>> > > > +
>> diag_decl);
>> > > > +
>> > > > +           /* We also at this point know that the reason access
>> failed was
>> > > > +              because decl was private.  */
>> > > > +              access_failure_reason = ak_private;
>> > >
>> > > Looks like this line is indented too much (even in the newer patch).
>> > >
>> > > >
>> > > >         /* Finally, generate an error message.  */
>> > > >         complain_about_access (decl, diag_decl, diag_location, true,
>> > > > -                              parent_access);
>> > > > +                             access_failure_reason);
>> > > >       }
>> > > >        if (afi)
>> > > >       afi->record_access_failure (basetype_path, decl, diag_decl);
>> > > > diff --git a/gcc/testsuite/g++.dg/pr19377.C
>> b/gcc/testsuite/g++.dg/pr19377.C
>> > > > new file mode 100644
>> > > > index 00000000000..fb023a33f82
>> > > > --- /dev/null
>> > > > +++ b/gcc/testsuite/g++.dg/pr19377.C
>> > > > @@ -0,0 +1,28 @@
>> > > > +/* { dg-do assemble } */
>> > >
>> > > We usually use dg-do compile.
>> > >
>> > > > +// { dg-options "-std=c++17" }
>> > >
>> > > Best to replace both with
>> > >
>> > > // { dg-do compile { target c++17 } }
>> > >
>> > > > +class A
>> > > > +{
>> > > > +  protected:
>> > > > +  int i();
>> > > > +};
>> > > > +
>> > > > +class A2
>> > > > +{
>> > > > +  protected:
>> > > > +  int i(int a);
>> > > > +};
>> > > > +
>> > > > +class B:private A, private A2
>> > > > +{
>> > > > +  // Comma separated list only officially supported in c++17 and
>> later.
>> > > > +  using A::i, A2::i; // { dg-message "declared" }
>> > > > +};
>> > > > +
>> > > > +class C:public B
>> > > > +{
>> > > > +  void f()
>> > > > +  {
>> > > > +    i(); // { dg-error "private" }
>> > > > +  }
>> > > > +};
>> > > > \ No newline at end of file
>> > > > --
>> > > > 2.25.1
>> > > >
>> > >
>> > > Marek
>> > >
>> >
>>
>> Marek
>>
>>
From bdf7f93ddbfac47e027259f63f7c9f8efaf4e13e Mon Sep 17 00:00:00 2001
From: Anthony Sharp <anthonyshar...@gmail.com>
Date: Sat, 6 Feb 2021 18:53:19 +0000
Subject: [PATCH] c++: Private parent access check for using decls [PR19377]

This bug was already mostly fixed by the patch for PR17314. This
patch continues that by ensuring that where a using decl is used,
causing an access failure to a child class because the using decl is
private, the compiler correctly points to the using decl as the
source of the problem.

Checks for the use of using decls in a parent that had
private access to decl (but the child had no access) in order
to ascertain the best diagnostic decl location.

gcc/cp/ChangeLog:

2021-02-06  Anthony Sharp  <anthonyshar...@gmail.com>

	* semantics.c (get_class_access_diagnostic_decl): New function for
	enforce_access.
	(enforce_access): Now checks using decls when encountering private
	parent in context of access failure.

gcc/testsuite/ChangeLog:

2021-02-06  Anthony Sharp  <anthonyshar...@gmail.com>

	* g++.dg/cpp1z/using9.C: New test.
---
 gcc/cp/semantics.c                  | 87 ++++++++++++++++++++++-------
 gcc/testsuite/g++.dg/cpp1z/using9.C | 29 ++++++++++
 2 files changed, 97 insertions(+), 19 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1z/using9.C

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 73834467fca..6a1ace85277 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -256,6 +256,56 @@ pop_to_parent_deferring_access_checks (void)
     }
 }
 
+/* Called from enforce_access.  A class has attempted (but failed) to access
+   DECL.  It is already established that a baseclass of that class,
+   PARENT_BINFO, has private access to DECL.  Examine certain sepcial cases
+   to find the decl that is the source of the problem.  If no special cases
+   are found, simply return DECL.  */
+
+static tree
+get_class_access_diagnostic_decl (tree parent_binfo, tree decl)
+{
+  /* When a class is denied access to a decl in a baseclass, most of the
+     time it is because the decl itself was declared as private at the point
+     of declaration.  So, by default, DECL is at fault.
+
+     However, in C++, there are (at least) two situations in which a decl
+     can be private even though it was not originally defined as such.  */
+
+  /* These two situations only apply if a baseclass had private access to
+     DECL (this function is only called if that is the case).  We must however
+     also ensure that the reason the parent had private access wasn't simply
+     because it was declared as private in the parent.  */
+  if (same_type_p (context_for_name_lookup (decl), BINFO_TYPE (parent_binfo)))
+    return decl;
+
+  /* 1.  If the "using" keyword is used to inherit DECL within a baseclass,
+     this may cause DECL to be private, so we return the using statement as
+     the source of the problem.
+
+     Scan the fields of PARENT_BINFO and see if there are any using decls.  If
+     there are, see if they inherit DECL.  If they do, that's where DECL was
+     truly declared private.  */
+  for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (parent_binfo));
+      parent_field;
+      parent_field = DECL_CHAIN (parent_field))
+    {
+      if ((TREE_CODE (parent_field) == USING_DECL)
+	 && TREE_PRIVATE (parent_field)
+	 && (DECL_NAME (decl) == DECL_NAME (parent_field)))
+	return parent_field;
+    }
+
+  /* 2.  If decl was privately inherited by a baseclass of the current class,
+     then decl will be inaccessible, even though it may originally have
+     been accessible to deriving classes.  In that case, the fault lies with
+     the baseclass that used a private inheritance, so we return the
+     baseclass type as the source of the problem.
+
+     Since this is the last check, we just assume it's true.  */
+  return TYPE_NAME (BINFO_TYPE (parent_binfo));
+}
+
 /* If the current scope isn't allowed to access DECL along
    BASETYPE_PATH, give an error, or if we're parsing a function or class
    template, defer the access check to be performed at instantiation time.
@@ -317,34 +367,33 @@ enforce_access (tree basetype_path, tree decl, tree diag_decl,
 	diag_decl = strip_inheriting_ctors (diag_decl);
       if (complain & tf_error)
 	{
-	  /* We will usually want to point to the same place as
-	     diag_decl but not always.  */
+	  access_kind access_failure_reason = ak_none;
+
+	  /* By default, using the decl as the source of the problem will
+	     usually give correct results.  */
 	  tree diag_location = diag_decl;
-	  access_kind parent_access = ak_none;
 
-	  /* See if any of BASETYPE_PATH's parents had private access
-	     to DECL.  If they did, that will tell us why we don't.  */
+	  /* However, if a parent of BASETYPE_PATH had private access to decl,
+	     then it actually might be the case that the source of the problem
+	     is not DECL.  */
 	  tree parent_binfo = get_parent_with_private_access (decl,
-							      basetype_path);
+							       basetype_path);
 
-	  /* If a parent had private access, then the diagnostic
-	     location DECL should be that of the parent class, since it
-	     failed to give suitable access by using a private
-	     inheritance.  But if DECL was actually defined in the parent,
-	     it wasn't privately inherited, and so we don't need to do
-	     this, and complain_about_access will figure out what to
-	     do.  */
-	  if (parent_binfo != NULL_TREE
-	      && (context_for_name_lookup (decl)
-		  != BINFO_TYPE (parent_binfo)))
+	  /* So if a parent did have private access, then we need to do
+	     special checks to obtain the best diagnostic location decl.  */
+	  if (parent_binfo != NULL_TREE)
 	    {
-	      diag_location = TYPE_NAME (BINFO_TYPE (parent_binfo));
-	      parent_access = ak_private;
+	      diag_location = get_class_access_diagnostic_decl (parent_binfo,
+							       diag_decl);
+
+	      /* We also at this point know that the reason access failed was
+		 because decl was private.  */
+	      access_failure_reason = ak_private;
 	    }
 
 	  /* Finally, generate an error message.  */
 	  complain_about_access (decl, diag_decl, diag_location, true,
-				 parent_access);
+				access_failure_reason);
 	}
       if (afi)
 	afi->record_access_failure (basetype_path, decl, diag_decl);
diff --git a/gcc/testsuite/g++.dg/cpp1z/using9.C b/gcc/testsuite/g++.dg/cpp1z/using9.C
new file mode 100644
index 00000000000..76965dbd0c6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/using9.C
@@ -0,0 +1,29 @@
+/* { dg-do assemble { target c++17 } } */
+// Assemble instead of compile since this code produces errors.
+// Created for c++ PR19377
+
+class A
+{
+  protected:
+  int i();
+};
+
+class A2
+{
+  protected:
+  int i(int a);
+};
+
+class B:private A, private A2
+{
+  // Comma separated list only officially supported in c++17 and later.
+  using A::i, A2::i; // { dg-message "declared" }
+};
+
+class C:public B
+{
+  void f()
+  {
+    i(); // { dg-error "private" }
+  }
+};
\ No newline at end of file
-- 
2.25.1

Reply via email to