Re: [PATCH] c++: error message for dependent template members [PR70417]

[Anthony Sharp via Gcc-patches]

Hi Jason,

Hope you are well. Apologies, I've not had time to sit down and look at
this since last month I quit my old job, then I had family around for the
whole of the Christmas period, and then even more recently I've had to
start my new job.

In any case happy that you managed to figure it all out. I noticed the
small regression at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104025. To
be honest I wouldn't even know how to begin to go about fixing that so
perhaps it's best if I leave that to you? I guess it's not playing well
with the use of warn_missing_template_keyword. Maybe I didn't set that
variable up properly or something.

Kind regards,
Anthony

On Thu, 13 Jan 2022 at 21:01, Jason Merrill  wrote:

> On 12/9/21 10:51, Jason Merrill wrote:
> > On 12/4/21 12:23, Anthony Sharp wrote:
> >> Hi Jason,
> >>
> >> Hope you are well. Apologies for not coming back sooner.
> >>
> >>  >I'd put it just above the definition of saved_token_sentinel in
> >> parser.c.
> >>
> >> Sounds good, done.
> >>
> >>  >Maybe cp_parser_require_end_of_template_parameter_list?  Either way
> >> is fine.
> >>
> >> Even better, have changed it.
> >>
> >>  >Hmm, good point; operators that are member functions must be
> >> non-static,
> >>  >so we couldn't be doing a comparison of the address of the function.
> >>
> >> In that case I have set it to return early there.
> >>
> >>  >So declarator_p should be true there.  I'll fix that.
> >>
> >> Thank you.
> >>
> >>  >> +  if (next_token->keyword == RID_TEMPLATE)
> >>  >> +{
> >>  >> +  /* But at least make sure it's properly formed (e.g. see
> >> PR19397).  */
> >>  >> +  if (cp_lexer_peek_nth_token (parser->lexer, 2)->type ==
> >> CPP_NAME)
> >>  >> +   return 1;
> >>  >> +
> >>  >> +  return -1;
> >>  >> +}
> >>  >> +
> >>  >> +  /* Could be a ~ referencing the destructor of a class template.
> >>  */
> >>  >> +  if (next_token->type == CPP_COMPL)
> >>  >> +{
> >>  >> +  /* It could only be a template.  */
> >>  >> +  if (cp_lexer_peek_nth_token (parser->lexer, 2)->type ==
> >> CPP_NAME)
> >>  >> +   return 1;
> >>  >> +
> >>  >> +  return -1;
> >>  >> +}
> >>  >
> >>  >Why don't these check for the < ?
> >>
> >> I think perhaps I could have named the function better; instead of
> >> next_token_begins_template_id, how's about
> >> next_token_begins_template_name?
> >> That's all I intended to check for.
> >
> > You're using it to control whether we try to parse a template-id, and
> > it's used to initialize variables named looks_like_template_id, so I
> > think better to keep the old name.
> >
> >> In the first case, something like "->template some_name" will always be
> >> intended as a template, so no need to check for the <. If there were
> >> default
> >> template arguments you could also validly omit the <> completely, I
> think
> >> (could be wrong).
> >
> > Or if the template arguments can be deduced, yes:
> >
> > template  struct A
> > {
> >template  void f(U u);
> > };
> >
> > template  void g(A a)
> > {
> >a->template f(42);
> > }
> >
> > But 'f' is still not a template-id.
> >
> > ...
> >
> > Actually, it occurs to me that you might be better off handling this in
> > cp_parser_template_name, something like the below, to avoid the complex
> > duplicate logic in the id-expression handling.
> >
> > Note that in this patch I'm using "any_object_scope" as a proxy for "I'm
> > parsing an expression", since !is_declaration doesn't work for that; as
> > a result, this doesn't handle the static member function template case.
> > For that you'd probably still need to pass down a flag so that
> > cp_parser_template_name knows it's being called from
> > cp_parser_id_expression.
> >
> > Your patch has a false positive on
> >
> > template  struct A { };
> > template  void f()
> > {
> >A();
> > };
> >
> > which my patch checks in_template_argument_list_p to avoid, though
> > checking any_object_scope also currently avoids it.
> >
> > What do you think?
>
> I decided that it made more sense to keep the check in
> cp_parser_id_expression like you had it, but I moved it to the end to
> simplify the logic.  Here's what I'm applying, thanks!

Re: [PATCH] c++: error message for dependent template members [PR70417]

[Anthony Sharp via Gcc-patches]

Hi Jason,

Hope you are well. Apologies for not coming back sooner.

>I'd put it just above the definition of saved_token_sentinel in parser.c.

Sounds good, done.

>Maybe cp_parser_require_end_of_template_parameter_list?  Either way is
fine.

Even better, have changed it.

>Hmm, good point; operators that are member functions must be non-static,
>so we couldn't be doing a comparison of the address of the function.

In that case I have set it to return early there.

>So declarator_p should be true there.  I'll fix that.

Thank you.

>> +  if (next_token->keyword == RID_TEMPLATE)
>> +{
>> +  /* But at least make sure it's properly formed (e.g. see
PR19397).  */
>> +  if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_NAME)
>> +   return 1;
>> +
>> +  return -1;
>> +}
>> +
>> +  /* Could be a ~ referencing the destructor of a class template.  */
>> +  if (next_token->type == CPP_COMPL)
>> +{
>> +  /* It could only be a template.  */
>> +  if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_NAME)
>> +   return 1;
>> +
>> +  return -1;
>> +}
>
>Why don't these check for the < ?

I think perhaps I could have named the function better; instead of
next_token_begins_template_id, how's about next_token_begins_template_name?
That's all I intended to check for.

In the first case, something like "->template some_name" will always be
intended as a template, so no need to check for the <. If there were default
template arguments you could also validly omit the <> completely, I think
(could be wrong).

In the second case I think you are correct. I have added a check for the
"<".

>> +  /* E.g. "::operator- <>". */
>> +  if (next_token->keyword == RID_OPERATOR)
>> +{
>> +  /* Consume it so it doesn't get in our way.  */
>> +  cp_lexer_consume_token (parser->lexer);
>> +  next_token = cp_lexer_peek_token (parser->lexer);
>> +  found_operator_keyword = true;
>> +}
>> +
>> +  if (next_token->type == CPP_TEMPLATE_ID)
>> +   return 1;
>> +
>> +  /* By now the next token should be a name/operator and the one after
that
>> + should be a "<".  */
>> +  if (!cp_parser_nth_token_starts_template_argument_list_p (parser, 2))
>> +return -1;
>> +
>> +  if (!found_operator_keyword && next_token->type != CPP_NAME)
>> +return -1;
>
>These could be if/else if so you don't need the found_operator_keyword
>variable.

Hmm yes I think so. But that's been refactored now anyways; it returns if
it sees
RID_OPERATOR.

>> +  if (next_token->type == CPP_TEMPLATE_ID)
>> +return 1;
>
>This could move above the saved_token_sentinel; you won't have a
>CPP_TEMPLATE_ID after RID_OPERATOR.

Yes thanks, done.

>> +  /* If this is a function template then we would see a "(" after the
>> + final ">".  It could also be a class template constructor.  */
>> +  if (next_token->type == CPP_OPEN_PAREN
>> +  /* If this is a class template then we could see a scope token
after
>> +  the final ">".  */
>> +  || next_token->type == CPP_SCOPE
>> +  /* We could see a ";" after a variable template.  */
>> +  || next_token->type == CPP_SEMICOLON
>> +  /* We could see something like
>> +friend vect (::operator- <>)( const vect&, const vect&
);
>> +*/
>> +  || next_token->type == CPP_CLOSE_PAREN)
>> +return 1;
>
>This seems too limited.  As I was saying before,
>
>>   I guess any operator-or-punctuator after the > would suggest a
>> template-id.
>
>For instance, the patch doesn't currently warn about
>
>template  struct A
>{
>   template  static U m;
>};
>
>template  int f (T t)
>{
>   return t.m + 42;
>}
>
>This is especially a problem since we use the return value of -1 to skip
>calling cp_parser_template_id_expr.

Maybe a better approach then as you hinted at before would be to check for
what
we would not expect to see (i.e. a name), rather than what we would. I've
updated it. Seems to work?

>> +++ b/gcc/testsuite/g++.dg/cpp0x/decltype34.C
>> @@ -7,13 +7,13 @@ struct impl
>>  };
>>
>>  template> -class = decltype(impl::create()->impl::create())>
>> +class = decltype(impl::create()->impl::template create())>
>
>As I was saying in earlier discussion, this is currently a false
>positive, because impl:: is a non-dependent scope.  If parser->scope is
>set, we don't need to look at parser->context->object_type...
>
>> +  && (dependent_scope_p (parser->context->object_type)
>> + /* :: access expressions.  */
>> + || (dependent_scope_p (parser->scope)
>
>...here.

Ah yes I see now. Sorry, I misunderstood what you had said before. I
thought you
were saying that there is no need to check whether the entire expression is
dependent, merely the class we are trying to access (although my hunch is
that
if any part of an expression is dependent, the whole thing must be). But yes
that regression test confused me quite a bit, having read the issue that it
was trying to solve it makes a lot 

Re: [PATCH] c++: error message for dependent template members [PR70417]

[Anthony Sharp via Gcc-patches]

Hi Jason,

Hope you are well. Thanks for the feedback.


> I like adding the configurability, but I think let's keep committing as
> the default behavior.  And adding the parameter to the rollback function
> seems unnecessary.  For the behavior argument, let's use an enum to be
> clearer.
>

Sure, all done. The declaration of the enum could have gone in many places
it seems but I put it in cp-tree.h. Hard to tell exactly where in the file
it needed to go but I took an educated guess and put it in what I think is
the parser.c bit.

If the next token is >, we're already at the end of the template
> argument list.  If not, then something has gone wrong, so give an error
> and skip ahead.
>

I guess it just seemed odd that with a name like
cp_parser_skip_to_end_of_template_parameter_list, the calling function
would expect that it should already be at the end of a template parameter
list. Maybe if it was called
cp_parser_check_reached_end_of_template_parameter_list... but like you
suggested that's been refactored now anyways.

Let's use cp_parser_nth_token_starts_template_argument_list_p.
>

Good point. Surprised that didn't trigger a failed test, but maybe there
just isn't one.

Without the <> they may be template names, but they aren't template-ids.
>

Very true, updated the comment to hopefully make more sense.

I don't think << is ever treated as two <; that only applies to >>.
>

Good to know, I've taken that comment out.

I think we don't want to return early here, we want to go through the
> same ( check that we do for regular names.
>

I have changed it to do that, but could something like "operator- <" ever
be intended as something other than the start of a template-id?

The || case is already handled by the test at the top of the function,
> right?
>

I thought this too originally but I don't think it is. The first check is
just a performance booster, and only returns if it sees a name without a <
after it. If it doesn't see a name, that's fine, because it might instead
be a keyword like operator or template, so we continue. We then checked for
the template and keyword operators and returned appropriately if we saw one
(although, now, because of the change above, we continue if we see the
operator keyword). But at this point we haven't checked what comes after
the operator keyword. It could be gibberish. So at that point we ensure
that we see a name and a < at that point. I don't think this check can go
any earlier, since that's the earliest we can safely ensure we see both a
name and a <, given the possibility of finding keywords initially. Although
that whole part had to be refactored so it might be clearer now anyways.

> +  /* Consume the name and the "<" because
> +  cp_parser_skip_to_end_of_template_parameter_list requires it.  */
> +  cp_lexer_consume_token (parser->lexer);
> +  cp_lexer_consume_token (parser->lexer);
> +
> +  /* Skip to the end.  If it fails, it wasn't a template.  */
> +  if (!cp_parser_skip_to_end_of_template_parameter_list (parser, true))
> +return -1;

How about factoring this call and the previous line into a

cp_parser_skip_template_argument_list (parser)

?

> -  /* Are we ready, yet?  If not, issue error message.  */
> -  if (cp_parser_require (parser, CPP_GREATER, RT_GREATER))
> -return;

We could also factor this check into a separate function, leaving only
the actual walking that's shared between the two callers.

Yeah I think that would make things a lot clearer. I think I've done what
you've suggested - I've removed the awkward call to cp_parser_require and
put it (along with the call to
cp_parser_skip_to_end_of_template_parameter_list) in its own new function
called cp_parser_ensure_reached_end_of_template_parameter_list.

I've then made another function called
cp_parser_skip_entire_template_parameter_list that consumes the "<" and
then skips to the end by calling the other function.

I'm not sure how you'd see ';' after a class template-id, but you could
> see it for a variable template.  Along with many other possible tokens.
>   I guess any operator-or-punctuator after the > would suggest a
> template-id.
>

At least one of us knows what they're talking about. Updated the comment.

You're building a cp_expr that immediately converts back to a tree; just
> pass next_token->u.value to constructor_name_p.
>

Oops... fixed.

This should mention the possible false positive and how to silence it:
> If you get actually meant to write two comparisons in t.m(0), you can
> make that clear by writing (t.m(0).
>

Done.

Hmm, that's a problem.  Can you avoid it by checking declarator_p?
>

We actually already check declarator_p in cp_parser_id_expression in that
case. The reason it throws a warning is because typename14.C is
intentionally malformed; in the eyes of the compiler it's written like an
expression because it's missing the return type (although, even adding a
return type would not make it valid). I'm not sure there's any worthwhile
way around this 

Re: [PATCH] c++: error message for dependent template members [PR70417]

[Anthony Sharp via Gcc-patches]

> next_token->location),
>parser->scope))
>
> This meant a lot of things were being excluded that weren't supposed
> to be. Oops! Changing this opened up a whole new can of worms, so I
> had to make some changes to the main logic, but it just a little bit
> in the end.
>
> Regtested everything again and all seems fine. Bootstraps fine. Patch
> attached. Let me know if it needs anything else.
>
> Thanks for the help,
> Anthony
>
>
>
> On Fri, 27 Aug 2021 at 22:33, Jason Merrill  wrote:
> >
> > On 8/20/21 12:56 PM, Anthony Sharp via Gcc-patches wrote:
> > > Hi, hope everyone is well. I have a patch here for issue 70417
> > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70417). I'm still a GCC
> > > noob, and this is probably the hardest thing I have ever coded in my
> > > life, so please forgive any initial mistakes!
> > >
> > > TLDR
> > >
> > > This patch introduces a helpful error message when the user attempts
> > > to put a template-id after a member access token (., -> or ::) in a
> > > dependent context without having put the "template" keyword after the
> > > access token.
> >
> > Great, thanks!
> >
> > > CONTEXT
> > >
> > > In C++, when referencing a class member (using ., -> or ::) in a
> > > dependent context, if that member is a template, the template keyword
> > > is required after the access token. For example:
> > >
> > > template void MyDependentTemplate(T t)
> > > {
> > >t.DoSomething(); /* Error - t is dependent. "<" is treated as a
> > > less-than sign because DoSomething is assumed to be a non-template
> > > identifier. */
> > >t.template DoSomething(); // Good.
> > > }
> > >
> > > PATCH EXPLANATION
> > >
> > > In order to throw an appropriate error message we need to identify
> > > and/or create a point in the compiler where the following conditions
> > > are all simultaneously satisfied:
> > >
> > > A) a class member access token (., ->, ::)
> > > B) a dependent context
> > > C) the thing being accessed is a template-id
> > > D) No template keyword
> > >
> > > A, B and D are relatively straightforward and I won't go into detail
> > > about how that was achieved. The real challenge is C - figuring out
> > > whether a name is a template-id.
> > >
> > > Usually, we would look up the name and use that to determine whether
> > > the name is a template or not. But, we cannot do that. We are in a
> > > dependent context, so lookup cannot (in theory) find anything at the
> > > point the access expression is parsed.
> > >
> > > We (maybe) could defer checking until the template is actually
> > > instantiated. But this is not the approach I have gone for, since this
> > > to me sounded unnecessarily awkward and clunky. In my mind this also
> > > seems like a syntax error, and IMO it seems more natural that syntax
> > > errors should get caught as soon as they are parsed.
> > >
> > > So, the solution I went for was to figure out whether the name was a
> > > template by looking at the surrounding tokens. The key to this lies in
> > > being able to differentiate between the start and end of a template
> > > parameter list (< and >) and greater-than and less-than tokens (and
> > > perhaps also things like << or >>). If the final > (if we find one at
> > > all) does indeed mark the end of a class or function template, then it
> > > will be followed by something like (, ::, or even just ;. On the other
> > > hand, a greater-than operator would be followed by a name.
> > >
> > > PERFORMANCE IMPACT
> > >
> > > My concern with this approach was that checking this would actually
> > > slow the compiler down. In the end it seemed the opposite was true. By
> > > parsing the tokens to determine whether the name looks like a
> > > template-id, we can cut out a lot of the template-checking code that
> > > gets run trying to find a template when there is none, making compile
> > > time generally faster. That being said, I'm not sure if the
> > > improvement will be that substantial, so I did not feel it necessary
> > > to introduce the template checking method everywhere where we could
> > > possibly have run into a template.
> > >
> >

Re: [PATCH] c++: error message for dependent template members [PR70417]

[Anthony Sharp via Gcc-patches]

Re-adding gcc-patches@gcc.gnu.org.

-- Forwarded message -
From: Anthony Sharp 
Date: Fri, 17 Sept 2021 at 23:11
Subject: Re: [PATCH] c++: error message for dependent template members [PR70417]
To: Jason Merrill 


Hi Jason! Apologies for the delay.

> This is basically core issue 1835, http://wg21.link/cwg1835

> This was changed for C++23 by the paper "Declarations and where to find
> them", http://wg21.link/p1787

Interesting, I was not aware of that. I was very vaguely aware that a
template-id in a class member access expression could be found by
ordinary lookup (very bottom of here
https://en.cppreference.com/w/cpp/language/dependent_name), but it's
interesting to see it is deeper than I realised.

> But in either case, whether create is in a dependent scope depends on
> how we resolve impl::, we don't need to remember further back in the
> expression.  So your dependent_expression_p parameter seems like the
> wrong approach.  Note that when we're looking up the name after ->, the
> type of the object expression is in parser->context->object_type.

That's true. I think my thinking was that since it already got figured
out in cp_parser_postfix_dot_deref_expression, which is where . and ->
access expressions come from, I thought I might as well pass it
through, since it seemed to work. But looking again, you're right,
it's not really worth the hassle; might as well just call
dependent_scope_p again.

> The cases you fixed in symbol-summary.h are indeed mistakes, but not
> ill-formed, so giving an error on them is wrong.  For example, here is a
> well-formed program that is rejected with your patch:

> template  void f(T t) { t.m(0); }
> struct A { int m; } a;
> int main() { f(a); }

I suppose there was always going to be edge-cases when doing it the
way I've done. But yes, it can be worked-around by making it a warning
instead. Interestingly Clang doesn't trip up on that example, so I
guess they must be examining it some other way (e.g. at instantiation
time) - but that approach perhaps misses out on the slight performance
improvement this seems to bring.

> Now that we're writing C++, I'd prefer to avoid this kind of pattern in
> favor of RAII, such as saved_token_sentinel.  If this is still relevant
> after addressing the above comments.

Sorry, it's the junior developer in me showing! So this confused me at
first. After having mucked around a bit I tried using
saved_token_sentinel but didn't see any benefit since it doesn't
rollback on going out of scope, and I'll always want to rollback. I
can call rollback directly, but then I might as well save and restore
myself. So what I did was use it but also modify it slightly to
rollback by default on going out of scope (in my mind that makes more
sense, since if something goes wrong you wouldn't normally want to
commit anything that happened [edit 1: unless committing was part of
the whole sanity checking thing] [edit 2: well I guess you could also
argue that since this is a parser afterall, we like to KEEP things
sometimes]). But anyways, I made this configurable; it now has three
modes - roll-back, commit or do nothing. Let me know if you think
that's not the way to go.

> This code doesn't handle skipping matched ()/{}/[] in the
> template-argument-list.  You probably want to involve
> cp_parser_skip_to_end_of_template_parameter_list somehow.

Good point. It required some refactoring, but I have used it. Also,
just putting it out there, this line from
cp_parser_skip_to_end_of_template_parameter_list makes zero sense to
me (why throw an error OR immediately return?), but I have worked
around it, since it seems to break without it:

> /* Are we ready, yet?  If not, issue error message.  */
> if (cp_parser_require (parser, CPP_GREATER, RT_GREATER))
>   return false;

Last thing - I initially made a mistake. I put something like:

(next_token->type == CPP_NAME
 && MAYBE_CLASS_TYPE_P (parser->scope)
 && !constructor_name_p (cp_expr (next_token->u.value,
  next_token->location),
   parser->scope))

Instead of:

!(next_token->type == CPP_NAME
  && MAYBE_CLASS_TYPE_P (parser->scope)
  && constructor_name_p (cp_expr (next_token->u.value,
  next_token->location),
   parser->scope))

This meant a lot of things were being excluded that weren't supposed
to be. Oops! Changing this opened up a whole new can of worms, so I
had to make some changes to the main logic, but it just a little bit
in the end.

Regtested everything again and all seems fine. Bootstraps fine. Patch
attached. Let me know if it needs anything else.

Thanks for the help,
Anthony



On Fri, 27 Aug 2021 at 22:33, Jason Merrill  wrote:
>
>

[PATCH] c++: error message for dependent template members [PR70417]

[Anthony Sharp via Gcc-patches]

Hi, hope everyone is well. I have a patch here for issue 70417
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70417). I'm still a GCC
noob, and this is probably the hardest thing I have ever coded in my
life, so please forgive any initial mistakes!

TLDR

This patch introduces a helpful error message when the user attempts
to put a template-id after a member access token (., -> or ::) in a
dependent context without having put the "template" keyword after the
access token.

CONTEXT

In C++, when referencing a class member (using ., -> or ::) in a
dependent context, if that member is a template, the template keyword
is required after the access token. For example:

template void MyDependentTemplate(T t)
{
  t.DoSomething(); /* Error - t is dependent. "<" is treated as a
less-than sign because DoSomething is assumed to be a non-template
identifier. */
  t.template DoSomething(); // Good.
}

PATCH EXPLANATION

In order to throw an appropriate error message we need to identify
and/or create a point in the compiler where the following conditions
are all simultaneously satisfied:

A) a class member access token (., ->, ::)
B) a dependent context
C) the thing being accessed is a template-id
D) No template keyword

A, B and D are relatively straightforward and I won't go into detail
about how that was achieved. The real challenge is C - figuring out
whether a name is a template-id.

Usually, we would look up the name and use that to determine whether
the name is a template or not. But, we cannot do that. We are in a
dependent context, so lookup cannot (in theory) find anything at the
point the access expression is parsed.

We (maybe) could defer checking until the template is actually
instantiated. But this is not the approach I have gone for, since this
to me sounded unnecessarily awkward and clunky. In my mind this also
seems like a syntax error, and IMO it seems more natural that syntax
errors should get caught as soon as they are parsed.

So, the solution I went for was to figure out whether the name was a
template by looking at the surrounding tokens. The key to this lies in
being able to differentiate between the start and end of a template
parameter list (< and >) and greater-than and less-than tokens (and
perhaps also things like << or >>). If the final > (if we find one at
all) does indeed mark the end of a class or function template, then it
will be followed by something like (, ::, or even just ;. On the other
hand, a greater-than operator would be followed by a name.

PERFORMANCE IMPACT

My concern with this approach was that checking this would actually
slow the compiler down. In the end it seemed the opposite was true. By
parsing the tokens to determine whether the name looks like a
template-id, we can cut out a lot of the template-checking code that
gets run trying to find a template when there is none, making compile
time generally faster. That being said, I'm not sure if the
improvement will be that substantial, so I did not feel it necessary
to introduce the template checking method everywhere where we could
possibly have run into a template.

I ran an ABAB test with the following code repeated enough times to
fill up about 10,000 lines:

ai = 1;
bi = 2;
ci = 3;
c.x = 4;
()->x = 5;
mytemplateclass::y = 6;
c.template class_func();
()->template class_func();
mytemplateclass::template class_func_static();
c2.class_func();
()->class_func();
myclass::class_func_static();
ai = 6;
bi = 5;
ci = 4;
c.x = 3;
()->x = 2;
mytemplateclass::y = 1;
c.template class_func();
()->template class_func();
mytemplateclass::template class_func_static();
c2.class_func();
()->class_func();
myclass::class_func_static();

It basically just contains a mixture of class access expressions plus
some regular assignments for good measure. The compiler was compiled
without any optimisations (and so slower than usual). In hindsight,
perhaps this test case assumes the worst-ish-case scenario since it
contains a lot of templates; most of the benefits of this patch occur
when parsing non-templates.

The compiler (clean) and the compiler with the patch applied (patched)
compiled the above code 20 times each in ABAB fashion. On average, the
clean compiler took 2.26295 seconds and the patched compiler took
2.25145 seconds (about 0.508% faster). Whether this improvement
remains or disappears when the compiler is built with optimisations
turned on I haven't tested. My main concern was just making sure it
didn't get any slower.

AWKWARD TEST CASES

You will see from the patch that it required the updating of a few
testcases (as well as one place within the compiler). I believe this
is because up until now, the compiler allowed the omission of the
template keyword in some places it shouldn't have. Take decltype34.C
for example:

struct impl
{
  template  static T create();
};

template()->impl::create())>
struct tester{};

GCC currently accepts this code. My patch causes it to be rejected.
For what it's worth, Clang also rejects this code:

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

[Anthony Sharp via Gcc-patches]

Hiya

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

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

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

Changed it.

Latest patch is attached. Compiles fine and no regressions.

Anthony
From 0b2dd8d8818c6e23b3c8d1b0de7b71c1bb86e6c3 Mon Sep 17 00:00:00 2001
From: Anthony Sharp 
Date: Wed, 10 Mar 2021 20:36:03 +
Subject: [PATCH] c++: Private parent access check for using decls [PR19377]

This bug was already mostly fixed by the patch for PR17314. This
patch continues that by ensuring that where a using decl is used,
causing an access failure to a child class because the using decl is
private, the compiler correctly points to the using decl as the
source of the problem.

gcc/cp/ChangeLog:

2021-03-10  Anthony Sharp  

	* semantics.c (get_class_access_diagnostic_decl): New
	function that examines special cases when a parent
	class causes a private access failure.
	(enforce_access): Slightly modified to call function
	above.

gcc/testsuite/ChangeLog:

2021-03-10  Anthony Sharp  

	* g++.dg/cpp1z/using9.C: New using decl test.
---
 gcc/cp/semantics.c  | 110 +++-
 gcc/testsuite/g++.dg/cpp1z/using9.C |  49 +
 2 files changed, 141 insertions(+), 18 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1z/using9.C

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 73834467fca..f1a4f102408 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -256,6 +256,81 @@ pop_to_parent_deferring_access_checks (void)
 }
 }
 
+/* Called from enforce_access.  A class has attempted (but failed) to access
+   DECL.  It is already established that a baseclass of that class,
+   PARENT_BINFO, has private access to DECL.  Examine certain special cases
+   to find a decl that accurately describes the source of the problem.  If
+   none of the special cases apply, simply return DECL as the source of the
+   problem.  */
+
+static tree
+get_class_access_diagnostic_decl (tree parent_binfo, tree decl)
+{
+  /* When a class is denied access to a decl in a baseclass, most of the
+ time it is because the decl itself was declared as private at the point
+ of declaration.
+
+ However, in C++, there are (at least) two situations in which a decl
+ can be private even though it was not originally defined as such.
+ These two situations only apply if a baseclass had private access to
+ DECL (this function is only called if that is the case).  */
+
+  /* We should first check whether the reason the parent had private access
+ to DECL was simply because DECL was created and declared as private in
+ the parent.  If it was, then DECL is definitively the source of the
+ problem.  */
+  if (SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
+			  BINFO_TYPE (parent_binfo)))
+return decl;
+
+  /* 1.  If the "using" keyword is used to inherit DECL within the parent,
+ this may cause DECL to be private, so we should return the using
+ statement as the source of the problem.
+
+ Scan the fields of PARENT_BINFO and see if there are any using decls.  If
+ there are, see if they inherit DECL.  If they do, that's where DECL must
+ have been declared private.  */
+
+  for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (parent_binfo));
+   parent_field;
+   parent_field = DECL_CHAIN (parent_field))
+{
+  /* Not necessary, but also check TREE_PRIVATE for the sake of
+  eliminating obviously non-relevant using decls.  */
+  if (TREE_CODE (parent_field) == USING_DECL
+	   && TREE_PRIVATE (parent_field))
+	{
+	  tree decl_stripped = strip_using_decl (parent_field);
+
+	  if (is_overloaded_fn (decl_stripped))
+	  {
+	/* The using statement might be overloaded.  If so, we need to
+	   check all of the overloads.  */
+	for (ovl_iterator iter (decl_stripped); iter; ++iter)
+	{
+	  /* If equal, the using statement inherits DECL, and so is the
+	  source of the access failure, so return it.  */
+	  if (*iter == decl)
+		return parent_field;
+	}
+	  }
+	  else if (decl_stripped == decl)
+	return parent_field;
+	}
+}
+
+  /* 2.  If DECL was privately inherited by the parent class, then DECL will
+ be inaccessible, even though it may originally have been accessible to
+ deriving classes.  In that case, the fault lies with the parent, since it
+ used a private inheritance, so we return the parent as the source of the
+ problem.
+
+ Since this is the last check, we just assume it's true.  At worst, it
+ will simply point to the class that failed to give access, which is
+ technically true.  */
+  return TYPE_NAME (BINFO_TYPE (parent_binfo));
+}
+
 /* If the current scope isn't allowed to access DECL along
BASETYPE_PATH, give an error, or if we're parsing a function or class
template, defer the access check to be performed 

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

[Anthony Sharp via Gcc-patches]

Hi all,

Here is the patch as promised. Regression tested on the c++ side and
everything seems okay. Compiles fine.

Sounds good, though strip_using_decl (parent_field) may be overloaded if
> the using-decl brings in multiple functions with that name.


Could you give my new regression test a quick glance over then just to make
sure I've not forgotten about something? It definitely seems to work but
I'm no expert in all the different ways using statements can be
constructed. If you were to use 'using comma' (in the testcase), it
generates another error because it's ambiguous, so that's okay. And if you
use a comma-separated list like I have in the test (i.e. using A2::comma,
A1::comma) it seems to find the correct bits just fine. Unless I'm getting
really lucky and it's actually just a coincidence.

It seems that strip_using_decl doesn't return an overloaded list. Or, if it
does, the first entry in that list just so happens to always be the right
answer, so it can be treated like it's a regular decl for this purpose. For
example, even if we change up the order of baseclasses, using statements
and class definitions, it still seems to work, e.g. the following *seems*
to work just fine:

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

class A1
{
  protected:
  int separate();
  int comma();
};

class A3
{
  protected:
  int comma(int a, int b);
};

class B:private A3, private A1, private A2
{
  // Using decls in a comma-separated list.
  using A2::comma, A3::comma, A1::comma;  // { dg-message "declared" }
  // Separate using statements.
  using A2::separate;
  using A1::separate; // { dg-message "declared" }
  // No ambiguity, just for the sake of it.
  using A2::alone; // { dg-message "declared" }
};

class C:public B
{
  void f()
  {
comma(); // { dg-error "private" }
separate(); // { dg-error "private" }
alone = 5; // { dg-error "private" }
  }
};

Anthony


On Thu, 25 Feb 2021 at 03:37, Jason Merrill  wrote:

> On 2/24/21 4:17 PM, Anthony Sharp wrote:
> >> "special"
> >
> >
> > It wouldn't be my code if it didn't have sp3ling mstakes innit!
> > Actually to be fair I already changed that spelling mistake a few days
> > ago in my local code ;)
> >
> > I was actually thinking about this last night as I was falling asleep
> > (as you do) and I realised that the whole of my using decl lookup is
> > redundant. I can simply do this (formatting probably messes up here):
> >
> > /* 1.  If the "using" keyword is used to inherit DECL within the parent,
> >   this may cause DECL to be private, so we should return the using
> >   statement as the source of the problem.
> >
> >   Scan the fields of PARENT_BINFO and see if there are any using
> decls.  If
> >   there are, see if they inherit DECL.  If they do, that's where
> DECL must
> >   have been declared private.  */
> >
> >for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (parent_binfo));
> > parent_field;
> > parent_field = DECL_CHAIN (parent_field))
> >  {
> >/* Not necessary, but also check TREE_PRIVATE for the sake of
> >eliminating obviously non-relevant using decls.  */
> >if (TREE_CODE (parent_field) == USING_DECL
> >   && TREE_PRIVATE (parent_field))
> > {
> > /* If the using statement inherits DECL, it is the source of the
> >   access failure, so return it.  */
> >   if (cp_tree_equal (strip_using_decl (parent_field), decl))
> > return parent_field;
> > }
> >  }
> >
> > I was wrong to say that the using decl does not store "where it came
> > from/what it inherits" - that's exactly what strip_using_decl
> > achieves. I think the problem was that when I did my initial testing
> > in trying out ways to get the original decl, I didn't strip it, so the
> > comparison failed, which led me to make the whole redundant lookup,
> > blah blah blah.
> >
> > I've run a quick test and it seems to work, even with the overloads.
> >
> > Will test it some more and if all's good I will probably send a new
> > patch some time this weekend.
>
> Sounds good, though strip_using_decl (parent_field) may be overloaded if
> the using-decl brings in multiple functions with that name.
>
> Jason
>
>
From b57afd2da59c2ec14c13b76c39aba9126170078d Mon Sep 17 00:00:00 2001
From: Anthony Sharp 
Date: Mon, 1 Mar 2021 21:58:35 +
Subject: [PATCH] c++: Private parent access check for using decls [PR19377]

This bug was already mostly fixed by the patch for PR17314. This
patch continues that by ensuring that where a using decl is used,
causing an access failure to a child class because the using decl is
private, the compiler correctly points to the using decl as the
source of the problem.

gcc/cp/ChangeLog:

2021-03-01  Anthony Sharp  

	* semantics.c (get_class_access_diagnostic_decl): New
	function that examines special cases when a parent
	class causes a private access failure.
	(enforce_access): Slightly modified to call function
	above.

gcc/testsuite/ChangeLog:

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

[Anthony Sharp via Gcc-patches]

> "special"


It wouldn't be my code if it didn't have sp3ling mstakes innit!
Actually to be fair I already changed that spelling mistake a few days
ago in my local code ;)

I was actually thinking about this last night as I was falling asleep
(as you do) and I realised that the whole of my using decl lookup is
redundant. I can simply do this (formatting probably messes up here):

/* 1.  If the "using" keyword is used to inherit DECL within the parent,
 this may cause DECL to be private, so we should return the using
 statement as the source of the problem.

 Scan the fields of PARENT_BINFO and see if there are any using decls.  If
 there are, see if they inherit DECL.  If they do, that's where DECL must
 have been declared private.  */

  for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (parent_binfo));
   parent_field;
   parent_field = DECL_CHAIN (parent_field))
{
  /* Not necessary, but also check TREE_PRIVATE for the sake of
  eliminating obviously non-relevant using decls.  */
  if (TREE_CODE (parent_field) == USING_DECL
 && TREE_PRIVATE (parent_field))
{
/* If the using statement inherits DECL, it is the source of the
 access failure, so return it.  */
 if (cp_tree_equal (strip_using_decl (parent_field), decl))
   return parent_field;
}
}

I was wrong to say that the using decl does not store "where it came
from/what it inherits" - that's exactly what strip_using_decl
achieves. I think the problem was that when I did my initial testing
in trying out ways to get the original decl, I didn't strip it, so the
comparison failed, which led me to make the whole redundant lookup,
blah blah blah.

I've run a quick test and it seems to work, even with the overloads.

Will test it some more and if all's good I will probably send a new
patch some time this weekend.

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

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

Anthony

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

[Anthony Sharp via Gcc-patches]

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

Should say

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

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

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

[Anthony Sharp via Gcc-patches]

Hi all,

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

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

So we need to do a long-winded lookup.

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

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

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

Anyways, to summarise ...

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

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

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

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

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

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

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

Actual code attached.

Anthony


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

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

[Anthony Sharp via Gcc-patches]

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


A okay.

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


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

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


I will give that a try.

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


Am I reading this wrong then? :

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

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

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

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

This line needs one more space.


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

I will send over a new patch later/tomorrow.

Anthony

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

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

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

[Anthony Sharp via Gcc-patches]

Hi all,

I tried both:

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

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

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

which works. New patch attached.

Anthony

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

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

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

[Anthony Sharp via Gcc-patches]

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


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

Yup, this one.


Awesome.

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


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

Anthony



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

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

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

[Anthony Sharp via Gcc-patches]

Hi Marek,

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

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

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

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

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

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

> These two comments can be merged into one.

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

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

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

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

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

> This could be part of the if above.

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

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

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

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

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

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

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

> We usually use dg-do compile.

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

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

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

I'll probably send another patch over tomorrow.

Anthony


On Fri, 5 Feb 2021 at 16:06, Marek Polacek  wrote:
>
> Hi,
>
> a few comments:
>
> On Fri, Feb 05, 2021 at 03:39:25PM +, Anthony Sharp via Gcc-patches wrote:
> > 2021-02-05  Anthony Sharp  
> >
> >   * semantics.c (get_class_access_diagnostic_decl): New function.
> >   (enforce_access): Altered function.
>
> Pedantically, "Altered function." is not very good, it should say what
> in enforce_access changed.
>
> > gcc/testsuite/ChangeLog:
> >
> > 2021-02-05  Anthony Sharp  
> >
> >   * g++.dg/pr19377.C: New test.
>
> Let's move the test into g++.dg/cpp1z and call it using9.C.
>
> >  gcc/cp/semantics.c | 89 ++
> >  gcc/testsuite/g++.dg/pr19377.C | 28 +++
> >  2 files changed, 98 insertions(+), 19 deletions(-)
> >  create mode 100644 gcc/testsuite/g++.dg/pr19377.C
> >
> > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> > index 73834467fca..ffb627f08ea 100644
> > --- a/gcc/cp/semantics.c
> > +++ b/gcc/cp/semantics.c
> > @@ -256,6 +256,58 @@ pop_to_parent_deferring_access_checks (void)
> >  }
> >  }
> >
> > +/* Called from enforce_access.  A class has attempted (but failed) to 
> > access
> > +   DECL.  It is already established that a baseclass of that class,
> > +   PARENT_BINFO, has private access to DECL.  Examine certain special 
> > cases to
> > +   generate a diagnostic decl location.  If no special cases are found, 
> > simply
>
> I don't understand the "generate a diagnostic decl location".  Maybe just
> "generate a diagnostic?"
>
> > +   return DECL.  */
> > +
> > +static tree
> > +get_class_access_diagnostic_decl (tree parent_binfo, tree decl)
> > +{
> > +  /* When a class is denied access to a decl in a baseclass, most of the
> > + time it is because the decl itself was declared as private at the 
> > point
> > + of declaration.  So, by defau

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

[Anthony Sharp via Gcc-patches]

== is enough, identifiers are unique.
> >
> > >> Maybe also check that the using is TREE_PRIVATE?
> > >
> > > Would that be necessary? Maybe if you wanted to sanity-check it I
> > > suppose. We know for sure that PARENT_BINFO has private access to
> > > DECL. If we find a using statement introducing DECL in PARENT_BINFO,
> > > then surely the using statement must (by definition) have been
> > > private? If it were not private, then the child class would have been
> > > able to access it, and enforce_access wouldn't have thrown an error.
> > > It doesn't seem to be the case that DECL could be private for any
> > > other reason other than the using decl being private.
> >
> > Agreed, but the using-declaration might not be introducing DECL.  This
> > would be another way to skip irrelevant usings.
> >
> > > Let me know your thoughts and I will update the patch. Thanks for your 
> > > help.
> > >
> > > Anthony
> > >
> > >
> > > On Thu, 4 Feb 2021 at 16:33, Jason Merrill  wrote:
> > >>
> > >> On 2/4/21 11:24 AM, Jason Merrill wrote:
> > >>> On 2/4/21 10:02 AM, Anthony Sharp via Gcc-patches wrote:
> > >>>> Hello,
> > >>>>
> > >>>> New bugfix for PR19377
> > >>>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19377). This is
> > >>>> basically an extension of what I did before for PR17314 except it also
> > >>>> fixes this other bug.
> > >>>>
> > >>>> I hope I didn't over-comment in the code ... better to say too much
> > >>>> than too little! It's a niche bug so I thought it could do with a
> > >>>> little explanation.
> > >>>
> > >>> Yes, thanks; it would take a lot to make me request less comments.
> > >>>
> > >>>> +  if (TREE_CODE (parent_field) == USING_DECL)
> > >>>> +   {
> > >>>> + if (cp_tree_equal (decl,
> > >>>> +lookup_member (parent_binfo,
> > >>>> +   DECL_NAME (parent_field),
> > >>>> +   /*protect=*/0,
> > >>>> +   /*want_type=*/false,
> > >>>> +   tf_warning_or_error)))
> > >>>
> > >>> Isn't it sufficient to check that the names match?
> > >>
> > >> Well, I guess it could be using a declaration of the same name from
> > >> another base.  But in that case you could end up with an overload set
> > >> containing both the decl we're complaining about and another of the same
> > >> name from another base, in which case the lookup result would include
> > >> both, and so the comparison would fail and we would fall through to the
> > >> private base assumption.
> > >>
> > >> But checking the name is a simple way to skip irrelevant usings.
> > >> Maybe also check that the using is TREE_PRIVATE?
> > >>
> > >>>>tree parent_binfo = get_parent_with_private_access (decl,
> > >>>> -
> > >>>> basetype_path);
> > >>>> +
> > >>>> basetype_path);
> > >>> ...
> > >>>> + diag_location = get_class_access_diagnostic_decl
> > >>>> (parent_binfo,
> > >>>> +
> > >>>> diag_decl);
> > >>>
> > >>> The second lines of arguments are indented one space too far in both
> > >>> these calls.
> > >>>
> > >>> Jason
> > >>
> > >
> >
From 8136df415670eb24edc73595fb6d4485d600a5d2 Mon Sep 17 00:00:00 2001
From: Anthony Sharp 
Date: Fri, 5 Feb 2021 15:30:45 +
Subject: [PATCH] c++: Private parent access check for using decl [PR19377]

This bug was already mostly fixed by the patch for PR17314. This
patch continues that by ensuring that where a using decl is used,
causing an access failure to a child class because the using decl is
private, the compiler correctly points to the using decl as the
source of the problem.

Checks for the use of using decls in a parent that had
private access to decl (but the child had no access) in order
to ascertain the best diagnostic decl location.

gcc/cp/ChangeLog:

2021-02-05  Anthony Sharp  

	* semantics.c (get_class_access_diagnostic_decl): New funct

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

[Anthony Sharp via Gcc-patches]

> I'm imagining member functions, i.e. A::f() and A2::f(int).

You're right - good spot.

> == is enough, identifiers are unique.

 Done.

> Agreed, but the using-declaration might not be introducing DECL.  This
> would be another way to skip irrelevant usings.

Okie dokie.

New patch attached (that rhymes?). C++ (compiled only) compiles fine
and fixes the bug.

Also modified the new regression test to use your overloaded function
testcase. I made the testcase c++17 only ... I could also add a
pre-c++17 testcase but not sure if that would really be beneficial?
Anyways, regression tests (C++ only) now give:

-- G++ --

# of expected passes203708
# of unexpected failures2
# of expected failures989
# of unsupported tests8714

Anthony


On Thu, 4 Feb 2021 at 20:55, Jason Merrill  wrote:
>
> On 2/4/21 12:46 PM, Anthony Sharp wrote:
> >> Yes, thanks; it would take a lot to make me request less comments.
> >
> > Awesome.
> >
> >> The second lines of arguments are indented one space too far in both these 
> >> calls.
> >
> > Oops! I will fix that.
> >
> >> Well, I guess it could be using a declaration of the same name from 
> >> another base.
> >
> >   Yes I had been worrying about that.
> >
> >> But in that case you could end up with an overload set
> >> containing both the decl we're complaining about and another of the same
> >> name from another base, in which case the lookup result would include
> >> both, and so the comparison would fail and we would fall through to the
> >> private base assumption.
> >
> > I think technically yes ... but also no since such code would not
> > compile anyways, plus oddly it seems to work anyway. For instance
> > (assuming I'm understanding correctly), if you do this (with the patch
> > applied):
> >
> > class A
> > {
> >protected:
> >int i;
> > };
> >
> > class A2
> > {
> >protected:
> >int i;
> > };
> >
> > class B:public A, public A2
> > {
> >private:
> >using A::i, A2::i;
> > };
> >
> > class C:public B
> > {
> >void f()
> >{
> >  A::i = 0;
> >}
> > };
> >
> > You get:
> >
> > error: redeclaration of ‘using A::i’
> > using A::i;
> >
> > note: previous declaration ‘using A2::i’
> >  using A2::i;
> >
> > error: redeclaration of ‘using A2::i’
> > using A::i, A2::i;
> >
> > previous declaration ‘using A::i’
> > using A::i, A2::i;
> >
> > In member function ‘void C::f()’:
> > error: ‘int A::i’ is private within this context
> > A::i = 0;
> >
> > note: declared private here
> > using A::i, A2::i;
> >
> > Which seems to work (well ... more like fail to compile ... as
> > expected). Maybe you're imagining a different situation to me?
>
> I'm imagining member functions, i.e. A::f() and A2::f(int).
>
> > You can even use void f() { i = 0; } and void f() { A2::i = 0; } and
> > you seem to get the same results either which way (although, to be
> > fair, if you do A2::i = 0, it suddenly doesn't complain about it being
> > private anymore [no idea why], it just complains about the
> > redeclaration , and once you fix the redeclaration, it THEN complains
> > about being private, so it's got a bit of a quirk - don't think that's
> > related to the patch though).
>
> That sounds fine, one error can hide another.
>
> >> But checking the name is a simple way to skip irrelevant usings.
> >
> > That does sound like a better way of doing it. Would I just do
> > cp_tree_equal (DECL_NAME (blah1), DECL_NAME (blah2)) [assuming
> > DECL_NAME returns a tree], or perhaps DECL_NAME (blah1) == DECL_NAME
> > (blah2)?
>
> == is enough, identifiers are unique.
>
> >> Maybe also check that the using is TREE_PRIVATE?
> >
> > Would that be necessary? Maybe if you wanted to sanity-check it I
> > suppose. We know for sure that PARENT_BINFO has private access to
> > DECL. If we find a using statement introducing DECL in PARENT_BINFO,
> > then surely the using statement must (by definition) have been
> > private? If it were not private, then the child class would have been
> > able to access it, and enforce_access wouldn't have thrown an error.
> > It doesn't seem to be the case that DECL could be private for any
> > other reason other than the using decl being private.
>
> Agreed, but the using-declaration might not be introducing DECL.  This
> would be anoth

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

[Anthony Sharp via Gcc-patches]

> Yes, thanks; it would take a lot to make me request less comments.

Awesome.

> The second lines of arguments are indented one space too far in both these 
> calls.

Oops! I will fix that.

> Well, I guess it could be using a declaration of the same name from another 
> base.

 Yes I had been worrying about that.

> But in that case you could end up with an overload set
> containing both the decl we're complaining about and another of the same
> name from another base, in which case the lookup result would include
> both, and so the comparison would fail and we would fall through to the
> private base assumption.

I think technically yes ... but also no since such code would not
compile anyways, plus oddly it seems to work anyway. For instance
(assuming I'm understanding correctly), if you do this (with the patch
applied):

class A
{
  protected:
  int i;
};

class A2
{
  protected:
  int i;
};

class B:public A, public A2
{
  private:
  using A::i, A2::i;
};

class C:public B
{
  void f()
  {
A::i = 0;
  }
};

You get:

error: redeclaration of ‘using A::i’
   using A::i;

note: previous declaration ‘using A2::i’
using A2::i;

error: redeclaration of ‘using A2::i’
   using A::i, A2::i;

previous declaration ‘using A::i’
   using A::i, A2::i;

In member function ‘void C::f()’:
error: ‘int A::i’ is private within this context
   A::i = 0;

note: declared private here
   using A::i, A2::i;

Which seems to work (well ... more like fail to compile ... as
expected). Maybe you're imagining a different situation to me?

You can even use void f() { i = 0; } and void f() { A2::i = 0; } and
you seem to get the same results either which way (although, to be
fair, if you do A2::i = 0, it suddenly doesn't complain about it being
private anymore [no idea why], it just complains about the
redeclaration , and once you fix the redeclaration, it THEN complains
about being private, so it's got a bit of a quirk - don't think that's
related to the patch though).

> But checking the name is a simple way to skip irrelevant usings.

That does sound like a better way of doing it. Would I just do
cp_tree_equal (DECL_NAME (blah1), DECL_NAME (blah2)) [assuming
DECL_NAME returns a tree], or perhaps DECL_NAME (blah1) == DECL_NAME
(blah2)?

> Maybe also check that the using is TREE_PRIVATE?

Would that be necessary? Maybe if you wanted to sanity-check it I
suppose. We know for sure that PARENT_BINFO has private access to
DECL. If we find a using statement introducing DECL in PARENT_BINFO,
then surely the using statement must (by definition) have been
private? If it were not private, then the child class would have been
able to access it, and enforce_access wouldn't have thrown an error.
It doesn't seem to be the case that DECL could be private for any
other reason other than the using decl being private.

Let me know your thoughts and I will update the patch. Thanks for your help.

Anthony


On Thu, 4 Feb 2021 at 16:33, Jason Merrill  wrote:
>
> On 2/4/21 11:24 AM, Jason Merrill wrote:
> > On 2/4/21 10:02 AM, Anthony Sharp via Gcc-patches wrote:
> >> Hello,
> >>
> >> New bugfix for PR19377
> >> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19377). This is
> >> basically an extension of what I did before for PR17314 except it also
> >> fixes this other bug.
> >>
> >> I hope I didn't over-comment in the code ... better to say too much
> >> than too little! It's a niche bug so I thought it could do with a
> >> little explanation.
> >
> > Yes, thanks; it would take a lot to make me request less comments.
> >
> >> +  if (TREE_CODE (parent_field) == USING_DECL)
> >> +   {
> >> + if (cp_tree_equal (decl,
> >> +lookup_member (parent_binfo,
> >> +   DECL_NAME (parent_field),
> >> +   /*protect=*/0,
> >> +   /*want_type=*/false,
> >> +   tf_warning_or_error)))
> >
> > Isn't it sufficient to check that the names match?
>
> Well, I guess it could be using a declaration of the same name from
> another base.  But in that case you could end up with an overload set
> containing both the decl we're complaining about and another of the same
> name from another base, in which case the lookup result would include
> both, and so the comparison would fail and we would fall through to the
> private base assumption.
>
> But checking the name is a simple way to skip irrelevant usings.
> Maybe also check that the using is TREE_PRIVATE?
>
> >>   tree parent_binfo = get_parent_with_private_access (decl,
> >> -
> >> basetype_path);
> >> +
> >> basetype_path);
> > ...
> >> + diag_location = get_class_access_diagnostic_decl
> >> (parent_binfo,
> >> +
> >> diag_decl);
> >
> > The second lines of arguments are indented one space too far in both
> > these calls.
> >
> > Jason
>

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

[Anthony Sharp via Gcc-patches]

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.

Added 1 new regression test.

Bootstraps fine on x86_64-pc-linux-gnu for x86_64-pc-linux-gnu.

Hopefully no formatting problems; checked with git gcc-verify and
check_GNU_style.sh.

--- REGRESSION ANALYSIS ---

No regressions reported.

I only ran the c++ regression tests since this is a c++ front-end
diagnostics code bug (i.e. it should have no effect on compilation, or
any other languages).

G++ (CLEAN) RESULTS

# of expected passes203705
# of unexpected failures2
# of expected failures989
# of unsupported tests8714

G++ (PATCHED) RESULTS

# of expected passes203717
# of unexpected failures2
# of expected failures989
# of unsupported tests8714

The extra passes are from my new regression test.

Let me know if there are any issues.

Kind regards,
Anthony Sharp
From e064f8d010baee288c47cce1981be80515101f0d Mon Sep 17 00:00:00 2001
From: Anthony Sharp 
Date: Thu, 4 Feb 2021 12:01:59 +
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 

Re: [PATCH] c++: private inheritance access diagnostics fix [PR17314]

[Anthony Sharp via Gcc-patches]

Hi Jason,

Attached changes. I just edited the patch file directly.

Kind regards,
Anthony
From 7984020f16e715017e62b8637d2e69c1aec3478a Mon Sep 17 00:00:00 2001
From: Anthony Sharp 
Date: Thu, 21 Jan 2021 15:26:25 +
Subject: [PATCH] c++: Private inheritance access diagnostics fix [PR17314]

This patch fixes PR17314. Previously, when class C attempted
to access member a declared in class A through class B, where class B
privately inherits from A and class C inherits from B, GCC would correctly
report an access violation, but would erroneously report that the reason was
because a was "protected", when in fact, from the point of view of class C,
it was really "private". This patch updates the diagnostics code to generate
more correct errors in cases of failed inheritance such as these.

The reason this bug happened was because GCC was examining the
declared access of decl, instead of looking at it in the
context of class inheritance.

gcc/cp/ChangeLog:

2021-01-21  Anthony Sharp  

	* call.c (complain_about_access): Altered function.
	* cp-tree.h (complain_about_access): Changed parameters of function.
	(get_parent_with_private_access): Declared new function.
	* search.c (get_parent_with_private_access): Defined new function.
	* semantics.c (enforce_access): Modified function.
	* typeck.c (complain_about_unrecognized_member): Updated function
	arguments in complain_about_access.

gcc/testsuite/ChangeLog:

2021-01-21  Anthony Sharp  

	* g++.dg/lookup/scoped1.C: Modified testcase to run successfully with
	changes.
	* g++.dg/tc1/dr142.C: Same as above.
	* g++.dg/tc1/dr52.C: Same as above.
	* g++.old-deja/g++.brendan/visibility6.C: Same as above.
	* g++.old-deja/g++.brendan/visibility8.C: Same as above.
	* g++.old-deja/g++.jason/access8.C: Same as above.
	* g++.old-deja/g++.law/access4.C: Same as above.
	* g++.old-deja/g++.law/visibility12.C: Same as above.
	* g++.old-deja/g++.law/visibility4.C: Same as above.
	* g++.old-deja/g++.law/visibility8.C: Same as above.
	* g++.old-deja/g++.other/access4.C: Same as above.
---
 gcc/cp/call.c | 38 ++-
 gcc/cp/cp-tree.h  |  4 +-
 gcc/cp/search.c   | 35 +
 gcc/cp/semantics.c| 31 ++-
 gcc/cp/typeck.c   |  3 +-
 gcc/testsuite/g++.dg/lookup/scoped1.C |  4 +-
 gcc/testsuite/g++.dg/tc1/dr142.C  |  8 ++--
 gcc/testsuite/g++.dg/tc1/dr52.C   |  6 +--
 .../g++.old-deja/g++.brendan/visibility6.C|  4 +-
 .../g++.old-deja/g++.brendan/visibility8.C|  4 +-
 .../g++.old-deja/g++.jason/access8.C  |  5 ++-
 gcc/testsuite/g++.old-deja/g++.law/access4.C  |  5 ++-
 .../g++.old-deja/g++.law/visibility12.C   |  7 ++--
 .../g++.old-deja/g++.law/visibility4.C|  5 ++-
 .../g++.old-deja/g++.law/visibility8.C|  5 ++-
 .../g++.old-deja/g++.other/access4.C  |  4 +-
 16 files changed, 129 insertions(+), 39 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index b6e9f125aeb..175a3d9b421 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7142,27 +7142,45 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
 /* Issue diagnostics about a disallowed access of DECL, using DIAG_DECL
in the diagnostics.
 
-   If ISSUE_ERROR is true, then issue an error about the
-   access, followed by a note showing the declaration.
-   Otherwise, just show the note.  */
+   If ISSUE_ERROR is true, then issue an error about the access, followed
+   by a note showing the declaration.  Otherwise, just show the note.
+
+   DIAG_DECL and DIAG_LOCATION will almost always be the same.
+   DIAG_LOCATION is just another DECL.  NO_ACCESS_REASON is an optional
+   parameter used to specify why DECL wasn't accessible (e.g. ak_private
+   would be because DECL was private).  If not using NO_ACCESS_REASON,
+   then it must be ak_none, and the access failure reason will be
+   figured out by looking at the protection of DECL.  */
 
 void
-complain_about_access (tree decl, tree diag_decl, bool issue_error)
+complain_about_access (tree decl, tree diag_decl, tree diag_location,
+bool issue_error, access_kind no_access_reason)
 {
-  if (TREE_PRIVATE (decl))
+  /* If we have not already figured out why DECL is inaccessible...  */
+  if (no_access_reason == ak_none)
+{
+  /* Examine the access of DECL to find out why.  */
+  if (TREE_PRIVATE (decl))
+	no_access_reason = ak_private;
+  else if (TREE_PROTECTED (decl))
+	no_access_reason = ak_protected;
+}
+
+  /* Now generate an error message depending on calculated access.  */
+  if (no_access_reason == ak_private)
 {
   if (issue_error)
 	error ("%q#D is private within this context", diag_decl);
-  inform (DECL_SOURCE_LOCATION (diag_decl),
-	  "declared private here");
+  inform (DECL_SOURCE_LOCATION (diag_location), "declared private here");
 }
-  else if 

Re: [PATCH] c++: private inheritance access diagnostics fix [PR17314]

[Anthony Sharp via Gcc-patches]

Hi Jason,

Thanks for getting back to me so quickly.

> Why two gcc-comit-mklog?  That would generate the log entries twice.

It did in fact generate the log entries twice, but I deleted out the second
copy. Perhaps it would have made more sense to do git commit --amend
instead.

> Instead of making access_in_type non-static, let's defiine
> get_parent_with_private_access in search.c and declare it in cp-tree.h
> (with the declarations of nearby search.c functions).

Done.

> Only one 'n' in inaccessible.

Oops!

Subsequent lines of a comment should be indented to line up with the
> first line.  This applies to all your multi-line comments.


My bad, hopefully fixed now.

Don't change the indentation of these blocks; in the GNU coding style
> the { } are indented two spaces from the if.
>

I think I see what you mean; I forgot to indent the { } (and therefore also
everything within it, by two spaces). Hopefully fixed.

The new line of arguments should be indented to line up with the first one.
>

Fixed I think.

Please find attached the latest patch version with all these changes. git
gcc-verify returns no problems and check_GNU_style.sh returns only false
positives. Source builds fine. To be super safe I re-cloned the source and
did git apply with the patch and it built and worked just fine, and
hopefully I haven't missed anything.

Thanks again for your help.

Kind regards,
Anthony
From 7984020f16e715017e62b8637d2e69c1aec3478a Mon Sep 17 00:00:00 2001
From: Anthony Sharp 
Date: Thu, 21 Jan 2021 15:26:25 +
Subject: [PATCH] This patch fixes PR17314. Previously, when class C attempted
 to access member a declared in class A through class B, where class B
 privately inherits from A and class C inherits from B, GCC would correctly
 report an access violation, but would erroneously report that the reason was
 because a was "protected", when in fact, from the point of view of class C,
 it was really "private". This patch updates the diagnostics code to generate
 more correct errors in cases of failed inheritance such as these.

The reason this bug happened was because GCC was examining the
declared access of decl, instead of looking at it in the
context of class inheritance.

gcc/cp/ChangeLog:

2021-01-21  Anthony Sharp  

	* call.c (complain_about_access): Altered function.
	* cp-tree.h (complain_about_access): Changed parameters of function.
	(get_parent_with_private_access): Declared new function.
	* search.c (get_parent_with_private_access): Defined new function.
	* semantics.c (enforce_access): Modified function.
	* typeck.c (complain_about_unrecognized_member): Updated function
	arguments in complain_about_access.

gcc/testsuite/ChangeLog:

2021-01-21  Anthony Sharp  

	* g++.dg/lookup/scoped1.C: Modified testcase to run successfully with
	changes.
	* g++.dg/tc1/dr142.C: Same as above.
	* g++.dg/tc1/dr52.C: Same as above.
	* g++.old-deja/g++.brendan/visibility6.C: Same as above.
	* g++.old-deja/g++.brendan/visibility8.C: Same as above.
	* g++.old-deja/g++.jason/access8.C: Same as above.
	* g++.old-deja/g++.law/access4.C: Same as above.
	* g++.old-deja/g++.law/visibility12.C: Same as above.
	* g++.old-deja/g++.law/visibility4.C: Same as above.
	* g++.old-deja/g++.law/visibility8.C: Same as above.
	* g++.old-deja/g++.other/access4.C: Same as above.
---
 gcc/cp/call.c | 38 ++-
 gcc/cp/cp-tree.h  |  4 +-
 gcc/cp/search.c   | 35 +
 gcc/cp/semantics.c| 31 ++-
 gcc/cp/typeck.c   |  3 +-
 gcc/testsuite/g++.dg/lookup/scoped1.C |  4 +-
 gcc/testsuite/g++.dg/tc1/dr142.C  |  8 ++--
 gcc/testsuite/g++.dg/tc1/dr52.C   |  6 +--
 .../g++.old-deja/g++.brendan/visibility6.C|  4 +-
 .../g++.old-deja/g++.brendan/visibility8.C|  4 +-
 .../g++.old-deja/g++.jason/access8.C  |  5 ++-
 gcc/testsuite/g++.old-deja/g++.law/access4.C  |  5 ++-
 .../g++.old-deja/g++.law/visibility12.C   |  7 ++--
 .../g++.old-deja/g++.law/visibility4.C|  5 ++-
 .../g++.old-deja/g++.law/visibility8.C|  5 ++-
 .../g++.old-deja/g++.other/access4.C  |  4 +-
 16 files changed, 129 insertions(+), 39 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index b6e9f125aeb..175a3d9b421 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7142,27 +7142,45 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
 /* Issue diagnostics about a disallowed access of DECL, using DIAG_DECL
in the diagnostics.
 
-   If ISSUE_ERROR is true, then issue an error about the
-   access, followed by a note showing the declaration.
-   Otherwise, just show the note.  */
+   If ISSUE_ERROR is true, then issue an error about the access, followed
+   by a note showing the declaration.  Otherwise, just show the note.
+
+   DIAG_DECL and DIAG_LOCATION will almost always be the same.
+   DIAG_LOCATION 

Re: [PATCH] c++: private inheritance access diagnostics fix [PR17314]

[Anthony Sharp via Gcc-patches]

Hi Jason,

I've finally completed my copyright assignment form. I've attached it
to this email for reference.

> You don't need write access to the main repository to use these commands
> on your local copy.  One nice thing about git compared to svn is that
> you don't need to touch the server for anything but push and pull.
>
> Incidentally, how are you producing your patch?  Maybe try git
> format-patch instead.

The method I am using at the moment is the one Ranjit Mathew talks
about here: http://rmathew.com/articles/gcj/crpatch.html. Actually,
having just re-read it, it says: 'NOTE: This is not the “proper” or
“official” way of creating and submitting patches - that process has
been explained in detail elsewhere. That process requires one to use
Subversion (SVN). The process described here is meant for “one-off
hackers” or people who cannot use SVN for some reason or the other.'
... oops!

It's my fault kind of - the official GCC webpage
(https://gcc.gnu.org/gitwrite.html) explaining how to do it is called
'Read-write Git access' so I assumed it was only relevant for people
who have access to the repo, but I see that is not the case.

I've tried the git way of doing it and I'm attaching a new patch file
that (hopefully) is better this time. Basically what I did was what
you suggested:

git pull
contrib/gcc-git-customization.sh
(make changes)
git add *
git gcc-commit-mklog
git gcc-commit-mklog --amend
git format-patch -1 master

I also re-built the source just to make sure I hadn't messed anything
up. I re-ran the C++ regression tests using make check-c and make
check-c++. Whilst I did not do a before/after comparison of the
results, I checked the FAILs in gcc.sum and g++.sum and they all
looked like they had nothing to do with my code. All the code is the
same as before, so I'm thinking it should be fine (I just wanted to be
safe). Also checked against check_GNU_style.sh.

Assuming that's all fine, as for the code itself, there might well be
some tweaks that could make it better, and so if that is the case then
please let me know.

Kind regards,
Anthony Sharp


Sharp.1672871.GCC.pdf
Description: Adobe PDF document
From 9f7fa0b4a892f717974d79a6a56a5f8a8a8d9943 Mon Sep 17 00:00:00 2001
From: Anthony Sharp 
Date: Thu, 21 Jan 2021 15:26:25 +
Subject: [PATCH] gcc/cp/ChangeLog:

	2021-1-21  Anthony Sharp  
	Fixes PR17314
	* call.c (complain_about_access): Altered function.
	* cp-tree.h (complain_about_access): Changed parameters of function.
	* search.c (access_in_type): Made function non-static so it can be
	used in semantics.c.
	* semantics.c (access_in_type): Added as extern function.
	(get_parent_with_private_access): Added function.
	(enforce_access): Modified function.
	* typeck.c (complain_about_unrecognized_member): Updated function
	arguments in complain_about_access.

gcc/testsuite/ChangeLog:

	2021-1-21  Anthony Sharp  
	Changes required due to PR17314 fix
	* g++.dg/lookup/scoped1.C: Modified testcase to run successfully with
	changes.
	* g++.dg/tc1/dr142.C: Same as above.
	* g++.dg/tc1/dr52.C: Same as above.
	* g++.old-deja/g++.brendan/visibility6.C: Same as above.
	* g++.old-deja/g++.brendan/visibility8.C: Same as above.
	* g++.old-deja/g++.jason/access8.C: Same as above.
	* g++.old-deja/g++.law/access4.C: Same as above.
	* g++.old-deja/g++.law/visibility12.C: Same as above.
	* g++.old-deja/g++.law/visibility4.C: Same as above.
	* g++.old-deja/g++.law/visibility8.C: Same as above.
	* g++.old-deja/g++.other/access4.C: Same as above.
---
 gcc/cp/call.c | 64 ++---
 gcc/cp/cp-tree.h  |  3 +-
 gcc/cp/search.c   |  4 +-
 gcc/cp/semantics.c| 68 ++-
 gcc/cp/typeck.c   |  3 +-
 gcc/testsuite/g++.dg/lookup/scoped1.C |  4 +-
 gcc/testsuite/g++.dg/tc1/dr142.C  |  8 +--
 gcc/testsuite/g++.dg/tc1/dr52.C   |  6 +-
 .../g++.old-deja/g++.brendan/visibility6.C|  4 +-
 .../g++.old-deja/g++.brendan/visibility8.C|  4 +-
 .../g++.old-deja/g++.jason/access8.C  |  5 +-
 gcc/testsuite/g++.old-deja/g++.law/access4.C  |  5 +-
 .../g++.old-deja/g++.law/visibility12.C   |  7 +-
 .../g++.old-deja/g++.law/visibility4.C|  5 +-
 .../g++.old-deja/g++.law/visibility8.C|  5 +-
 .../g++.old-deja/g++.other/access4.C  |  4 +-
 16 files changed, 145 insertions(+), 54 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index b6e9f125aeb..fc2f1c6226c 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7142,33 +7142,51 @@ build_op_delete_call (enum tree_code code, tree addr, tree size,
 /* Issue diagnostics about a disallowed access of DECL, using DIAG_DECL
in the diagnostics.
 
-   If ISSUE_ERROR is true, then issue an error about the
-   access, followed by a note showing the declaration.
-   Otherwise, just show the note.  */
+   If ISSUE_ERROR is true, then issue an error about 

Re: [PATCH] c++: private inheritance access diagnostics fix [PR17314]

[Anthony Sharp via Gcc-patches]

Hi Jason,

Thank you!

> To start with, do you have a copyright assignment on file or in the
> works already?

Good point. I incorrectly assumed it would only be a minor
contribution copyright-wise. Mr Edelsohn gave me a template which I've
now filled out and sent to ass...@gnu.org. I'm assuming I just need to
wait for them to send me the form. I'll update this thread when that's
sorted. In the meantime I've hopefully fixed some of the issues.

> Second, your patch was mangled by word wrap so that it can't be applied
> without manual repair.  If you can't prevent word wrap in your mail
> client, please send it as an attachment rather than inline.

Oh yes I see where it's gotten mangled now. I'm attaching it as a
.patch file (I assume that's okay).

> Also, there are a few whitespace issues in the patch; please run
> contrib/check_GNU_style.sh on the patch before submitting.

Should be all fixed now (there is one style issue left but it's a
false positive). Visual Studio Code was lying to me about what the
file looks like so if there are any more formatting issues please let
me know.

> If you use contrib/gcc-git-customization.sh and then git
> gcc-commit-mklog you don't need to touch ChangeLog files at all, just
> adjust the generated ChangeLog entries in the git commit message.  I
> personally tend to commit first with a placeholder message and then use
> git gcc-commit-mklog --amend to generate the ChangeLog entries.

Wouldn't that require read-write access? (Just from looking here
https://gcc.gnu.org/gitwrite.html.)

> Probably.  Can you use sort/uniq/diff on the .sum testsuite output to
> determine which passes are missing in the patched sources?

According to contrib/dg-cmp-results.sh ...

I get a bunch of these weird NA->PASSes (and vice-versa), for example:

PASS->NA: g++.dg/modules/alias-1_a.H module-cmi
(gcm.cache/home/anthony/Desktop/GCC/builds_and_source/source_clean/gcc/testsuite/g++.dg/modules/alias-1_a.H.gcm)
NA->PASS: g++.dg/modules/alias-1_a.H module-cmi
(gcm.cache/home/anthony/Desktop/GCC/builds_and_source/source_pr17314/gcc/testsuite/g++.dg/modules/alias-1_a.H.gcm)
PASS->NA: g++.dg/modules/alias-1_a.H module-cmi
(gcm.cache/home/anthony/Desktop/GCC/builds_and_source/source_clean/gcc/testsuite/g++.dg/modules/alias-1_a.H.gcm)
NA->PASS: g++.dg/modules/alias-1_a.H module-cmi
(gcm.cache/home/anthony/Desktop/GCC/builds_and_source/source_pr17314/gcc/testsuite/g++.dg/modules/alias-1_a.H.gcm)

They're weird because I haven't actually touched those files (so I'm
assuming this is normal). There are about ~400 of those and they're
all .gcm files. They seem to balance out.

dr142.c reports:

NA->PASS: g++.dg/tc1/dr142.C  -std=c++14  (test for warnings, line 11)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++14  (test for warnings, line 5)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++14  (test for warnings, line 7)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++14  (test for warnings, line 8)
NA->PASS: g++.dg/tc1/dr142.C  -std=c++17  (test for warnings, line 11)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++17  (test for warnings, line 5)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++17  (test for warnings, line 7)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++17  (test for warnings, line 8)
NA->PASS: g++.dg/tc1/dr142.C  -std=c++2a  (test for warnings, line 11)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++2a  (test for warnings, line 5)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++2a  (test for warnings, line 7)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++2a  (test for warnings, line 8)
NA->PASS: g++.dg/tc1/dr142.C  -std=c++98  (test for warnings, line 11)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++98  (test for warnings, line 5)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++98  (test for warnings, line 7)
PASS->NA: g++.dg/tc1/dr142.C  -std=c++98  (test for warnings, line 8)

In other words, there are 12 PASS->NAs and 4 NA->PASSes in this file,
meaning a net change of -8 (which explains why there are eight fewer).
My other changes also report PASS->NAs and vice-versa, but for those
the number of new NAs equals the number of new PASSes, so they don't
cause a change in quantity.

Thanks for being patient with me. I'll let you know when I've
completed the forms.

Also if I need to adjust the .patch to deal with the changelogs issue
please let me know.

Kind regards,
Anthony
Index: gcc/testsuite/g++.old-deja/g++.jason/access8.C
===
--- gcc/testsuite/g++.old-deja/g++.jason/access8.C	2020-12-31 16:51:34.0 +
+++ gcc/testsuite/g++.old-deja/g++.jason/access8.C	2021-01-03 00:22:14.969854000 +
@@ -4,5 +4,5 @@
 // Bug: g++ forgets access decls after the definition.
 
-class inh { // { dg-message "" } inaccessible
+class inh { 
 int a;
 protected:
@@ -10,5 +10,6 @@ protected:
 };
 
-class mel : private inh {
+class mel : private inh // { dg-message "" } inaccessible
+{
 protected:
 int t;
Index: gcc/testsuite/g++.old-deja/g++.law/access4.C

[PATCH] c++: private inheritance access diagnostics fix [PR17314]

[Anthony Sharp via Gcc-patches]

This patch fixes PR17314 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17314).
Previously, when class C attempted to access member a declared in class A
through class B, where class B privately inherits from A and class C inherits
from B, GCC would correctly report an access violation, but would erroneously
report that the reason was because a was "protected", when in fact, from the
point of view of class C, it was "private". This patch updates the
diagnostics code to generate more correct errors in cases of failed
inheritance such as these.

The reason this bug happened was because GCC was examining the
declared access of decl, instead of looking at it in the context of class
inheritance.

--- COMMENTS ---

This is my first GCC patch ever so there is probably something I have done
very wrong. Please let me know :) The thought of my code being scrutinised
by people with PhDs and doctorates is quite frankly terrifying.

Note that since it is a new year I had to make a new changelog file so the
diff for the patch might be slightly off.

There was no need to add additional regression tests since it was adequate
to simply change some of the regression tests that were there originally
(all the patch changes is the informative message telling the user where
a decl was defined as private).

--- REGRESSION ANALYSIS ---

No regressions reported.

G++ (CLEAN) RESULTS

# of expected passes202879
# of unexpected failures1
# of expected failures988
# of unsupported tests8654

GCC (CLEAN) RESULTS

# of expected passes163377
# of unexpected failures94
# of unexpected successes37
# of expected failures915
# of unsupported tests2530

G++ (PR17314 PATCHED) RESULTS

# of expected passes202871
# of unexpected failures1
# of expected failures988
# of unsupported tests8654

GCC (PR17314 PATCHED) RESULTS

# of expected passes163377
# of unexpected failures94
# of unexpected successes37
# of expected failures915
# of unsupported tests2530

When I build and make -k check -j 6 on the patched source it reports
202871 passes (8 fewer), although the FAILs do not increase. I am not 100%
sure why this happens since I have not removed any testcases, only edited a
few, but I think this happens because in files like dr142.c I removed more
output checks than I added. make -k check -j 6 also returns error 2
sometimes, although there are no obvious errors or warnings in the logs
explaining why. Probably harmless?

--- BUILD REPORT ---

GCC builds normally on x86_64-pc-linux-gnu for x86_64-pc-linux-gnu using
make -j 6. I didn't see it necessary to test on other build targets since the
patch only affects the C++ front end and so functionality is unlikely
to differ between platforms.

The compile log reports:

Comparing stages 2 and 3
warning: gcc/cc1obj-checksum.o differs
Comparison successful.

and then continues. I assume this means it was actually successful.






Index: gcc/cp/ChangeLog
from  Anthony Sharp  

Fixes PR17314
* typeck.c (complain_about_unrecognized_member): Updated function
arguments in complain_about_access.
* call.c (complain_about_access): Altered function.
* semantics.c (get_parent_with_private_access): Added function.
(access_in_type): Added as extern function.
* search.c (access_in_type): Made function non-static so it can be
used in semantics.c.
* cp-tree.h (complain_about_access): Changed parameters of function.
Index: gcc/testsuite/ChangeLog
from  Anthony Sharp  

Fixes PR17314
* g++.dg/lookup/scoped1.c modified testcase to run successfully with
changes.
* g++.dg/tc1/dr142.c modified testcase to run successfully with
changes.
* g++.dg/tc1/dr142.c modified testcase to run successfully with
changes.
* g++.dg/tc1/dr142.c modified testcase to run successfully with
changes.
* g++.dg/tc1/dr52.c modified testcase to run successfully with changes.
* g++.old-deja/g++.brendan/visibility6.c modified testcase to run
successfully with changes.
* g++.old-deja/g++.brendan/visibility8.c modified testcase to run
successfully with changes.
* g++.old-deja/g++.jason/access8.c modified testcase to run
successfully with changes.
* g++.old-deja/g++.law/access4.c modified testcase to run successfully
with changes.
* g++.old-deja/g++.law/visibility12.c modified testcase to run
successfully with changes.
* g++.old-deja/g++.law/visibility4.c modified testcase to run
successfully with changes.
* g++.old-deja/g++.law/visibility8.c modified testcase to run
successfully with changes.
* g++.old-deja/g++.other/access4.c modified testcase to run
successfully with changes.
Index: gcc/testsuite/g++.old-deja/g++.jason/access8.C
===
--- gcc/testsuite/g++.old-deja/g++.jason/access8.C