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

[Anthony Sharp <anthonysharp15@gmail.com>]

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

Hi Jason,

Thank you!

> To start with, do you have a copyright assignment on file or in the
> works already?

Good point. I incorrectly assumed it would only be a minor
contribution copyright-wise. Mr Edelsohn gave me a template which I've
now filled out and sent to assign@gnu.org. I'm assuming I just need to
wait for them to send me the form. I'll update this thread when that's
sorted. In the meantime I've hopefully fixed some of the issues.

> Second, your patch was mangled by word wrap so that it can't be applied
> without manual repair.  If you can't prevent word wrap in your mail
> client, please send it as an attachment rather than inline.

Oh yes I see where it's gotten mangled now. I'm attaching it as a
.patch file (I assume that's okay).

> Also, there are a few whitespace issues in the patch; please run
> contrib/check_GNU_style.sh on the patch before submitting.

Should be all fixed now (there is one style issue left but it's a
false positive). Visual Studio Code was lying to me about what the
file looks like so if there are any more formatting issues please let
me know.

> If you use contrib/gcc-git-customization.sh and then git
> gcc-commit-mklog you don't need to touch ChangeLog files at all, just
> adjust the generated ChangeLog entries in the git commit message.  I
> personally tend to commit first with a placeholder message and then use
> git gcc-commit-mklog --amend to generate the ChangeLog entries.

Wouldn't that require read-write access? (Just from looking here
https://gcc.gnu.org/gitwrite.html.)

> Probably.  Can you use sort/uniq/diff on the .sum testsuite output to
> determine which passes are missing in the patched sources?

According to contrib/dg-cmp-results.sh ...

I get a bunch of these weird NA->PASSes (and vice-versa), for example:

PASS->NA: g++.dg/modules/alias-1_a.H module-cmi
(gcm.cache/home/anthony/Desktop/GCC/builds_and_source/source_clean/gcc/testsuite/g++.dg/modules/alias-1_a.H.gcm)
NA->PASS: g++.dg/modules/alias-1_a.H module-cmi
(gcm.cache/home/anthony/Desktop/GCC/builds_and_source/source_pr17314/gcc/testsuite/g++.dg/modules/alias-1_a.H.gcm)
PASS->NA: g++.dg/modules/alias-1_a.H module-cmi
(gcm.cache/home/anthony/Desktop/GCC/builds_and_source/source_clean/gcc/testsuite/g++.dg/modules/alias-1_a.H.gcm)
NA->PASS: g++.dg/modules/alias-1_a.H module-cmi
(gcm.cache/home/anthony/Desktop/GCC/builds_and_source/source_pr17314/gcc/testsuite/g++.dg/modules/alias-1_a.H.gcm)

They're weird because I haven't actually touched those files (so I'm
assuming this is normal). There are about ~400 of those and they're
all .gcm files. They seem to balance out.

dr142.c reports:

NA->PASS: g++.dg/tc1/dr142.C  -std=c++14  (test for warnings, line 11)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++14  (test for warnings, line 5)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++14  (test for warnings, line 7)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++14  (test for warnings, line 8)
NA->PASS: g++.dg/tc1/dr142.C  -std=c++17  (test for warnings, line 11)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++17  (test for warnings, line 5)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++17  (test for warnings, line 7)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++17  (test for warnings, line 8)
NA->PASS: g++.dg/tc1/dr142.C  -std=c++2a  (test for warnings, line 11)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++2a  (test for warnings, line 5)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++2a  (test for warnings, line 7)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++2a  (test for warnings, line 8)
NA->PASS: g++.dg/tc1/dr142.C  -std=c++98  (test for warnings, line 11)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++98  (test for warnings, line 5)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++98  (test for warnings, line 7)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++98  (test for warnings, line 8)

In other words, there are 12 PASS->NAs and 4 NA->PASSes in this file,
meaning a net change of -8 (which explains why there are eight fewer).
My other changes also report PASS->NAs and vice-versa, but for those
the number of new NAs equals the number of new PASSes, so they don't
cause a change in quantity.

Thanks for being patient with me. I'll let you know when I've
completed the forms.

Also if I need to adjust the .patch to deal with the changelogs issue
please let me know.

Kind regards,
Anthony

[-- Attachment #2: pr17314.patch --]
[-- Type: text/x-patch, Size: 15991 bytes --]

Index: gcc/testsuite/g++.old-deja/g++.jason/access8.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.jason/access8.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.old-deja/g++.jason/access8.C	2021-01-03 00:22:14.969854000 +0000
@@ -4,5 +4,5 @@
 // Bug: g++ forgets access decls after the definition.
 
-class inh { // { dg-message "" } inaccessible
+class inh { 
         int a;
 protected:
@@ -10,5 +10,6 @@ protected:
 };
 
-class mel : private inh {
+class mel : private inh // { dg-message "" } inaccessible
+{
 protected:
         int t;
Index: gcc/testsuite/g++.old-deja/g++.law/access4.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.law/access4.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.old-deja/g++.law/access4.C	2021-01-03 00:21:01.736320000 +0000
@@ -7,10 +7,11 @@
 // 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() {}
Index: gcc/testsuite/g++.old-deja/g++.law/visibility12.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.law/visibility12.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.old-deja/g++.law/visibility12.C	2021-01-03 00:20:24.243535000 +0000
@@ -7,9 +7,10 @@
 // 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 {
Index: gcc/testsuite/g++.old-deja/g++.law/visibility8.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.law/visibility8.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.old-deja/g++.law/visibility8.C	2021-01-03 00:19:45.574725000 +0000
@@ -8,9 +8,10 @@
 class t1 {
 protected:
-    int a; // { dg-message "" } protected
+    int a; 
 };
 
 
-class t2 : private t1 {
+class t2 : private t1 // { dg-message "" } protected
+{ 
 public:
     int b;
Index: gcc/testsuite/g++.old-deja/g++.law/visibility4.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.law/visibility4.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.old-deja/g++.law/visibility4.C	2021-01-03 00:20:06.815170000 +0000
@@ -9,8 +9,9 @@
 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;
Index: gcc/testsuite/g++.old-deja/g++.other/access4.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.other/access4.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.old-deja/g++.other/access4.C	2021-01-03 00:19:25.362302000 +0000
@@ -1,9 +1,9 @@
 // { 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 {
Index: gcc/testsuite/g++.old-deja/g++.brendan/visibility8.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.brendan/visibility8.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.old-deja/g++.brendan/visibility8.C	2021-01-03 00:19:17.918146000 +0000
@@ -6,7 +6,7 @@ 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
Index: gcc/testsuite/g++.old-deja/g++.brendan/visibility6.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.brendan/visibility6.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.old-deja/g++.brendan/visibility6.C	2021-01-03 00:21:55.873454000 +0000
@@ -4,7 +4,7 @@ class bottom
 {
 public:
-  int b; // { dg-message "" } private
+  int b; 
 };
-class middle : private bottom
+class middle : private bottom // { dg-message "" } private
 {
 public:
Index: gcc/testsuite/g++.dg/lookup/scoped1.C
===================================================================
--- gcc/testsuite/g++.dg/lookup/scoped1.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.dg/lookup/scoped1.C	2021-01-02 21:51:30.088674000 +0000
@@ -5,10 +5,10 @@ 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
 {
Index: gcc/testsuite/g++.dg/tc1/dr142.C
===================================================================
--- gcc/testsuite/g++.dg/tc1/dr142.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.dg/tc1/dr142.C	2021-01-02 21:54:09.839546000 +0000
@@ -3,11 +3,11 @@
 // 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" }
 };
 
Index: gcc/testsuite/g++.dg/tc1/dr52.C
===================================================================
--- gcc/testsuite/g++.dg/tc1/dr52.C	2020-12-31 16:51:34.000000000 +0000
+++ gcc/testsuite/g++.dg/tc1/dr52.C	2021-01-03 13:20:08.267946000 +0000
@@ -18,9 +18,9 @@ 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
Index: gcc/testsuite/ChangeLog
from  Anthony Sharp  <anthonysharp15@gmail.com>

	Fixes PR17314
	* g++.dg/lookup/scoped1.c modified testcase to run successfully with
	changes.
	* g++.dg/tc1/dr142.c modified testcase to run successfully with 
	changes.
	* g++.dg/tc1/dr142.c modified testcase to run successfully with 
	changes.
	* g++.dg/tc1/dr142.c modified testcase to run successfully with 
	changes.
	* g++.dg/tc1/dr52.c modified testcase to run successfully with changes.
	* g++.old-deja/g++.brendan/visibility6.c modified testcase to run 
	successfully with changes.
	* g++.old-deja/g++.brendan/visibility8.c modified testcase to run 
	successfully with changes.
	* g++.old-deja/g++.jason/access8.c modified testcase to run 
	successfully with changes.
	* g++.old-deja/g++.law/access4.c modified testcase to run successfully 
	with changes.
	* g++.old-deja/g++.law/visibility12.c modified testcase to run 
	successfully with changes.
	* g++.old-deja/g++.law/visibility4.c modified testcase to run 
	successfully with changes.
	* g++.old-deja/g++.law/visibility8.c modified testcase to run 
	successfully with changes.
	* g++.old-deja/g++.other/access4.c modified testcase to run 
	successfully with changes.
Index: gcc/cp/semantics.c
===================================================================
--- gcc/cp/semantics.c	2020-12-31 16:51:34.000000000 +0000
+++ gcc/cp/semantics.c	2021-01-09 00:03:36.308446274 +0000
@@ -47,4 +47,6 @@ along with GCC; see the file COPYING3.
 #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
@@ -257,4 +259,39 @@ pop_to_parent_deferring_access_checks (v
 }
 
+/* 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
@@ -315,9 +352,38 @@ enforce_access (tree basetype_path, tree
     {
       if (flag_new_inheriting_ctors)
-	diag_decl = strip_inheriting_ctors (diag_decl);
+	    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);
+	    afi->record_access_failure (basetype_path, decl, diag_decl);
       return false;
     }
Index: gcc/cp/cp-tree.h
===================================================================
--- gcc/cp/cp-tree.h	2020-12-31 16:51:34.000000000 +0000
+++ gcc/cp/cp-tree.h	2021-01-08 22:45:02.106467958 +0000
@@ -6435,5 +6435,6 @@ class access_failure_info
 };
 
-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);
Index: gcc/cp/search.c
===================================================================
--- gcc/cp/search.c	2020-12-31 16:51:34.000000000 +0000
+++ gcc/cp/search.c	2021-01-01 01:33:56.788180000 +0000
@@ -48,5 +48,5 @@ static tree dfs_walk_once_accessible (tr
 				      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 *);
 
@@ -584,5 +584,5 @@ dfs_access_in_type (tree binfo, void *da
 /* Return the access to DECL in TYPE.  */
 
-static access_kind
+access_kind
 access_in_type (tree type, tree decl)
 {
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	2020-12-31 16:51:34.000000000 +0000
+++ gcc/cp/typeck.c	2021-01-01 01:37:13.602815000 +0000
@@ -2981,5 +2981,6 @@ complain_about_unrecognized_member (tree
 		    ? 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);
 	}
     }
Index: gcc/cp/call.c
===================================================================
--- gcc/cp/call.c	2020-12-31 16:51:34.000000000 +0000
+++ gcc/cp/call.c	2021-01-09 00:08:05.249597968 +0000
@@ -7133,31 +7133,49 @@ build_op_delete_call (enum tree_code cod
    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");
+  }
 }
 
Index: gcc/cp/ChangeLog
from  Anthony Sharp  <anthonysharp15@gmail.com>

	Fixes PR17314
	* typeck.c (complain_about_unrecognized_member): Updated function 
	arguments in complain_about_access.
	* call.c (complain_about_access): Altered function.
	* semantics.c (get_parent_with_private_access): Added function.
	(access_in_type): Added as extern function.
	* search.c (access_in_type): Made function non-static so it can be 
	used in semantics.c.
	* cp-tree.h (complain_about_access): Changed parameters of function.