Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Jason Merrill via Gcc-patches]

On 3/10/21 4:14 PM, Anthony Sharp wrote:

Hiya


That's because none of the names are overloaded within a single base
class.


Ah, thanks. Thought there must be something I wasn't thinking of.


Also, you can use == instead of cp_tree_equal for comparing FUNCTION_DECLs.


Changed it.

Latest patch is attached. Compiles fine and no regressions.


Great!  You may have already noticed that I applied the patch with a 
little simplification: we can use ovl_iterator for non-overloaded decls 
as well.


Thanks,
Jason

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

Hiya

> That's because none of the names are overloaded within a single base
> class.

Ah, thanks. Thought there must be something I wasn't thinking of.

> Also, you can use == instead of cp_tree_equal for comparing FUNCTION_DECLs.

Changed it.

Latest patch is attached. Compiles fine and no regressions.

Anthony
From 0b2dd8d8818c6e23b3c8d1b0de7b71c1bb86e6c3 Mon Sep 17 00:00:00 2001
From: Anthony Sharp 
Date: Wed, 10 Mar 2021 20:36:03 +
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-10  Anthony Sharp  

	* 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-10  Anthony Sharp  

	* g++.dg/cpp1z/using9.C: New using decl test.
---
 gcc/cp/semantics.c  | 110 +++-
 gcc/testsuite/g++.dg/cpp1z/using9.C |  49 +
 2 files changed, 141 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..f1a4f102408 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -256,6 +256,81 @@ 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))
+	{
+	  tree decl_stripped = strip_using_decl (parent_field);
+
+	  if (is_overloaded_fn (decl_stripped))
+	  {
+	/* The using statement might be overloaded.  If so, we need to
+	   check all of the overloads.  */
+	for (ovl_iterator iter (decl_stripped); iter; ++iter)
+	{
+	  /* If equal, the using statement inherits DECL, and so is the
+	  source of the access failure, so return it.  */
+	  if (*iter == decl)
+		return parent_field;
+	}
+	  }
+	  else if (decl_stripped == 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 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Jason Merrill via Gcc-patches]

On 3/1/21 5:11 PM, Anthony Sharp wrote:

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:


That's because none of the names are overloaded within a single base 
class.  But if I add



class A2
{
   protected:
   int separate(int a);
   int comma(int a);
   int alone;
};

class A1
{
   protected:
   int separate();

 int separate(int,int,int);

then strip_using_decl for A1::separate gives an OVERLOAD.

You can iterate over the result of strip_using decl with the

for (ovl_iterator iter (fns); iter; ++iter)
  {
tree fn = *iter;

pattern.

Also, you can use == instead of cp_tree_equal for comparing FUNCTION_DECLs.

Jason

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

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  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 
Date: Mon, 1 Mar 2021 21:58:35 +
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  

	* 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:

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Jason Merrill via Gcc-patches]

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

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

> "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.

> I was thinking you could walk through the overload set to see if it
> contains DECL.

I did try that ... sort of. I did a name lookup on the using decl and
that returned a baselink (no idea why, since the lookup function says
it returns a tree list [probably me being dumb]), which then gave me a
bunch of overloads. But that didn't seem to help since if multiple
using decls give me the answer I'm looking for (a match for DECL)
because they were overloaded, then there was no way for me to tell
which using decl was actually the correct one. Kind of like if three
cakes are equally as tasty, then how are you supposed to tell which
one is the most delicious?

Anthony

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Jason Merrill via Gcc-patches]

On 2/15/21 7:45 AM, Anthony Sharp wrote:

PARENT_BINFO, has private access to DECL.  Examine certain sepcial cases


"special"


I did try the name
lookup as Jason suggested but, as I say, in the case of overloaded
functions it returns multiple values, so it didn't help in determining
what DECL a USING_DECL inherited.


I was thinking you could walk through the overload set to see if it 
contains DECL.


Jason

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

Scan the fields of BINFO for an exact match of DECL.  If found, return
   DECL.  Otherwise, return NULL_TREE.  DECL is really the type "tree". */

Should say

Scan the fields of BINFO for an exact match of DECL.  If found, return
   the field.  Otherwise, return NULL_TREE.  DECL is really the type "tree". */

On Mon, 15 Feb 2021 at 12:45, Anthony Sharp  wrote:
>
> Hi all,
>
> This overloaded functions issue has been a pain in the ass but I have
> found a solution that seems to work. I could really do with some
> feedback on it though since I'm not sure if there's a better way to do
> it that I am missing.
>
> Here's the problem. When a using statement causes an access failure,
> we need to find out WHICH using statement caused the access failure
> (this information is not given to enforce_access; it merely gets the
> ORIGINAL decl [i.e. the one that the using statement inherited]). To
> make matters worse, a USING_DECL does not seem to store any
> information about where it "came from", e.g. there's no ORIGINAL_DECL
> (USING_DECL) macro or anything like that. DECL_INITIAL doesn't seem to
> help (and is probably totally not related to the issue).
>
> So we need to do a long-winded lookup.
>
> First, we iterate through the USING_DECLs in the class where access
> failed (in the context of a parent that has caused the access failure
> because it has private access). For normal field variables, we COULD
> simply do a name lookup on the USING_DECL and compare it to the
> original DECL. That would be easy, since there can only be one
> variable field with the same name.
>
> However, that doesn't work for overloaded functions. Name lookup would
> return multiple results, making comparison meaningless.
>
> What we need is therefore not a name lookup, but a decl lookup. It
> sounds stupid, because what that basically means is looking for an
> exact match of a decl, which is intuitively stupid, because why would
> you look for an exact match of something you already have? But
> actually we can take advantage of a quirk that USING_DECLs have, which
> is that, when stripped, cp_tree_equal (stripped_using_decl,
> original_decl) returns true, even through stripped_using_decl and
> original_decl exist on different lines and in different places. In
> other words, a USING_DECL is the same as the original DECL it
> inherits, even though they are in different places (Unless I am just
> being really dumb?).
>
> Anyways, to summarise ...
>
> 1) We iterate through USING_DECLs in the class that caused a private
> access failure.
> 2) For each USING_DECL, we find the DECL that USING_DECL inherited.
>   2.1) To do this, we iterate through all fields of all base classes
> (possibly slow? but it is just diagnostics code afterall,
>  so idk if that's a big deal)
>   2.2)  We compare each FIELD against the USING_DECL. if equal, then
> we return FIELD.
> 3) if the DECL that USING_DECL inherited is equal to the diagnostic
> decl we were given in enforce_access, we return USING_DECL as being
> the source of the problem.
>
> In a perfect world, I guess the USING_DECL would store somewhere what
> it inherited as it was parsed. But I'm not sure if that's practical to
> do and I'm not sure it would be worth the effort considering it would
> probably be used only for this niche scenario. Donno.
>
> I have not regression tested this, but it does seem to work on the
> test case at least (which I've also included). Also please ignore
> formatting issues ATM.
>
> If you think this is a reasonable way to do it then I will tidy up the
> code, test it and make a patch and send it over. If anyone has any
> better ideas (probably), then please let me know. I did try the name
> lookup as Jason suggested but, as I say, in the case of overloaded
> functions it returns multiple values, so it didn't help in determining
> what DECL a USING_DECL inherited.
>
> BTW, I will eventually put find_decl_using_decl_inherits and
> lookup_decl_exact_match in search.c.
>
> Just for proof, here's the output from the testcase (hopefully it
> formats this correctly):
>
> /home/anthony/Desktop/using9.C: In member function ‘void C::f()’:
> /home/anthony/Desktop/using9.C:34:11: error: ‘int A1::comma()’ is
> private within this context
>34 | comma();
>   |   ^
> /home/anthony/Desktop/using9.C:22:24: note: declared private here
>22 |   using A2::comma, A1::comma;  // { dg-message "declared" }
>   |   ^
> /home/anthony/Desktop/using9.C:35:14: error: ‘int A1::separate()’ is
> private within this context
>35 | separate(); // { dg-error "private" }
>   | ^
> /home/anthony/Desktop/using9.C:25:13: note: declared private here
>25 |   using A1::separate; // { dg-message "declared" }
>   |  ^~~~
> /home/anthony/Desktop/using9.C:36:5: error: ‘int A2::alone’ is private
> within this context
>36 | alone = 5; // { dg-error "private" }
> 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

Hi all,

This overloaded functions issue has been a pain in the ass but I have
found a solution that seems to work. I could really do with some
feedback on it though since I'm not sure if there's a better way to do
it that I am missing.

Here's the problem. When a using statement causes an access failure,
we need to find out WHICH using statement caused the access failure
(this information is not given to enforce_access; it merely gets the
ORIGINAL decl [i.e. the one that the using statement inherited]). To
make matters worse, a USING_DECL does not seem to store any
information about where it "came from", e.g. there's no ORIGINAL_DECL
(USING_DECL) macro or anything like that. DECL_INITIAL doesn't seem to
help (and is probably totally not related to the issue).

So we need to do a long-winded lookup.

First, we iterate through the USING_DECLs in the class where access
failed (in the context of a parent that has caused the access failure
because it has private access). For normal field variables, we COULD
simply do a name lookup on the USING_DECL and compare it to the
original DECL. That would be easy, since there can only be one
variable field with the same name.

However, that doesn't work for overloaded functions. Name lookup would
return multiple results, making comparison meaningless.

What we need is therefore not a name lookup, but a decl lookup. It
sounds stupid, because what that basically means is looking for an
exact match of a decl, which is intuitively stupid, because why would
you look for an exact match of something you already have? But
actually we can take advantage of a quirk that USING_DECLs have, which
is that, when stripped, cp_tree_equal (stripped_using_decl,
original_decl) returns true, even through stripped_using_decl and
original_decl exist on different lines and in different places. In
other words, a USING_DECL is the same as the original DECL it
inherits, even though they are in different places (Unless I am just
being really dumb?).

Anyways, to summarise ...

1) We iterate through USING_DECLs in the class that caused a private
access failure.
2) For each USING_DECL, we find the DECL that USING_DECL inherited.
  2.1) To do this, we iterate through all fields of all base classes
(possibly slow? but it is just diagnostics code afterall,
 so idk if that's a big deal)
  2.2)  We compare each FIELD against the USING_DECL. if equal, then
we return FIELD.
3) if the DECL that USING_DECL inherited is equal to the diagnostic
decl we were given in enforce_access, we return USING_DECL as being
the source of the problem.

In a perfect world, I guess the USING_DECL would store somewhere what
it inherited as it was parsed. But I'm not sure if that's practical to
do and I'm not sure it would be worth the effort considering it would
probably be used only for this niche scenario. Donno.

I have not regression tested this, but it does seem to work on the
test case at least (which I've also included). Also please ignore
formatting issues ATM.

If you think this is a reasonable way to do it then I will tidy up the
code, test it and make a patch and send it over. If anyone has any
better ideas (probably), then please let me know. I did try the name
lookup as Jason suggested but, as I say, in the case of overloaded
functions it returns multiple values, so it didn't help in determining
what DECL a USING_DECL inherited.

BTW, I will eventually put find_decl_using_decl_inherits and
lookup_decl_exact_match in search.c.

Just for proof, here's the output from the testcase (hopefully it
formats this correctly):

/home/anthony/Desktop/using9.C: In member function ‘void C::f()’:
/home/anthony/Desktop/using9.C:34:11: error: ‘int A1::comma()’ is
private within this context
   34 | comma();
  |   ^
/home/anthony/Desktop/using9.C:22:24: note: declared private here
   22 |   using A2::comma, A1::comma;  // { dg-message "declared" }
  |   ^
/home/anthony/Desktop/using9.C:35:14: error: ‘int A1::separate()’ is
private within this context
   35 | separate(); // { dg-error "private" }
  | ^
/home/anthony/Desktop/using9.C:25:13: note: declared private here
   25 |   using A1::separate; // { dg-message "declared" }
  |  ^~~~
/home/anthony/Desktop/using9.C:36:5: error: ‘int A2::alone’ is private
within this context
   36 | alone = 5; // { dg-error "private" }
  |   ^
/home/anthony/Desktop/using9.C:27:13: note: declared private here
   27 |   using A2::alone;
  |^

Actual code attached.

Anthony


On Tue, 9 Feb 2021 at 20:07, Jason Merrill  wrote:
>
> On 2/9/21 6:18 AM, Anthony Sharp wrote:
> > The parens are to help the editor line up the last line properly.  If
> > the subexpression fits on one line, the parens aren't needed.
> >
> >
> > A okay.
> >
> > No; "compile" means translate from C++ to assembly, "assemble" means
> > that, 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Jason Merrill via Gcc-patches]

On 2/9/21 6:18 AM, Anthony Sharp wrote:

The parens are to help the editor line up the last line properly.  If
the subexpression fits on one line, the parens aren't needed.


A okay.

No; "compile" means translate from C++ to assembly, "assemble" means
that, plus call the assembler to produce an object file.  Though since
compilation errors out, assembling never happens.


lol I was being dumb and thinking it was the other way around. I will 
change it.


   You could bring back your lookup from the earlier patch and look
through the result to see if the function we're complaining about is
part of what the particular using-decl brings in.


I will give that a try.

I think you want
SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
                     BINFO_TYPE (parent_binfo))


Am I reading this wrong then? :

/* Compare a BINFO_TYPE with another type for equality.
For a binfo, this is functionally equivalent to using same_type_p, but 
measurably faster.
  At least one of the arguments must be a BINFO_TYPE.  The other can be 
a BINFO_TYPE
or a regular type.  If BINFO_TYPE(T) ever stops being the main variant 
of the class the

binfo is for, this macro must change.  */
#define SAME_BINFO_TYPE_P(A, B) ((A) == (B))

That leads me to believe that arguments A and B can be: BINFO, BINFO ... 
or BINFO, TYPE ... or TYPE, BINFO. If so I don't see why this doesn't work:


(SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))
and
(SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)), 
parent_binfo))


Unless "BINFO_TYPE" is actually just how you refer to a regular class 
type and not a BINFO. Seems a bit confusing to me.


The comment is pretty conusing, I agree.  But what it's saying is that 
both arguments must be types, and one of those types must be BINFO_TYPE 
(some_binfo).


Your first line doesn't work because you're comparing a type and a 
binfo.  The second one doesn't work because you're comparing the binfo 
for a most-derived object of the type to the binfo for a base subobject 
of the same type, and those are different, because binfos represent 
nodes in inheritance graphs.



This line needs one more space.


Oh I see ... so second line's arguments need to line up with the first 
argument, not the first (.


I will send over a new patch later/tomorrow.

Anthony

On Tue, 9 Feb 2021 at 04:55, Jason Merrill > wrote:


On 2/5/21 12:28 PM, Anthony Sharp wrote:
 > Hi Marek,
 >
 >>> +      if ((TREE_CODE (parent_field) == USING_DECL)
 >>
 >> This first term doesn't need to be wrapped in ().
 >
 > I normally wouldn't use the (), but I think that's how Jason likes it
 > so that's why I did it. I guess it makes it more readable.

Ah, no, I don't see any need for the extra () there.  When I asked for
added parens previously:

 >> +       if (parent_binfo != NULL_TREE
 >> +           && context_for_name_lookup (decl)
 >> +           != BINFO_TYPE (parent_binfo))
 >
 > Here you want parens around the second != expression and its != token
 > aligned with "context"

The parens are to help the editor line up the last line properly.  If
the subexpression fits on one line, the parens aren't needed.

 >> We usually use dg-do compile.
 >
 > True, but wouldn't that technically be slower? I would agree if it
 > were more than just error-handling code.

No; "compile" means translate from C++ to assembly, "assemble" means
that, plus call the assembler to produce an object file.  Though since
compilation errors out, assembling never happens.

 > +      if ((TREE_CODE (parent_field) == USING_DECL)
 > +        && TREE_PRIVATE (parent_field)
 > +        && (DECL_NAME (decl) == DECL_NAME (parent_field)))
 > +       return parent_field;

Following our discussion about an earlier revision, this will often
produce the right using-declaration, but might not if two functions of
the same name are imported from different bases.  If I split the
using-declaration in using9.C into two, i.e.

 >   using A2::i; // { dg-message "declared" }
 >   using A::i;

then I get

 > wa.ii: In member function ‘void C::f()’:
 > wa.ii:28:7: error: ‘int A::i()’ is private within this context
 >    28 |     i();
 >       |       ^
 > wa.ii:20:13: note: declared private here
 >    20 |   using A2::i;

pointing out the wrong using-declaration because it happens to be
first.
   You could bring back your lookup from the earlier patch and look
through the result to see if the function we're complaining about is
part of what the particular using-decl brings in.

 > I tried both:
 > (SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))
 > and
 > (SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)),
parent_binfo))

I think you 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

>
> The parens are to help the editor line up the last line properly.  If
> the subexpression fits on one line, the parens aren't needed.


A okay.

No; "compile" means translate from C++ to assembly, "assemble" means
> that, plus call the assembler to produce an object file.  Though since
> compilation errors out, assembling never happens.


lol I was being dumb and thinking it was the other way around. I will
change it.

  You could bring back your lookup from the earlier patch and look
> through the result to see if the function we're complaining about is
> part of what the particular using-decl brings in.


I will give that a try.

I think you want
> SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
> BINFO_TYPE (parent_binfo))


Am I reading this wrong then? :

/* Compare a BINFO_TYPE with another type for equality.
For a binfo, this is functionally equivalent to using same_type_p, but
measurably faster.
 At least one of the arguments must be a BINFO_TYPE.  The other can be a
BINFO_TYPE
or a regular type.  If BINFO_TYPE(T) ever stops being the main variant of
the class the
binfo is for, this macro must change.  */
#define SAME_BINFO_TYPE_P(A, B) ((A) == (B))

That leads me to believe that arguments A and B can be: BINFO, BINFO ... or
BINFO, TYPE ... or TYPE, BINFO. If so I don't see why this doesn't work:

(SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))
and
(SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)),
parent_binfo))

Unless "BINFO_TYPE" is actually just how you refer to a regular class type
and not a BINFO. Seems a bit confusing to me.

This line needs one more space.


Oh I see ... so second line's arguments need to line up with the first
argument, not the first (.

I will send over a new patch later/tomorrow.

Anthony

On Tue, 9 Feb 2021 at 04:55, Jason Merrill  wrote:

> On 2/5/21 12:28 PM, Anthony Sharp wrote:
> > Hi Marek,
> >
> >>> +  if ((TREE_CODE (parent_field) == USING_DECL)
> >>
> >> This first term doesn't need to be wrapped in ().
> >
> > I normally wouldn't use the (), but I think that's how Jason likes it
> > so that's why I did it. I guess it makes it more readable.
>
> Ah, no, I don't see any need for the extra () there.  When I asked for
> added parens previously:
>
> >> +   if (parent_binfo != NULL_TREE
> >> +   && context_for_name_lookup (decl)
> >> +   != BINFO_TYPE (parent_binfo))
> >
> > Here you want parens around the second != expression and its != token
> > aligned with "context"
>
> The parens are to help the editor line up the last line properly.  If
> the subexpression fits on one line, the parens aren't needed.
>
> >> We usually use dg-do compile.
> >
> > True, but wouldn't that technically be slower? I would agree if it
> > were more than just error-handling code.
>
> No; "compile" means translate from C++ to assembly, "assemble" means
> that, plus call the assembler to produce an object file.  Though since
> compilation errors out, assembling never happens.
>
> > +  if ((TREE_CODE (parent_field) == USING_DECL)
> > +&& TREE_PRIVATE (parent_field)
> > +&& (DECL_NAME (decl) == DECL_NAME (parent_field)))
> > +   return parent_field;
>
> Following our discussion about an earlier revision, this will often
> produce the right using-declaration, but might not if two functions of
> the same name are imported from different bases.  If I split the
> using-declaration in using9.C into two, i.e.
>
> >   using A2::i; // { dg-message "declared" }
>
> >   using A::i;
>
> then I get
>
> > wa.ii: In member function ‘void C::f()’:
> > wa.ii:28:7: error: ‘int A::i()’ is private within this context
> >28 | i();
> >   |   ^
> > wa.ii:20:13: note: declared private here
> >20 |   using A2::i;
>
> pointing out the wrong using-declaration because it happens to be first.
>   You could bring back your lookup from the earlier patch and look
> through the result to see if the function we're complaining about is
> part of what the particular using-decl brings in.
>
> > I tried both:
> > (SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))
> > and
> > (SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)),
> parent_binfo))
>
> I think you want
>
> SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
> BINFO_TYPE (parent_binfo))
>
> i.e. just change same_type_p to SAME_BINFO_TYPE_P.
>
> >   tree parent_binfo = get_parent_with_private_access (decl,
> > -
>  basetype_path);
> > +
> basetype_path);
>
> This line was indented properly before, and is changed to be indented
> one space too far.
>
> > + diag_location = get_class_access_diagnostic_decl
> (parent_binfo,
> > +
> diag_decl);
>
> This line needs one more space.
>
> >   complain_about_access (decl, diag_decl, diag_location, true,
> > -parent_access);
> > +   access_failure_reason);
>
> This 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Jason Merrill via Gcc-patches]

On 2/5/21 12:28 PM, Anthony Sharp wrote:

Hi Marek,


+  if ((TREE_CODE (parent_field) == USING_DECL)


This first term doesn't need to be wrapped in ().


I normally wouldn't use the (), but I think that's how Jason likes it
so that's why I did it. I guess it makes it more readable.


Ah, no, I don't see any need for the extra () there.  When I asked for 
added parens previously:



+   if (parent_binfo != NULL_TREE
+   && context_for_name_lookup (decl)
+   != BINFO_TYPE (parent_binfo))


Here you want parens around the second != expression and its != token
aligned with "context"


The parens are to help the editor line up the last line properly.  If 
the subexpression fits on one line, the parens aren't needed.



We usually use dg-do compile.


True, but wouldn't that technically be slower? I would agree if it
were more than just error-handling code.


No; "compile" means translate from C++ to assembly, "assemble" means 
that, plus call the assembler to produce an object file.  Though since 
compilation errors out, assembling never happens.



+  if ((TREE_CODE (parent_field) == USING_DECL)
+&& TREE_PRIVATE (parent_field)
+&& (DECL_NAME (decl) == DECL_NAME (parent_field)))
+   return parent_field;


Following our discussion about an earlier revision, this will often 
produce the right using-declaration, but might not if two functions of 
the same name are imported from different bases.  If I split the 
using-declaration in using9.C into two, i.e.


  using A2::i; // { dg-message "declared" }
  using A::i;


then I get


wa.ii: In member function ‘void C::f()’:
wa.ii:28:7: error: ‘int A::i()’ is private within this context
   28 | i();
  |   ^
wa.ii:20:13: note: declared private here
   20 |   using A2::i;


pointing out the wrong using-declaration because it happens to be first. 
 You could bring back your lookup from the earlier patch and look 
through the result to see if the function we're complaining about is 
part of what the particular using-decl brings in.


I tried both: 
(SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))

and
(SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)), parent_binfo))


I think you want

SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
   BINFO_TYPE (parent_binfo))

i.e. just change same_type_p to SAME_BINFO_TYPE_P.


  tree parent_binfo = get_parent_with_private_access (decl,
- basetype_path);
+  basetype_path);


This line was indented properly before, and is changed to be indented 
one space too far.



+ diag_location = get_class_access_diagnostic_decl (parent_binfo,
+  diag_decl);


This line needs one more space.


  complain_about_access (decl, diag_decl, diag_location, true,
-parent_access);
+   access_failure_reason);


This line, too.


+};
\ No newline at end of file


Let's add a newline.

Jason

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

Hi all,

I tried both:

(SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))
and
(SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)),
parent_binfo))

but both caused around 80 regressions because it was returning false when
it should have been returning true. No idea why. In the end I used

(same_type_p (context_for_name_lookup (decl), BINFO_TYPE (parent_binfo)))

which works. New patch attached.

Anthony

On Sat, 6 Feb 2021 at 13:09, Anthony Sharp  wrote:

> I think at least something like "Improve private access checking." would
>> be better.  No need to be verbose in the ChangeLog.  :)
>
>
> That sounds like a good idea, I will change it.
>
> Yup, this one.
>
>
> Awesome.
>
> Yeah, that can be a pain.  Best if your editor does it for you.  If you
>> use vim, you can use gcc/contrib/vimrc and then vim will do most of the
>> formatting for you.
>
>
> Aah I didn't know that, thanks for the tip!
>
> Anthony
>
>
>
> On Fri, 5 Feb 2021 at 17:46, Marek Polacek  wrote:
>
>> On Fri, Feb 05, 2021 at 05:28:07PM +, Anthony Sharp wrote:
>> > Hi Marek,
>> >
>> > > Pedantically, "Altered function." is not very good, it should say what
>> > > in enforce_access changed.
>> >
>> > I would normally 100% agree, but the changes are a bit too complicated
>> > to be concisely (and helpfully) described there. I think the patch
>> > description covers it well enough; not sure what I would say without
>> > having to write a big paragraph there.
>>
>> I think at least something like "Improve private access checking." would
>> be better.  No need to be verbose in the ChangeLog.  :)
>>
>> > > Let's move the test into g++.dg/cpp1z and call it using9.C.
>> >
>> > Okie dokie - it's a bit hard to know where stuff's supposed to go in
>> > that folder. I'll put a comment in the testcase mentioning pr19377
>> > just in case there's ever a regression.
>>
>> Yeah, it's customary to start a test with
>> // PR c++/19377
>>
>> > > I don't understand the "generate a diagnostic decl location".  Maybe
>> just
>> > > "generate a diagnostic?"
>> >
>> > get_class_access_diagnostic_decl returns a decl which is used as the
>> > location that the error-producing code points to as the source of the
>> > problem. It could be better - I will change it to say "Examine certain
>> > special cases to find the decl that is the source of the problem" to
>> > make it a bit clearer.
>>
>> Oh, I see now.
>>
>> > > These two comments can be merged into one.
>> >
>> > Technically yes ... but I like how it is since in a very subtle way it
>> > creates a sense of separation between the first two paragraphs and the
>> > third. The first two paras are sort of an introduction and the third
>> > begins to describe the code.
>>
>> Fair enough.
>>
>> > > I think for comparing a binfo with a type we should use
>> SAME_BINFO_TYPE_P.
>> >
>> > Okay, I think that simplifies the code a bit aswell.
>> >
>> > > This first term doesn't need to be wrapped in ().
>> >
>> > I normally wouldn't use the (), but I think that's how Jason likes it
>> > so that's why I did it. I guess it makes it more readable.
>> >
>> > > This could be part of the if above.
>> >
>> > Oops - I forgot to change that when I modified the code.
>> >
>> > > Just "had" instead of "did had"?
>> >
>> > Good spot - that's a spelling mistake on my part. Should be "did have".
>> >
>> > > Looks like this line is indented too much (even in the newer patch).
>> >
>> > You're right (if you meant access_failure_reason = ak_private), I will
>> change.
>>
>> Yup, this one.
>>
>> > If you mean get_class_access_diagnostic_decl (parent_binfo, diag_decl)
>> > then I donno, because Linux Text Editor says both are on column 64.
>> >
>> > To be honest, I'm sure there is a way to do it, but I'm not really
>> > sure how to consistently align code. Every text/code editor/browser
>> > seems to give different column numbers (and display it differently)
>> > since they seem to all treat whitespace differently.
>>
>> Yeah, that can be a pain.  Best if your editor does it for you.  If you
>> use vim, you can use gcc/contrib/vimrc and then vim will do most of the
>> formatting for you.
>>
>> > > We usually use dg-do compile.
>> >
>> > True, but wouldn't that technically be slower? I would agree if it
>> > were more than just error-handling code.
>> >
>> > > Best to replace both with
>> > > // { dg-do compile { target c++17 } }
>> >
>> > Okie dokie. I did see both being used and I wasn't sure which to go for.
>> >
>> > I'll probably send another patch over tomorrow.
>>
>> Sounds good, thanks!
>>
>> > On Fri, 5 Feb 2021 at 16:06, Marek Polacek  wrote:
>> > >
>> > > Hi,
>> > >
>> > > a few comments:
>> > >
>> > > On Fri, Feb 05, 2021 at 03:39:25PM +, Anthony Sharp via
>> Gcc-patches wrote:
>> > > > 2021-02-05  Anthony Sharp  
>> > > >
>> > > >   * semantics.c (get_class_access_diagnostic_decl): New
>> function.
>> > > >   (enforce_access): Altered function.
>> > >
>> > > Pedantically, "Altered 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

>
> I think at least something like "Improve private access checking." would
> be better.  No need to be verbose in the ChangeLog.  :)


That sounds like a good idea, I will change it.

Yup, this one.


Awesome.

Yeah, that can be a pain.  Best if your editor does it for you.  If you
> use vim, you can use gcc/contrib/vimrc and then vim will do most of the
> formatting for you.


Aah I didn't know that, thanks for the tip!

Anthony



On Fri, 5 Feb 2021 at 17:46, Marek Polacek  wrote:

> On Fri, Feb 05, 2021 at 05:28:07PM +, Anthony Sharp wrote:
> > Hi Marek,
> >
> > > Pedantically, "Altered function." is not very good, it should say what
> > > in enforce_access changed.
> >
> > I would normally 100% agree, but the changes are a bit too complicated
> > to be concisely (and helpfully) described there. I think the patch
> > description covers it well enough; not sure what I would say without
> > having to write a big paragraph there.
>
> I think at least something like "Improve private access checking." would
> be better.  No need to be verbose in the ChangeLog.  :)
>
> > > Let's move the test into g++.dg/cpp1z and call it using9.C.
> >
> > Okie dokie - it's a bit hard to know where stuff's supposed to go in
> > that folder. I'll put a comment in the testcase mentioning pr19377
> > just in case there's ever a regression.
>
> Yeah, it's customary to start a test with
> // PR c++/19377
>
> > > I don't understand the "generate a diagnostic decl location".  Maybe
> just
> > > "generate a diagnostic?"
> >
> > get_class_access_diagnostic_decl returns a decl which is used as the
> > location that the error-producing code points to as the source of the
> > problem. It could be better - I will change it to say "Examine certain
> > special cases to find the decl that is the source of the problem" to
> > make it a bit clearer.
>
> Oh, I see now.
>
> > > These two comments can be merged into one.
> >
> > Technically yes ... but I like how it is since in a very subtle way it
> > creates a sense of separation between the first two paragraphs and the
> > third. The first two paras are sort of an introduction and the third
> > begins to describe the code.
>
> Fair enough.
>
> > > I think for comparing a binfo with a type we should use
> SAME_BINFO_TYPE_P.
> >
> > Okay, I think that simplifies the code a bit aswell.
> >
> > > This first term doesn't need to be wrapped in ().
> >
> > I normally wouldn't use the (), but I think that's how Jason likes it
> > so that's why I did it. I guess it makes it more readable.
> >
> > > This could be part of the if above.
> >
> > Oops - I forgot to change that when I modified the code.
> >
> > > Just "had" instead of "did had"?
> >
> > Good spot - that's a spelling mistake on my part. Should be "did have".
> >
> > > Looks like this line is indented too much (even in the newer patch).
> >
> > You're right (if you meant access_failure_reason = ak_private), I will
> change.
>
> Yup, this one.
>
> > If you mean get_class_access_diagnostic_decl (parent_binfo, diag_decl)
> > then I donno, because Linux Text Editor says both are on column 64.
> >
> > To be honest, I'm sure there is a way to do it, but I'm not really
> > sure how to consistently align code. Every text/code editor/browser
> > seems to give different column numbers (and display it differently)
> > since they seem to all treat whitespace differently.
>
> Yeah, that can be a pain.  Best if your editor does it for you.  If you
> use vim, you can use gcc/contrib/vimrc and then vim will do most of the
> formatting for you.
>
> > > We usually use dg-do compile.
> >
> > True, but wouldn't that technically be slower? I would agree if it
> > were more than just error-handling code.
> >
> > > Best to replace both with
> > > // { dg-do compile { target c++17 } }
> >
> > Okie dokie. I did see both being used and I wasn't sure which to go for.
> >
> > I'll probably send another patch over tomorrow.
>
> Sounds good, thanks!
>
> > On Fri, 5 Feb 2021 at 16:06, Marek Polacek  wrote:
> > >
> > > Hi,
> > >
> > > a few comments:
> > >
> > > On Fri, Feb 05, 2021 at 03:39:25PM +, Anthony Sharp via
> Gcc-patches wrote:
> > > > 2021-02-05  Anthony Sharp  
> > > >
> > > >   * semantics.c (get_class_access_diagnostic_decl): New function.
> > > >   (enforce_access): Altered function.
> > >
> > > Pedantically, "Altered function." is not very good, it should say what
> > > in enforce_access changed.
> > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > 2021-02-05  Anthony Sharp  
> > > >
> > > >   * g++.dg/pr19377.C: New test.
> > >
> > > Let's move the test into g++.dg/cpp1z and call it using9.C.
> > >
> > > >  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 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Marek Polacek via Gcc-patches]

On Fri, Feb 05, 2021 at 05:28:07PM +, Anthony Sharp wrote:
> Hi Marek,
> 
> > Pedantically, "Altered function." is not very good, it should say what
> > in enforce_access changed.
> 
> I would normally 100% agree, but the changes are a bit too complicated
> to be concisely (and helpfully) described there. I think the patch
> description covers it well enough; not sure what I would say without
> having to write a big paragraph there.

I think at least something like "Improve private access checking." would
be better.  No need to be verbose in the ChangeLog.  :)
 
> > Let's move the test into g++.dg/cpp1z and call it using9.C.
> 
> Okie dokie - it's a bit hard to know where stuff's supposed to go in
> that folder. I'll put a comment in the testcase mentioning pr19377
> just in case there's ever a regression.

Yeah, it's customary to start a test with
// PR c++/19377

> > I don't understand the "generate a diagnostic decl location".  Maybe just
> > "generate a diagnostic?"
> 
> get_class_access_diagnostic_decl returns a decl which is used as the
> location that the error-producing code points to as the source of the
> problem. It could be better - I will change it to say "Examine certain
> special cases to find the decl that is the source of the problem" to
> make it a bit clearer.

Oh, I see now.
 
> > These two comments can be merged into one.
> 
> Technically yes ... but I like how it is since in a very subtle way it
> creates a sense of separation between the first two paragraphs and the
> third. The first two paras are sort of an introduction and the third
> begins to describe the code.

Fair enough.
 
> > I think for comparing a binfo with a type we should use SAME_BINFO_TYPE_P.
> 
> Okay, I think that simplifies the code a bit aswell.
> 
> > This first term doesn't need to be wrapped in ().
> 
> I normally wouldn't use the (), but I think that's how Jason likes it
> so that's why I did it. I guess it makes it more readable.
> 
> > This could be part of the if above.
> 
> Oops - I forgot to change that when I modified the code.
> 
> > Just "had" instead of "did had"?
> 
> Good spot - that's a spelling mistake on my part. Should be "did have".
> 
> > Looks like this line is indented too much (even in the newer patch).
> 
> You're right (if you meant access_failure_reason = ak_private), I will change.

Yup, this one.

> If you mean get_class_access_diagnostic_decl (parent_binfo, diag_decl)
> then I donno, because Linux Text Editor says both are on column 64.
> 
> To be honest, I'm sure there is a way to do it, but I'm not really
> sure how to consistently align code. Every text/code editor/browser
> seems to give different column numbers (and display it differently)
> since they seem to all treat whitespace differently.

Yeah, that can be a pain.  Best if your editor does it for you.  If you
use vim, you can use gcc/contrib/vimrc and then vim will do most of the
formatting for you.
 
> > We usually use dg-do compile.
> 
> True, but wouldn't that technically be slower? I would agree if it
> were more than just error-handling code.
> 
> > Best to replace both with
> > // { dg-do compile { target c++17 } }
> 
> Okie dokie. I did see both being used and I wasn't sure which to go for.
> 
> I'll probably send another patch over tomorrow.

Sounds good, thanks!

> On Fri, 5 Feb 2021 at 16:06, Marek Polacek  wrote:
> >
> > Hi,
> >
> > a few comments:
> >
> > On Fri, Feb 05, 2021 at 03:39:25PM +, Anthony Sharp via Gcc-patches 
> > wrote:
> > > 2021-02-05  Anthony Sharp  
> > >
> > >   * semantics.c (get_class_access_diagnostic_decl): New function.
> > >   (enforce_access): Altered function.
> >
> > Pedantically, "Altered function." is not very good, it should say what
> > in enforce_access changed.
> >
> > > gcc/testsuite/ChangeLog:
> > >
> > > 2021-02-05  Anthony Sharp  
> > >
> > >   * g++.dg/pr19377.C: New test.
> >
> > Let's move the test into g++.dg/cpp1z and call it using9.C.
> >
> > >  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
> >
> > I don't understand the "generate a diagnostic decl location".  Maybe just
> > "generate a diagnostic?"
> >
> > > +   return DECL.  */
> > > +
> > > 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

Hi Marek,

> Pedantically, "Altered function." is not very good, it should say what
> in enforce_access changed.

I would normally 100% agree, but the changes are a bit too complicated
to be concisely (and helpfully) described there. I think the patch
description covers it well enough; not sure what I would say without
having to write a big paragraph there.

> Let's move the test into g++.dg/cpp1z and call it using9.C.

Okie dokie - it's a bit hard to know where stuff's supposed to go in
that folder. I'll put a comment in the testcase mentioning pr19377
just in case there's ever a regression.

> I don't understand the "generate a diagnostic decl location".  Maybe just
> "generate a diagnostic?"

get_class_access_diagnostic_decl returns a decl which is used as the
location that the error-producing code points to as the source of the
problem. It could be better - I will change it to say "Examine certain
special cases to find the decl that is the source of the problem" to
make it a bit clearer.

> These two comments can be merged into one.

Technically yes ... but I like how it is since in a very subtle way it
creates a sense of separation between the first two paragraphs and the
third. The first two paras are sort of an introduction and the third
begins to describe the code.

> I think for comparing a binfo with a type we should use SAME_BINFO_TYPE_P.

Okay, I think that simplifies the code a bit aswell.

> This first term doesn't need to be wrapped in ().

I normally wouldn't use the (), but I think that's how Jason likes it
so that's why I did it. I guess it makes it more readable.

> This could be part of the if above.

Oops - I forgot to change that when I modified the code.

> Just "had" instead of "did had"?

Good spot - that's a spelling mistake on my part. Should be "did have".

> Looks like this line is indented too much (even in the newer patch).

You're right (if you meant access_failure_reason = ak_private), I will change.

If you mean get_class_access_diagnostic_decl (parent_binfo, diag_decl)
then I donno, because Linux Text Editor says both are on column 64.

To be honest, I'm sure there is a way to do it, but I'm not really
sure how to consistently align code. Every text/code editor/browser
seems to give different column numbers (and display it differently)
since they seem to all treat whitespace differently.

> We usually use dg-do compile.

True, but wouldn't that technically be slower? I would agree if it
were more than just error-handling code.

> Best to replace both with
> // { dg-do compile { target c++17 } }

Okie dokie. I did see both being used and I wasn't sure which to go for.

I'll probably send another patch over tomorrow.

Anthony


On Fri, 5 Feb 2021 at 16:06, Marek Polacek  wrote:
>
> Hi,
>
> a few comments:
>
> On Fri, Feb 05, 2021 at 03:39:25PM +, Anthony Sharp via Gcc-patches wrote:
> > 2021-02-05  Anthony Sharp  
> >
> >   * semantics.c (get_class_access_diagnostic_decl): New function.
> >   (enforce_access): Altered function.
>
> Pedantically, "Altered function." is not very good, it should say what
> in enforce_access changed.
>
> > gcc/testsuite/ChangeLog:
> >
> > 2021-02-05  Anthony Sharp  
> >
> >   * g++.dg/pr19377.C: New test.
>
> Let's move the test into g++.dg/cpp1z and call it using9.C.
>
> >  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
>
> I don't understand the "generate a diagnostic decl location".  Maybe just
> "generate a diagnostic?"
>
> > +   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 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Marek Polacek via Gcc-patches]

Hi,

a few comments:

On Fri, Feb 05, 2021 at 03:39:25PM +, Anthony Sharp via Gcc-patches wrote:
> 2021-02-05  Anthony Sharp  
> 
>   * semantics.c (get_class_access_diagnostic_decl): New function.
>   (enforce_access): Altered function.

Pedantically, "Altered function." is not very good, it should say what
in enforce_access changed.

> gcc/testsuite/ChangeLog:
> 
> 2021-02-05  Anthony Sharp  
> 
>   * g++.dg/pr19377.C: New test.

Let's move the test into g++.dg/cpp1z and call it using9.C.

>  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

I don't understand the "generate a diagnostic decl location".  Maybe just
"generate a diagnostic?"

> +   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.  */

These two comments can be merged into one.

> +  if (context_for_name_lookup (decl) == BINFO_TYPE (parent_binfo))
> +return decl;

I think for comparing a binfo with a type we should use SAME_BINFO_TYPE_P.

> +  /* 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)

This first term doesn't need to be wrapped in ().

> +  && TREE_PRIVATE (parent_field))
> + {
> +   if (DECL_NAME (decl) == DECL_NAME (parent_field))

This could be part of the if above.  And then we can drop the braces (in the
if and for both).

> + 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 = 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

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  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 passes203708
> # of unexpected failures2
> # of expected failures989
> # of unsupported tests8714
>
> Anthony
>
>
> On Thu, 4 Feb 2021 at 20:55, Jason Merrill  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  wrote:

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

> 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 passes203708
# of unexpected failures2
# of expected failures989
# of unsupported tests8714

Anthony


On Thu, 4 Feb 2021 at 20:55, Jason Merrill  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  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 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Jason Merrill via Gcc-patches]

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  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 

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Anthony Sharp via Gcc-patches]

> 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?

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).

> 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)?

> 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.

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  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
>

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Jason Merrill via Gcc-patches]

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

Re: [PATCH] c++: Private parent access check for using decls [PR19377]

[Jason Merrill via Gcc-patches]

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?


  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