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 <anthonyshar...@gmail.com> 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 <pola...@redhat.com> wrote: > >> On Fri, Feb 05, 2021 at 05:28:07PM +0000, 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 <pola...@redhat.com> wrote: >> > > >> > > Hi, >> > > >> > > a few comments: >> > > >> > > On Fri, Feb 05, 2021 at 03:39:25PM +0000, Anthony Sharp via >> Gcc-patches wrote: >> > > > 2021-02-05 Anthony Sharp <anthonyshar...@gmail.com> >> > > > >> > > > * 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 <anthonyshar...@gmail.com> >> > > > >> > > > * 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 = 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 >> > > >> > > Just "had" instead of "did had"? >> > > >> > > > + 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; >> > > >> > > Looks like this line is indented too much (even in the newer patch). >> > > >> > > > >> > > > /* 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 } */ >> > > >> > > We usually use dg-do compile. >> > > >> > > > +// { dg-options "-std=c++17" } >> > > >> > > Best to replace both with >> > > >> > > // { dg-do compile { target 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 >> > > > >> > > >> > > Marek >> > > >> > >> >> Marek >> >>
From bdf7f93ddbfac47e027259f63f7c9f8efaf4e13e Mon Sep 17 00:00:00 2001 From: Anthony Sharp <anthonyshar...@gmail.com> Date: Sat, 6 Feb 2021 18:53:19 +0000 Subject: [PATCH] c++: Private parent access check for using decls [PR19377] This bug was already mostly fixed by the patch for PR17314. This patch continues that by ensuring that where a using decl is used, causing an access failure to a child class because the using decl is private, the compiler correctly points to the using decl as the source of the problem. 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-06 Anthony Sharp <anthonyshar...@gmail.com> * semantics.c (get_class_access_diagnostic_decl): New function for enforce_access. (enforce_access): Now checks using decls when encountering private parent in context of access failure. gcc/testsuite/ChangeLog: 2021-02-06 Anthony Sharp <anthonyshar...@gmail.com> * g++.dg/cpp1z/using9.C: New test. --- gcc/cp/semantics.c | 87 ++++++++++++++++++++++------- gcc/testsuite/g++.dg/cpp1z/using9.C | 29 ++++++++++ 2 files changed, 97 insertions(+), 19 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..6a1ace85277 100644 --- a/gcc/cp/semantics.c +++ b/gcc/cp/semantics.c @@ -256,6 +256,56 @@ 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 sepcial cases + to find the decl that is the source of the problem. 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 (same_type_p (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) + && (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 +367,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 have private access, then we need to do + special checks to obtain the best diagnostic location decl. */ + if (parent_binfo != NULL_TREE) { - diag_location = TYPE_NAME (BINFO_TYPE (parent_binfo)); - parent_access = ak_private; + diag_location = get_class_access_diagnostic_decl (parent_binfo, + diag_decl); + + /* We also at this point know that the reason access failed was + because decl was private. */ + access_failure_reason = ak_private; } /* Finally, generate an error message. */ complain_about_access (decl, diag_decl, diag_location, true, - parent_access); + access_failure_reason); } if (afi) afi->record_access_failure (basetype_path, decl, diag_decl); diff --git a/gcc/testsuite/g++.dg/cpp1z/using9.C b/gcc/testsuite/g++.dg/cpp1z/using9.C new file mode 100644 index 00000000000..76965dbd0c6 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1z/using9.C @@ -0,0 +1,29 @@ +/* { dg-do assemble { target c++17 } } */ +// Assemble instead of compile since this code produces errors. +// Created for c++ PR19377 + +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