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

[Jason Merrill via Gcc-patches]

On 1/5/21 9:24 AM, Anthony Sharp via Gcc-patches wrote:
This patch fixes PR17314 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17314).
Previously, when class C attempted to access member a declared in class A
through class B, where class B privately inherits from A and class C inherits
from B, GCC would correctly report an access violation, but would erroneously
report that the reason was because a was "protected", when in fact, from the
point of view of class C, it was "private". This patch updates the
diagnostics code to generate more correct errors in cases of failed
inheritance such as these.
The reason this bug happened was because GCC was examining the
declared access of decl, instead of looking at it in the context of class
inheritance.

----------- COMMENTS -----------

This is my first GCC patch ever

Thanks, and welcome!

To start with, do you have a copyright assignment on file or in the 
works already?  If not, see the top of

  https://gcc.gnu.org/contribute.html

for more information.  I probably shouldn't look at the patch in detail 
until that's addressed.

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.

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

so there is probably something I have done
very wrong. Please let me know :) The thought of my code being scrutinised
by people with PhDs and doctorates is quite frankly terrifying.

Note that since it is a new year I had to make a new changelog file so the
diff for the patch might be slightly off.

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.

There was no need to add additional regression tests since it was adequate
to simply change some of the regression tests that were there originally
(all the patch changes is the informative message telling the user where
a decl was defined as private).

Agreed.

----------- REGRESSION ANALYSIS -----------

No regressions reported.

G++ (CLEAN) RESULTS

# of expected passes        202879
# of unexpected failures    1
# of expected failures        988
# of unsupported tests        8654

GCC (CLEAN) RESULTS

# of expected passes        163377
# of unexpected failures    94
# of unexpected successes    37
# of expected failures        915
# of unsupported tests        2530

G++ (PR17314 PATCHED) RESULTS

# of expected passes        202871
# of unexpected failures    1
# of expected failures        988
# of unsupported tests        8654

GCC (PR17314 PATCHED) RESULTS

# of expected passes        163377
# of unexpected failures    94
# of unexpected successes    37
# of expected failures        915
# of unsupported tests        2530

When I build and make -k check -j 6 on the patched source it reports
202871 passes (8 fewer), although the FAILs do not increase. I am not 100%
sure why this happens since I have not removed any testcases, only edited a
few, but I think this happens because in files like dr142.c I removed more
output checks than I added. make -k check -j 6 also returns error 2
sometimes, although there are no obvious errors or warnings in the logs
explaining why. Probably harmless?

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

----------- BUILD REPORT -----------

GCC builds normally on x86_64-pc-linux-gnu for x86_64-pc-linux-gnu using
make -j 6. I didn't see it necessary to test on other build targets since the
patch only affects the C++ front end and so functionality is unlikely
to differ between platforms.

The compile log reports:

Comparing stages 2 and 3
warning: gcc/cc1obj-checksum.o differs
Comparison successful.

and then continues. I assume this means it was actually successful.

Yes.

Thanks,
Jason

Index: gcc/cp/ChangeLog
from  Anthony Sharp  <anthonyshar...@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.
Index: gcc/testsuite/ChangeLog
from  Anthony Sharp  <anthonyshar...@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/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.aa04...@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.aa17...@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/cp/semantics.c
===================================================================
--- gcc/cp/semantics.c    2020-12-31 16:51:34.000000000 +0000
+++ gcc/cp/semantics.c    2021-01-04 20:26:51.912302840 +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,34 @@ 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-01 01:33:03.194982000 +0000
@@ -6435,5 +6435,5 @@ 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-04 19:40:18.564108581 +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");
+  }
  }