Sorry - last one had a formatting error. This one might be better.
Linux text editor doesn't consistently show whitespace for me!

On Fri, 5 Feb 2021 at 15:39, Anthony Sharp <anthonyshar...@gmail.com> wrote:
>
> > I'm imagining member functions, i.e. A::f() and A2::f(int).
>
> You're right - good spot.
>
> > == is enough, identifiers are unique.
>
>  Done.
>
> > Agreed, but the using-declaration might not be introducing DECL.  This
> > would be another way to skip irrelevant usings.
>
> Okie dokie.
>
> New patch attached (that rhymes?). C++ (compiled only) compiles fine
> and fixes the bug.
>
> Also modified the new regression test to use your overloaded function
> testcase. I made the testcase c++17 only ... I could also add a
> pre-c++17 testcase but not sure if that would really be beneficial?
> Anyways, regression tests (C++ only) now give:
>
> -- G++ --
>
> # of expected passes        203708
> # of unexpected failures    2
> # of expected failures        989
> # of unsupported tests        8714
>
> Anthony
>
>
> On Thu, 4 Feb 2021 at 20:55, Jason Merrill <ja...@redhat.com> wrote:
> >
> > On 2/4/21 12:46 PM, Anthony Sharp wrote:
> > >> Yes, thanks; it would take a lot to make me request less comments.
> > >
> > > Awesome.
> > >
> > >> The second lines of arguments are indented one space too far in both 
> > >> these calls.
> > >
> > > Oops! I will fix that.
> > >
> > >> Well, I guess it could be using a declaration of the same name from 
> > >> another base.
> > >
> > >   Yes I had been worrying about that.
> > >
> > >> But in that case you could end up with an overload set
> > >> containing both the decl we're complaining about and another of the same
> > >> name from another base, in which case the lookup result would include
> > >> both, and so the comparison would fail and we would fall through to the
> > >> private base assumption.
> > >
> > > I think technically yes ... but also no since such code would not
> > > compile anyways, plus oddly it seems to work anyway. For instance
> > > (assuming I'm understanding correctly), if you do this (with the patch
> > > applied):
> > >
> > > class A
> > > {
> > >    protected:
> > >    int i;
> > > };
> > >
> > > class A2
> > > {
> > >    protected:
> > >    int i;
> > > };
> > >
> > > class B:public A, public A2
> > > {
> > >    private:
> > >    using A::i, A2::i;
> > > };
> > >
> > > class C:public B
> > > {
> > >    void f()
> > >    {
> > >      A::i = 0;
> > >    }
> > > };
> > >
> > > You get:
> > >
> > > error: redeclaration of ‘using A::i’
> > >     using A::i;
> > >
> > > note: previous declaration ‘using A2::i’
> > >      using A2::i;
> > >
> > > error: redeclaration of ‘using A2::i’
> > >     using A::i, A2::i;
> > >
> > > previous declaration ‘using A::i’
> > >     using A::i, A2::i;
> > >
> > > In member function ‘void C::f()’:
> > > error: ‘int A::i’ is private within this context
> > >     A::i = 0;
> > >
> > > note: declared private here
> > >     using A::i, A2::i;
> > >
> > > Which seems to work (well ... more like fail to compile ... as
> > > expected). Maybe you're imagining a different situation to me?
> >
> > I'm imagining member functions, i.e. A::f() and A2::f(int).
> >
> > > You can even use void f() { i = 0; } and void f() { A2::i = 0; } and
> > > you seem to get the same results either which way (although, to be
> > > fair, if you do A2::i = 0, it suddenly doesn't complain about it being
> > > private anymore [no idea why], it just complains about the
> > > redeclaration , and once you fix the redeclaration, it THEN complains
> > > about being private, so it's got a bit of a quirk - don't think that's
> > > related to the patch though).
> >
> > That sounds fine, one error can hide another.
> >
> > >> But checking the name is a simple way to skip irrelevant usings.
> > >
> > > That does sound like a better way of doing it. Would I just do
> > > cp_tree_equal (DECL_NAME (blah1), DECL_NAME (blah2)) [assuming
> > > DECL_NAME returns a tree], or perhaps DECL_NAME (blah1) == DECL_NAME
> > > (blah2)?
> >
> > == is enough, identifiers are unique.
> >
> > >> Maybe also check that the using is TREE_PRIVATE?
> > >
> > > Would that be necessary? Maybe if you wanted to sanity-check it I
> > > suppose. We know for sure that PARENT_BINFO has private access to
> > > DECL. If we find a using statement introducing DECL in PARENT_BINFO,
> > > then surely the using statement must (by definition) have been
> > > private? If it were not private, then the child class would have been
> > > able to access it, and enforce_access wouldn't have thrown an error.
> > > It doesn't seem to be the case that DECL could be private for any
> > > other reason other than the using decl being private.
> >
> > Agreed, but the using-declaration might not be introducing DECL.  This
> > would be another way to skip irrelevant usings.
> >
> > > Let me know your thoughts and I will update the patch. Thanks for your 
> > > help.
> > >
> > > Anthony
> > >
> > >
> > > On Thu, 4 Feb 2021 at 16:33, Jason Merrill <ja...@redhat.com> wrote:
> > >>
> > >> On 2/4/21 11:24 AM, Jason Merrill wrote:
> > >>> On 2/4/21 10:02 AM, Anthony Sharp via Gcc-patches wrote:
> > >>>> Hello,
> > >>>>
> > >>>> New bugfix for PR19377
> > >>>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19377). This is
> > >>>> basically an extension of what I did before for PR17314 except it also
> > >>>> fixes this other bug.
> > >>>>
> > >>>> I hope I didn't over-comment in the code ... better to say too much
> > >>>> than too little! It's a niche bug so I thought it could do with a
> > >>>> little explanation.
> > >>>
> > >>> Yes, thanks; it would take a lot to make me request less comments.
> > >>>
> > >>>> +      if (TREE_CODE (parent_field) == USING_DECL)
> > >>>> +       {
> > >>>> +         if (cp_tree_equal (decl,
> > >>>> +                            lookup_member (parent_binfo,
> > >>>> +                                           DECL_NAME (parent_field),
> > >>>> +                                           /*protect=*/0,
> > >>>> +                                           /*want_type=*/false,
> > >>>> +                                           tf_warning_or_error)))
> > >>>
> > >>> Isn't it sufficient to check that the names match?
> > >>
> > >> Well, I guess it could be using a declaration of the same name from
> > >> another base.  But in that case you could end up with an overload set
> > >> containing both the decl we're complaining about and another of the same
> > >> name from another base, in which case the lookup result would include
> > >> both, and so the comparison would fail and we would fall through to the
> > >> private base assumption.
> > >>
> > >> But checking the name is a simple way to skip irrelevant usings.
> > >> Maybe also check that the using is TREE_PRIVATE?
> > >>
> > >>>>            tree parent_binfo = get_parent_with_private_access (decl,
> > >>>> -
> > >>>> basetype_path);
> > >>>> +
> > >>>> basetype_path);
> > >>> ...
> > >>>> +             diag_location = get_class_access_diagnostic_decl
> > >>>> (parent_binfo,
> > >>>> +
> > >>>> diag_decl);
> > >>>
> > >>> The second lines of arguments are indented one space too far in both
> > >>> these calls.
> > >>>
> > >>> Jason
> > >>
> > >
> >
From 8136df415670eb24edc73595fb6d4485d600a5d2 Mon Sep 17 00:00:00 2001
From: Anthony Sharp <anthonyshar...@gmail.com>
Date: Fri, 5 Feb 2021 15:30:45 +0000
Subject: [PATCH] c++: Private parent access check for using decl [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-05  Anthony Sharp  <anthonyshar...@gmail.com>

	* semantics.c (get_class_access_diagnostic_decl): New function.
	(enforce_access): Altered function.

gcc/testsuite/ChangeLog:

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

	* g++.dg/pr19377.C: New test.
---
 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
+   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 (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))
+	{
+	  if (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 +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
+	     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/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 } */
+// { dg-options "-std=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

Reply via email to