Re: [PATCH] c++: private inheritance access diagnostics fix [PR17314]

[Anthony Sharp <anthonysharp15@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

Hi Jason,

I've finally completed my copyright assignment form. I've attached it
to this email for reference.

> You don't need write access to the main repository to use these commands
> on your local copy.  One nice thing about git compared to svn is that
> you don't need to touch the server for anything but push and pull.
>
> Incidentally, how are you producing your patch?  Maybe try git
> format-patch instead.

The method I am using at the moment is the one Ranjit Mathew talks
about here: http://rmathew.com/articles/gcj/crpatch.html. Actually,
having just re-read it, it says: 'NOTE: This is not the “proper” or
“official” way of creating and submitting patches - that process has
been explained in detail elsewhere. That process requires one to use
Subversion (SVN). The process described here is meant for “one-off
hackers” or people who cannot use SVN for some reason or the other.'
... oops!

It's my fault kind of - the official GCC webpage
(https://gcc.gnu.org/gitwrite.html) explaining how to do it is called
'Read-write Git access' so I assumed it was only relevant for people
who have access to the repo, but I see that is not the case.

I've tried the git way of doing it and I'm attaching a new patch file
that (hopefully) is better this time. Basically what I did was what
you suggested:

git pull
contrib/gcc-git-customization.sh
(make changes)
git add *
git gcc-commit-mklog
git gcc-commit-mklog --amend
git format-patch -1 master

I also re-built the source just to make sure I hadn't messed anything
up. I re-ran the C++ regression tests using make check-c and make
check-c++. Whilst I did not do a before/after comparison of the
results, I checked the FAILs in gcc.sum and g++.sum and they all
looked like they had nothing to do with my code. All the code is the
same as before, so I'm thinking it should be fine (I just wanted to be
safe). Also checked against check_GNU_style.sh.

Assuming that's all fine, as for the code itself, there might well be
some tweaks that could make it better, and so if that is the case then
please let me know.

Kind regards,
Anthony Sharp

[-- Attachment #2: Sharp.1672871.GCC.pdf --]
[-- Type: application/pdf, Size: 89622 bytes --]

[-- Attachment #3: pr17314_v2.patch --]
[-- Type: text/x-patch, Size: 16890 bytes --]

From 9f7fa0b4a892f717974d79a6a56a5f8a8a8d9943 Mon Sep 17 00:00:00 2001
From: Anthony Sharp <anthonysharp15@gmail.com>
Date: Thu, 21 Jan 2021 15:26:25 +0000
Subject: [PATCH] gcc/cp/ChangeLog:

	2021-1-21  Anthony Sharp  <anthonysharp15@gmail.com>
	Fixes PR17314
	* call.c (complain_about_access): Altered function.
	* cp-tree.h (complain_about_access): Changed parameters of function.
	* search.c (access_in_type): Made function non-static so it can be
	used in semantics.c.
	* semantics.c (access_in_type): Added as extern function.
	(get_parent_with_private_access): Added function.
	(enforce_access): Modified function.
	* typeck.c (complain_about_unrecognized_member): Updated function
	arguments in complain_about_access.

gcc/testsuite/ChangeLog:

	2021-1-21  Anthony Sharp  <anthonysharp15@gmail.com>
	Changes required due to PR17314 fix
	* g++.dg/lookup/scoped1.C: Modified testcase to run successfully with
	changes.
	* g++.dg/tc1/dr142.C: Same as above.
	* g++.dg/tc1/dr52.C: Same as above.
	* g++.old-deja/g++.brendan/visibility6.C: Same as above.
	* g++.old-deja/g++.brendan/visibility8.C: Same as above.
	* g++.old-deja/g++.jason/access8.C: Same as above.
	* g++.old-deja/g++.law/access4.C: Same as above.
	* g++.old-deja/g++.law/visibility12.C: Same as above.
	* g++.old-deja/g++.law/visibility4.C: Same as above.
	* g++.old-deja/g++.law/visibility8.C: Same as above.
	* g++.old-deja/g++.other/access4.C: Same as above.
---
 gcc/cp/call.c                                 | 64 ++++++++++-------
 gcc/cp/cp-tree.h                              |  3 +-
 gcc/cp/search.c                               |  4 +-
 gcc/cp/semantics.c                            | 68 ++++++++++++++++++-
 gcc/cp/typeck.c                               |  3 +-
 gcc/testsuite/g++.dg/lookup/scoped1.C         |  4 +-
 gcc/testsuite/g++.dg/tc1/dr142.C              |  8 +--
 gcc/testsuite/g++.dg/tc1/dr52.C               |  6 +-
 .../g++.old-deja/g++.brendan/visibility6.C    |  4 +-
 .../g++.old-deja/g++.brendan/visibility8.C    |  4 +-
 .../g++.old-deja/g++.jason/access8.C          |  5 +-
 gcc/testsuite/g++.old-deja/g++.law/access4.C  |  5 +-
 .../g++.old-deja/g++.law/visibility12.C       |  7 +-
 .../g++.old-deja/g++.law/visibility4.C        |  5 +-
 .../g++.old-deja/g++.law/visibility8.C        |  5 +-
 .../g++.old-deja/g++.other/access4.C          |  4 +-
 16 files changed, 145 insertions(+), 54 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index b6e9f125aeb..fc2f1c6226c 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7142,33 +7142,51 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
 /* Issue diagnostics about a disallowed access of DECL, using DIAG_DECL
    in the diagnostics.
 
-   If ISSUE_ERROR is true, then issue an error about the
-   access, followed by a note showing the declaration.
-   Otherwise, just show the note.  */
+   If ISSUE_ERROR is true, then issue an error about the access, followed
+   by a note showing the declaration.  Otherwise, just show the note.
+
+   DIAG_DECL and DIAG_LOCATION will almost always be the same.
+   DIAG_LOCATION is just another DECL.  NO_ACCESS_REASON is an optional
+   parameter used to specify why DECL wasn't accessible (e.g. ak_private
+   would be because DECL was private).  If not using NO_ACCESS_REASON,
+   then it must be ak_none, and the access failure reason will be
+   figured out by looking at the protection of DECL.  */
 
 void
-complain_about_access (tree decl, tree diag_decl, bool issue_error)
+complain_about_access (tree decl, tree diag_decl, tree diag_location,
+bool issue_error, access_kind no_access_reason)
 {
-  if (TREE_PRIVATE (decl))
-    {
-      if (issue_error)
-	error ("%q#D is private within this context", diag_decl);
-      inform (DECL_SOURCE_LOCATION (diag_decl),
-	      "declared private here");
-    }
-  else if (TREE_PROTECTED (decl))
-    {
-      if (issue_error)
-	error ("%q#D is protected within this context", diag_decl);
-      inform (DECL_SOURCE_LOCATION (diag_decl),
-	      "declared protected here");
-    }
+  /* If we have not already figured out why DECL is innaccessible...  */
+  if (no_access_reason == ak_none)
+  {
+    /* Examine the access of DECL to find out why.  */
+    if (TREE_PRIVATE (decl))
+      no_access_reason = ak_private;
+    else if (TREE_PROTECTED (decl))
+      no_access_reason = ak_protected;
+  }
+
+  /* Now generate an error message depending on calculated access.  */
+  if (no_access_reason == ak_private)
+  {
+    if (issue_error)
+      error ("%q#D is private within this context", diag_decl);
+    inform (DECL_SOURCE_LOCATION (diag_location), "declared private here");
+  }
+  else if (no_access_reason == ak_protected)
+  {
+    if (issue_error)
+      error ("%q#D is protected within this context", diag_decl);
+    inform (DECL_SOURCE_LOCATION (diag_location), "declared protected here");
+  }
+  /* Couldn't figure out why DECL is innaccesible, so just say it's
+  innaccessible.  */
   else
-    {
-      if (issue_error)
-	error ("%q#D is inaccessible within this context", diag_decl);
-      inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
-    }
+  {
+    if (issue_error)
+      error ("%q#D is inaccessible within this context", diag_decl);
+    inform (DECL_SOURCE_LOCATION (diag_decl), "declared here");
+  }
 }
 
 /* Initialize a temporary of type TYPE with EXPR.  The FLAGS are a
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 51139f4a4be..130f91b0ed2 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6434,7 +6434,8 @@ class access_failure_info
   tree m_diag_decl;
 };
 
-extern void complain_about_access		(tree, tree, bool);
+extern void complain_about_access		(tree, tree, tree, bool,
+						  access_kind);
 extern void push_defarg_context			(tree);
 extern void pop_defarg_context			(void);
 extern tree convert_default_arg			(tree, tree, tree, int,
diff --git a/gcc/cp/search.c b/gcc/cp/search.c
index dd3773da4f7..6cb881a20c2 100644
--- a/gcc/cp/search.c
+++ b/gcc/cp/search.c
@@ -47,7 +47,7 @@ static tree dfs_walk_once_accessible (tree, bool,
 				      tree (*post_fn) (tree, void *),
 				      void *data);
 static tree dfs_access_in_type (tree, void *);
-static access_kind access_in_type (tree, tree);
+access_kind access_in_type (tree, tree);
 static tree dfs_get_pure_virtuals (tree, void *);
 
 \f
@@ -583,7 +583,7 @@ dfs_access_in_type (tree binfo, void *data)
 
 /* Return the access to DECL in TYPE.  */
 
-static access_kind
+access_kind
 access_in_type (tree type, tree decl)
 {
   tree binfo = TYPE_BINFO (type);
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 244fc70d02d..a576489e3b9 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -46,6 +46,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "predict.h"
 #include "memmodel.h"
 
+extern access_kind access_in_type (tree type, tree decl);
+
 /* There routines provide a modular interface to perform many parsing
    operations.  They may therefore be used during actual parsing, or
    during template instantiation, which may be regarded as a
@@ -256,6 +258,41 @@ pop_to_parent_deferring_access_checks (void)
     }
 }
 
+/* This deals with bug PR17314.
+
+  DECL is a declaration and BINFO represents a class that has attempted (but
+  failed) to access DECL.
+
+  Examine the parent binfos of BINFO and determine whether any of them had
+  private access to DECL.  If they did, return the parent binfo.  This helps
+  in figuring out the correct error message to show (if the parents had
+  access, it's their fault for not giving sufficient access to BINFO).
+
+  If no parents had access, return NULL_TREE.  */
+
+static tree
+get_parent_with_private_access (tree decl, tree binfo)
+{
+  /* Only BINFOs should come through here.  */
+  gcc_assert (TREE_CODE (binfo) == TREE_BINFO);
+
+  tree base_binfo = NULL_TREE;
+
+  /* Iterate through immediate parent classes.  */
+  for (int i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
+  {
+	/* This parent had private access.  Therefore that's why BINFO can't
+	access DECL.  */
+	if (access_in_type (BINFO_TYPE (base_binfo), decl) == ak_private)
+	  return base_binfo;
+  }
+
+  /* None of the parents had access.  Note: it's impossible for one of the
+  parents to have had public or protected access to DECL, since then
+  BINFO would have been able to access DECL too.  */
+  return NULL_TREE;
+}
+
 /* 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.
@@ -316,7 +353,36 @@ enforce_access (tree basetype_path, tree decl, tree diag_decl,
       if (flag_new_inheriting_ctors)
 	diag_decl = strip_inheriting_ctors (diag_decl);
       if (complain & tf_error)
-	complain_about_access (decl, diag_decl, true);
+      {
+	/* We will usually want to point to the same place as
+	diag_decl but not always.  */
+	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.  */
+	tree parent_binfo = get_parent_with_private_access (decl,
+	  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))
+	{
+	  diag_location = TYPE_NAME (BINFO_TYPE (parent_binfo));
+	  parent_access = ak_private;
+	}
+
+	/* Finally, generate an error message.  */
+	complain_about_access (decl, diag_decl, diag_location, true,
+	  parent_access);
+	  }
       if (afi)
 	afi->record_access_failure (basetype_path, decl, diag_decl);
       return false;
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index cd592fd558e..0b3fd3785ae 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -2980,7 +2980,8 @@ complain_about_unrecognized_member (tree access_path, tree name,
 		    TREE_CODE (access_path) == TREE_BINFO
 		    ? TREE_TYPE (access_path) : object_type,
 		    name, afi.get_diag_decl ());
-	  complain_about_access (afi.get_decl (), afi.get_diag_decl (), false);
+	  complain_about_access (afi.get_decl (), afi.get_diag_decl (),
+	    afi.get_diag_decl (), false, ak_none);
 	}
     }
   else
diff --git a/gcc/testsuite/g++.dg/lookup/scoped1.C b/gcc/testsuite/g++.dg/lookup/scoped1.C
index 663f718b734..2f7f603b49f 100644
--- a/gcc/testsuite/g++.dg/lookup/scoped1.C
+++ b/gcc/testsuite/g++.dg/lookup/scoped1.C
@@ -4,12 +4,12 @@
 struct A
 {
   static int i1;
-  int i2; // { dg-message "declared" }
+  int i2; 
   static void f1 ();
   void f2 ();
 };
 
-struct B: private A { };
+struct B: private A { }; // { dg-message "declared" }
 struct C: public B
 {
   void g ()
diff --git a/gcc/testsuite/g++.dg/tc1/dr142.C b/gcc/testsuite/g++.dg/tc1/dr142.C
index 2f0370233e6..6e216da89f0 100644
--- a/gcc/testsuite/g++.dg/tc1/dr142.C
+++ b/gcc/testsuite/g++.dg/tc1/dr142.C
@@ -2,13 +2,13 @@
 // Origin: Giovanni Bajo <giovannibajo at gcc dot gnu dot org>
 // DR142: Injection-related errors in access example 
 
-class B {                 // { dg-message "declared" }
+class B {                 
 public:
-  int mi;                 // { dg-message "declared" }
-  static int si;          // { dg-message "declared" }
+  int mi;                 
+  static int si;          
 };
 
-class D: private B {
+class D: private B { // { dg-message "declared" }
 };
 
 class DD: public D {
diff --git a/gcc/testsuite/g++.dg/tc1/dr52.C b/gcc/testsuite/g++.dg/tc1/dr52.C
index 7cff847023f..17b6496916c 100644
--- a/gcc/testsuite/g++.dg/tc1/dr52.C
+++ b/gcc/testsuite/g++.dg/tc1/dr52.C
@@ -17,11 +17,11 @@ struct B1 : B {};
 struct B2 : B {};
 
 struct C
-{ // { dg-message "declared" }
-  void foo(void);
+{ 
+  void foo(void); 
 };
 
-struct D : private C {};
+struct D : private C {}; // { dg-message "declared" }
 
 struct X: A, B1, B2, D
 {
diff --git a/gcc/testsuite/g++.old-deja/g++.brendan/visibility6.C b/gcc/testsuite/g++.old-deja/g++.brendan/visibility6.C
index 3dfaf7fd0ea..8d6c6f10806 100644
--- a/gcc/testsuite/g++.old-deja/g++.brendan/visibility6.C
+++ b/gcc/testsuite/g++.old-deja/g++.brendan/visibility6.C
@@ -3,9 +3,9 @@
 class bottom
 {
 public:
-  int b; // { dg-message "" } private
+  int b; 
 };
-class middle : private bottom
+class middle : private bottom // { dg-message "" } private
 {
 public:
   void foo () { b; }
diff --git a/gcc/testsuite/g++.old-deja/g++.brendan/visibility8.C b/gcc/testsuite/g++.old-deja/g++.brendan/visibility8.C
index 3c443afe678..c165b0874a0 100644
--- a/gcc/testsuite/g++.old-deja/g++.brendan/visibility8.C
+++ b/gcc/testsuite/g++.old-deja/g++.brendan/visibility8.C
@@ -5,9 +5,9 @@
 class foo
 {
 public:
-  static int y; // { dg-message "" } private
+  static int y; 
 };
-class foo1 : private foo
+class foo1 : private foo // { dg-message "" } private
 { };
 class foo2 : public foo1
 { public:
diff --git a/gcc/testsuite/g++.old-deja/g++.jason/access8.C b/gcc/testsuite/g++.old-deja/g++.jason/access8.C
index 4404d8ad95d..0aa85d0457c 100644
--- a/gcc/testsuite/g++.old-deja/g++.jason/access8.C
+++ b/gcc/testsuite/g++.old-deja/g++.jason/access8.C
@@ -3,13 +3,14 @@
 // Date: 25 Jan 1994 23:41:33 -0500
 // Bug: g++ forgets access decls after the definition.
 
-class inh { // { dg-message "" } inaccessible
+class inh { 
         int a;
 protected:
         void myf(int);
 };
 
-class mel : private inh {
+class mel : private inh // { dg-message "" } inaccessible
+{
 protected:
         int t;
 	inh::myf;  // { dg-warning "deprecated" }
diff --git a/gcc/testsuite/g++.old-deja/g++.law/access4.C b/gcc/testsuite/g++.old-deja/g++.law/access4.C
index 54072ce3b32..57fa24a0dde 100644
--- a/gcc/testsuite/g++.old-deja/g++.law/access4.C
+++ b/gcc/testsuite/g++.old-deja/g++.law/access4.C
@@ -6,12 +6,13 @@
 // Subject:  g++ 2.5.5 doesn't warn about inaccessible virtual base ctor
 // Message-ID: <9403030024.AA04534@ses.com>
 
-class ForceLeafSterile { // { dg-message "" } 
+class ForceLeafSterile { 
     friend class Sterile;
       ForceLeafSterile() {} // { dg-message "" } 
 };
 
-class Sterile : private virtual ForceLeafSterile {
+class Sterile : private virtual ForceLeafSterile // { dg-message "" } 
+{
 public:
     Sterile() {}
     Sterile(const char* /*blah*/) {}
diff --git a/gcc/testsuite/g++.old-deja/g++.law/visibility12.C b/gcc/testsuite/g++.old-deja/g++.law/visibility12.C
index 59467ba7d80..6b7ff75d2ca 100644
--- a/gcc/testsuite/g++.old-deja/g++.law/visibility12.C
+++ b/gcc/testsuite/g++.old-deja/g++.law/visibility12.C
@@ -6,11 +6,12 @@
 // Subject:  member access rule bug
 // Message-ID: <9306300528.AA17185@coda.mel.dit.CSIRO.AU>
 struct a {
-  int aa; // { dg-message "" } private
+  int aa; 
         };
 
-class b : private a {
-        };
+class b : private a // { dg-message "" } private
+{
+};
 
 class c : public b {
         int xx(void) { return (aa); }  // aa should be invisible// { dg-error "" } .*
diff --git a/gcc/testsuite/g++.old-deja/g++.law/visibility4.C b/gcc/testsuite/g++.old-deja/g++.law/visibility4.C
index 1cdec1c2b55..644154e66fb 100644
--- a/gcc/testsuite/g++.old-deja/g++.law/visibility4.C
+++ b/gcc/testsuite/g++.old-deja/g++.law/visibility4.C
@@ -8,10 +8,11 @@
 
 class A {
 public:
-     int b; // { dg-message "" } private
+     int b; 
 };
 
-class C : private A {                   // NOTE WELL. private, not public
+class C : private A // { dg-message "" } private
+{                   
 public:
         int d;
 };
diff --git a/gcc/testsuite/g++.old-deja/g++.law/visibility8.C b/gcc/testsuite/g++.old-deja/g++.law/visibility8.C
index 5242dfc804f..4457ddf46c7 100644
--- a/gcc/testsuite/g++.old-deja/g++.law/visibility8.C
+++ b/gcc/testsuite/g++.old-deja/g++.law/visibility8.C
@@ -7,11 +7,12 @@
 // Message-ID: <m0nof3E-0021ifC@jts.com
 class t1 {
 protected:
-    int a; // { dg-message "" } protected
+    int a; 
 };
 
 
-class t2 : private t1 {
+class t2 : private t1 // { dg-message "" } protected
+{ 
 public:
     int b;
 };
diff --git a/gcc/testsuite/g++.old-deja/g++.other/access4.C b/gcc/testsuite/g++.old-deja/g++.other/access4.C
index d3c8d85c3f5..6c47700db19 100644
--- a/gcc/testsuite/g++.old-deja/g++.other/access4.C
+++ b/gcc/testsuite/g++.old-deja/g++.other/access4.C
@@ -1,10 +1,10 @@
 // { dg-do assemble  }
 
-struct A { // { dg-message "" } inaccessible
+struct A { 
   static int i;
 };
 
-struct B : private A { };
+struct B : private A { }; // { dg-message "" } inaccessible
 
 struct C : public B {
   int f () { return A::i; } // { dg-error "" } context
-- 
2.25.1