| Message ID | CA+Lh_AkNPsou8AwPrFSHNd-=w5bVpo=ssiv_HeOkCd=oZjV8_Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Private parent access check for using decls [PR19377] | expand |
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
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
> 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 <jason@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 >
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 <jason@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 >> >
> 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 <jason@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 <jason@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 > >> > > >
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 <anthonysharp15@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 <jason@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 <jason@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 > > >> > > > > >
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 <anthonysharp15@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 <anthonysharp15@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
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 <polacek@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 <anthonysharp15@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 <anthonysharp15@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 >
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 <polacek@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 <anthonysharp15@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 <anthonysharp15@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
> > 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 <polacek@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 <polacek@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 <anthonysharp15@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 <anthonysharp15@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 > >
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 <anthonysharp15@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 <polacek@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 <polacek@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 <anthonysharp15@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 <anthonysharp15@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 >> >>
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
> > 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. Ahhhh 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 <jason@redhat.com> 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 line, too. > > > +}; > > \ No newline at end of file > > Let's add a newline. > > Jason > >
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. > > > Ahhhh 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 <jason@redhat.com > <mailto:jason@redhat.com>> 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 line, too. > > > +}; > > \ No newline at end of file > > Let's add a newline. > > Jason >
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 <jason@redhat.com> 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.
> >
> >
> > Ahhhh 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 <jason@redhat.com
> > <mailto:jason@redhat.com>> 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 line, too.
> >
> > > +};
> > > \ No newline at end of file
> >
> > Let's add a newline.
> >
> > Jason
> >
>
/* { dg-do compile } */
// Created for c++ PR19377
class A1
{
protected:
int separate();
int comma();
};
class A2
{
protected:
int separate(int a);
int comma(int a);
int alone;
};
class B:private A1, private A2
{
// Using decls in a comma-separated list.
using A2::comma, A1::comma; // { dg-message "declared" }
// Separate using statements.
using A2::separate;
using A1::separate; // { dg-message "declared" }
// No ambiguity, just for the sake of it.
using A2::alone;
};
class C:public B
{
void f()
{
comma(); // { dg-error "private" }
separate(); // { dg-error "private" }
alone = 5; // { dg-error "private" }
}
};
/* This function may seem dumb, but actually it is possible for two decls that
are equal to really be slightly different. For example, they might be
declared in different places, such as is the case when a using statement
inherits a decl.
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". */
tree
lookup_decl_exact_match(tree binfo, void *decl)
{
for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (binfo));
parent_field;
parent_field = DECL_CHAIN (parent_field))
{
if(cp_tree_equal ((tree)decl, parent_field))
return parent_field;
}
return NULL_TREE;
}
/* Return the decl that DECL (which is a non-stripped using decl) inherits.
DECL is a member of DECLS_CLASS, which is a BINFO. On failure, return
NULL_TREE. */
tree
find_decl_using_decl_inherits (tree decl, tree decls_class)
{
/* Strip DECL to extract the actual decl. Contrary to the name, it only
returns 1 decl. */
tree decl_stripped = strip_using_decl (decl);
/* The only way to find the decl is to walk all the members of all the
bases. */
tree result = dfs_walk_once (decls_class, lookup_decl_exact_match, NULL,
decl_stripped);
/* If nothing found. */
if (!result)
return NULL_TREE;
return result;
}
/* 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 that parent. */
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 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 must
have been 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))
{
/* Find what this using decl inherited. */
tree original_decl =
find_decl_using_decl_inherits (parent_field, parent_binfo);
/* If parent_field inherits DECL, it is the source of the problem,
so return it. */
if (cp_tree_equal(original_decl, decl))
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.
The most derived class in BASETYPE_PATH is the one used to qualify DECL.
DIAG_DECL is the declaration to use in the error diagnostic. */
static bool
enforce_access (tree basetype_path, tree decl, tree diag_decl,
tsubst_flags_t complain, access_failure_info *afi = NULL)
{
gcc_assert (TREE_CODE (basetype_path) == TREE_BINFO);
if (flag_new_inheriting_ctors
&& DECL_INHERITED_CTOR (decl))
{
/* 7.3.3/18: The additional constructors are accessible if they would be
accessible when used to construct an object of the corresponding base
class. */
decl = strip_inheriting_ctors (decl);
basetype_path = lookup_base (basetype_path, DECL_CONTEXT (decl),
ba_any, NULL, complain);
}
tree cs = current_scope ();
if (processing_template_decl
&& (CLASS_TYPE_P (cs) || TREE_CODE (cs) == FUNCTION_DECL))
if (tree template_info = get_template_info (cs))
{
/* When parsing a function or class template, we in general need to
defer access checks until template instantiation time, since a friend
declaration may grant access only to a particular specialization of
the template. */
if (accessible_p (basetype_path, decl, /*consider_local_p=*/true))
/* But if the member is deemed accessible at parse time, then we can
assume it'll be accessible at instantiation time. */
return true;
/* Access of a dependent decl should be rechecked after tsubst'ing
into the user of the decl, rather than explicitly deferring the
check here. */
gcc_assert (!uses_template_parms (decl));
if (TREE_CODE (decl) == FIELD_DECL)
gcc_assert (!uses_template_parms (DECL_CONTEXT (decl)));
/* Defer this access check until instantiation time. */
deferred_access_check access_check;
access_check.binfo = basetype_path;
access_check.decl = decl;
access_check.diag_decl = diag_decl;
access_check.loc = input_location;
vec_safe_push (TI_DEFERRED_ACCESS_CHECKS (template_info), access_check);
return true;
}
if (!accessible_p (basetype_path, decl, /*consider_local_p=*/true))
{
if (flag_new_inheriting_ctors)
diag_decl = strip_inheriting_ctors (diag_decl);
if (complain & tf_error)
{
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;
/* 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);
/* 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 = 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,
access_failure_reason);
}
if (afi)
afi->record_access_failure (basetype_path, decl, diag_decl);
return false;
}
return true;
}
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 <anthonysharp15@gmail.com> 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" } > | ^~~~~ > /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 <jason@redhat.com> 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. > > > > > > > > > Ahhhh 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 <jason@redhat.com > > > <mailto:jason@redhat.com>> 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 line, too. > > > > > > > +}; > > > > \ No newline at end of file > > > > > > Let's add a newline. > > > > > > Jason > > > > >
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
> "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
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
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 <jason@redhat.com> wrote: > On 2/24/21 4:17 PM, Anthony Sharp wrote: > >> "special" > > > > > > It wouldn't be my code if it didn't have sp3ling mstakes innit! > > Actually to be fair I already changed that spelling mistake a few days > > ago in my local code ;) > > > > I was actually thinking about this last night as I was falling asleep > > (as you do) and I realised that the whole of my using decl lookup is > > redundant. I can simply do this (formatting probably messes up here): > > > > /* 1. If the "using" keyword is used to inherit DECL within the parent, > > this may cause DECL to be private, so we should return the using > > statement as the source of the problem. > > > > Scan the fields of PARENT_BINFO and see if there are any using > decls. If > > there are, see if they inherit DECL. If they do, that's where > DECL must > > have been declared private. */ > > > > for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (parent_binfo)); > > parent_field; > > parent_field = DECL_CHAIN (parent_field)) > > { > > /* Not necessary, but also check TREE_PRIVATE for the sake of > > eliminating obviously non-relevant using decls. */ > > if (TREE_CODE (parent_field) == USING_DECL > > && TREE_PRIVATE (parent_field)) > > { > > /* If the using statement inherits DECL, it is the source of the > > access failure, so return it. */ > > if (cp_tree_equal (strip_using_decl (parent_field), decl)) > > return parent_field; > > } > > } > > > > I was wrong to say that the using decl does not store "where it came > > from/what it inherits" - that's exactly what strip_using_decl > > achieves. I think the problem was that when I did my initial testing > > in trying out ways to get the original decl, I didn't strip it, so the > > comparison failed, which led me to make the whole redundant lookup, > > blah blah blah. > > > > I've run a quick test and it seems to work, even with the overloads. > > > > Will test it some more and if all's good I will probably send a new > > patch some time this weekend. > > Sounds good, though strip_using_decl (parent_field) may be overloaded if > the using-decl brings in multiple functions with that name. > > Jason > >
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
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
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
From e064f8d010baee288c47cce1981be80515101f0d Mon Sep 17 00:00:00 2001 From: Anthony Sharp <anthonysharp15@gmail.com> Date: Thu, 4 Feb 2021 12:01:59 +0000 Subject: [PATCH] c++: Check for using decl in private parent access [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: * semantics.c (get_class_access_diagnostic_decl): New function. (enforce_access): Altered function. gcc/testsuite/ChangeLog: * g++.dg/pr19377.C: New test. --- gcc/cp/semantics.c | 93 +++++++++++++++++++++++++++------- gcc/testsuite/g++.dg/pr19377.C | 21 ++++++++ 2 files changed, 95 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..6d4ef683d65 100644 --- a/gcc/cp/semantics.c +++ b/gcc/cp/semantics.c @@ -256,6 +256,62 @@ 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) + { + if (cp_tree_equal (decl, + lookup_member (parent_binfo, + DECL_NAME (parent_field), + /*protect=*/0, + /*want_type=*/false, + tf_warning_or_error))) + 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 +373,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..356329801ac --- /dev/null +++ b/gcc/testsuite/g++.dg/pr19377.C @@ -0,0 +1,21 @@ +/* { dg-do assemble } */ + +class A +{ + protected: + int i; +}; + +class B:public A +{ + private: + using A::i; // { dg-message "declared" } +}; + +class C:public B +{ + void f() + { + A::i = 0; // { dg-error "private" } + } +}; \ No newline at end of file -- 2.25.1