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 <anthonyshar...@gmail.com> 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 passes 203708 > # of unexpected failures 2 > # of expected failures 989 > # of unsupported tests 8714 > > Anthony > > > On Thu, 4 Feb 2021 at 20:55, Jason Merrill <ja...@redhat.com> 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 <ja...@redhat.com> 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 > > >> > > > > >
From 8136df415670eb24edc73595fb6d4485d600a5d2 Mon Sep 17 00:00:00 2001 From: Anthony Sharp <anthonyshar...@gmail.com> Date: Fri, 5 Feb 2021 15:30:45 +0000 Subject: [PATCH] c++: Private parent access check for using decl [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. Checks for the use of using decls in a parent that had private access to decl (but the child had no access) in order to ascertain the best diagnostic decl location. gcc/cp/ChangeLog: 2021-02-05 Anthony Sharp <anthonyshar...@gmail.com> * semantics.c (get_class_access_diagnostic_decl): New function. (enforce_access): Altered function. gcc/testsuite/ChangeLog: 2021-02-05 Anthony Sharp <anthonyshar...@gmail.com> * g++.dg/pr19377.C: New test. --- 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 + 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. */ + if (context_for_name_lookup (decl) == BINFO_TYPE (parent_binfo)) + return decl; + + /* 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) + && TREE_PRIVATE (parent_field)) + { + if (DECL_NAME (decl) == DECL_NAME (parent_field)) + 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 = get_parent_with_private_access (decl, - basetype_path); + 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 had 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/pr19377.C b/gcc/testsuite/g++.dg/pr19377.C new file mode 100644 index 00000000000..fb023a33f82 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr19377.C @@ -0,0 +1,28 @@ +/* { dg-do assemble } */ +// { dg-options "-std=c++17" } + +class A +{ + protected: + int i(); +}; + +class A2 +{ + protected: + int i(int a); +}; + +class B:private A, private A2 +{ + // Comma separated list only officially supported in c++17 and later. + using A::i, A2::i; // { dg-message "declared" } +}; + +class C:public B +{ + void f() + { + i(); // { dg-error "private" } + } +}; \ No newline at end of file -- 2.25.1