Hi all,
Here is the patch as promised. Regression tested on the c++ side and
everything seems okay. Compiles fine.
Sounds good, though strip_using_decl (parent_field) may be overloaded if
> the using-decl brings in multiple functions with that name.
Could you give my new regression test a quick glance over then just to make
sure I've not forgotten about something? It definitely seems to work but
I'm no expert in all the different ways using statements can be
constructed. If you were to use 'using comma' (in the testcase), it
generates another error because it's ambiguous, so that's okay. And if you
use a comma-separated list like I have in the test (i.e. using A2::comma,
A1::comma) it seems to find the correct bits just fine. Unless I'm getting
really lucky and it's actually just a coincidence.
It seems that strip_using_decl doesn't return an overloaded list. Or, if it
does, the first entry in that list just so happens to always be the right
answer, so it can be treated like it's a regular decl for this purpose. For
example, even if we change up the order of baseclasses, using statements
and class definitions, it still seems to work, e.g. the following *seems*
to work just fine:
class A2
{
protected:
int separate(int a);
int comma(int a);
int alone;
};
class A1
{
protected:
int separate();
int comma();
};
class A3
{
protected:
int comma(int a, int b);
};
class B:private A3, private A1, private A2
{
// Using decls in a comma-separated list.
using A2::comma, A3::comma, A1::comma; // { dg-message "declared" }
// Separate using statements.
using A2::separate;
using A1::separate; // { dg-message "declared" }
// No ambiguity, just for the sake of it.
using A2::alone; // { dg-message "declared" }
};
class C:public B
{
void f()
{
comma(); // { dg-error "private" }
separate(); // { dg-error "private" }
alone = 5; // { dg-error "private" }
}
};
Anthony
On Thu, 25 Feb 2021 at 03:37, Jason Merrill <ja...@redhat.com> wrote:
> On 2/24/21 4:17 PM, Anthony Sharp wrote:
> >> "special"
> >
> >
> > It wouldn't be my code if it didn't have sp3ling mstakes innit!
> > Actually to be fair I already changed that spelling mistake a few days
> > ago in my local code ;)
> >
> > I was actually thinking about this last night as I was falling asleep
> > (as you do) and I realised that the whole of my using decl lookup is
> > redundant. I can simply do this (formatting probably messes up here):
> >
> > /* 1. If the "using" keyword is used to inherit DECL within the parent,
> > this may cause DECL to be private, so we should 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 must
> > have been declared private. */
> >
> > for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (parent_binfo));
> > parent_field;
> > parent_field = DECL_CHAIN (parent_field))
> > {
> > /* Not necessary, but also check TREE_PRIVATE for the sake of
> > eliminating obviously non-relevant using decls. */
> > if (TREE_CODE (parent_field) == USING_DECL
> > && TREE_PRIVATE (parent_field))
> > {
> > /* If the using statement inherits DECL, it is the source of the
> > access failure, so return it. */
> > if (cp_tree_equal (strip_using_decl (parent_field), decl))
> > return parent_field;
> > }
> > }
> >
> > I was wrong to say that the using decl does not store "where it came
> > from/what it inherits" - that's exactly what strip_using_decl
> > achieves. I think the problem was that when I did my initial testing
> > in trying out ways to get the original decl, I didn't strip it, so the
> > comparison failed, which led me to make the whole redundant lookup,
> > blah blah blah.
> >
> > I've run a quick test and it seems to work, even with the overloads.
> >
> > Will test it some more and if all's good I will probably send a new
> > patch some time this weekend.
>
> Sounds good, though strip_using_decl (parent_field) may be overloaded if
> the using-decl brings in multiple functions with that name.
>
> Jason
>
>
From b57afd2da59c2ec14c13b76c39aba9126170078d Mon Sep 17 00:00:00 2001
From: Anthony Sharp <anthonyshar...@gmail.com>
Date: Mon, 1 Mar 2021 21:58:35 +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.
gcc/cp/ChangeLog:
2021-03-01 Anthony Sharp <anthonyshar...@gmail.com>
* semantics.c (get_class_access_diagnostic_decl): New
function that examines special cases when a parent
class causes a private access failure.
(enforce_access): Slightly modified to call function
above.
gcc/testsuite/ChangeLog:
2021-03-01 Anthony Sharp <anthonyshar...@gmail.com>
* g++.dg/cpp1z/using9.C: New using decl test.
---
gcc/cp/semantics.c | 98 +++++++++++++++++++++++------
gcc/testsuite/g++.dg/cpp1z/using9.C | 38 +++++++++++
2 files changed, 118 insertions(+), 18 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..9d8f7d85ee8 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -256,6 +256,69 @@ 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 find a decl that accurately describes the source of the problem. If
+ none of the special cases apply, simply return DECL as the source of the
+ problem. */
+
+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.
+
+ 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 should first check whether the reason the parent had private access
+ to DECL was simply because DECL was created and declared as private in
+ the parent. If it was, then DECL is definitively the source of the
+ problem. */
+ if (SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
+ BINFO_TYPE (parent_binfo)))
+ return decl;
+
+ /* 1. If the "using" keyword is used to inherit DECL within the parent,
+ this may cause DECL to be private, so we should 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 must
+ have been declared private. */
+
+ for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (parent_binfo));
+ parent_field;
+ parent_field = DECL_CHAIN (parent_field))
+ {
+ /* Not necessary, but also check TREE_PRIVATE for the sake of
+ eliminating obviously non-relevant using decls. */
+ if (TREE_CODE (parent_field) == USING_DECL
+ && TREE_PRIVATE (parent_field))
+ {
+ /* If the using statement inherits DECL, it is the source of the
+ access failure, so return it. */
+ if (cp_tree_equal (strip_using_decl (parent_field), decl))
+ return parent_field;
+ }
+ }
+
+ /* 2. If DECL was privately inherited by the parent 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 parent, since it
+ used a private inheritance, so we return the parent as the source of the
+ problem.
+
+ Since this is the last check, we just assume it's true. At worst, it
+ will simply point to the class that failed to give access, which is
+ technically 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 +380,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);
- /* 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..6aac79c32f6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/using9.C
@@ -0,0 +1,38 @@
+/* { dg-do compile { target c++17 } } */
+// Created for c++ PR19377
+
+class A2
+{
+ protected:
+ int separate(int a);
+ int comma(int a);
+ int alone;
+};
+
+class A1
+{
+ protected:
+ int separate();
+ int comma();
+};
+
+class B:private A1, private A2
+{
+ // Using decls in a comma-separated list.
+ using A2::comma, A1::comma; // { dg-message "declared" }
+ // Separate using statements.
+ using A2::separate;
+ using A1::separate; // { dg-message "declared" }
+ // No ambiguity, just for the sake of it.
+ using A2::alone; // { dg-message "declared" }
+};
+
+class C:public B
+{
+ void f()
+ {
+ comma(); // { dg-error "private" }
+ separate(); // { dg-error "private" }
+ alone = 5; // { dg-error "private" }
+ }
+};
--
2.25.1
