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 <ja...@redhat.com> wrote:
>
> On 2/9/21 6:18 AM, Anthony Sharp wrote:
> > The parens are to help the editor line up the last line properly. If
> > the subexpression fits on one line, the parens aren't needed.
> >
> >
> > Ahhhh okay.
> >
> > No; "compile" means translate from C++ to assembly, "assemble" means
> > that, plus call the assembler to produce an object file. Though since
> > compilation errors out, assembling never happens.
> >
> >
> > lol I was being dumb and thinking it was the other way around. I will
> > change it.
> >
> > You could bring back your lookup from the earlier patch and look
> > through the result to see if the function we're complaining about is
> > part of what the particular using-decl brings in.
> >
> >
> > I will give that a try.
> >
> > I think you want
> > SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
> > BINFO_TYPE (parent_binfo))
> >
> >
> > Am I reading this wrong then? :
> >
> > /* Compare a BINFO_TYPE with another type for equality.
> > For a binfo, this is functionally equivalent to using same_type_p, but
> > measurably faster.
> > At least one of the arguments must be a BINFO_TYPE. The other can be
> > a BINFO_TYPE
> > or a regular type. If BINFO_TYPE(T) ever stops being the main variant
> > of the class the
> > binfo is for, this macro must change. */
> > #define SAME_BINFO_TYPE_P(A, B) ((A) == (B))
> >
> > That leads me to believe that arguments A and B can be: BINFO, BINFO ...
> > or BINFO, TYPE ... or TYPE, BINFO. If so I don't see why this doesn't work:
> >
> > (SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))
> > and
> > (SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)),
> > parent_binfo))
> >
> > Unless "BINFO_TYPE" is actually just how you refer to a regular class
> > type and not a BINFO. Seems a bit confusing to me.
>
> The comment is pretty conusing, I agree. But what it's saying is that
> both arguments must be types, and one of those types must be BINFO_TYPE
> (some_binfo).
>
> Your first line doesn't work because you're comparing a type and a
> binfo. The second one doesn't work because you're comparing the binfo
> for a most-derived object of the type to the binfo for a base subobject
> of the same type, and those are different, because binfos represent
> nodes in inheritance graphs.
>
> > This line needs one more space.
> >
> >
> > Oh I see ... so second line's arguments need to line up with the first
> > argument, not the first (.
> >
> > I will send over a new patch later/tomorrow.
> >
> > Anthony
> >
> > On Tue, 9 Feb 2021 at 04:55, Jason Merrill <ja...@redhat.com
> > <mailto:ja...@redhat.com>> wrote:
> >
> > On 2/5/21 12:28 PM, Anthony Sharp wrote:
> > > Hi Marek,
> > >
> > >>> + if ((TREE_CODE (parent_field) == USING_DECL)
> > >>
> > >> This first term doesn't need to be wrapped in ().
> > >
> > > I normally wouldn't use the (), but I think that's how Jason likes it
> > > so that's why I did it. I guess it makes it more readable.
> >
> > Ah, no, I don't see any need for the extra () there. When I asked for
> > added parens previously:
> >
> > >> + if (parent_binfo != NULL_TREE
> > >> + && context_for_name_lookup (decl)
> > >> + != BINFO_TYPE (parent_binfo))
> > >
> > > Here you want parens around the second != expression and its != token
> > > aligned with "context"
> >
> > The parens are to help the editor line up the last line properly. If
> > the subexpression fits on one line, the parens aren't needed.
> >
> > >> We usually use dg-do compile.
> > >
> > > True, but wouldn't that technically be slower? I would agree if it
> > > were more than just error-handling code.
> >
> > No; "compile" means translate from C++ to assembly, "assemble" means
> > that, plus call the assembler to produce an object file. Though since
> > compilation errors out, assembling never happens.
> >
> > > + if ((TREE_CODE (parent_field) == USING_DECL)
> > > + && TREE_PRIVATE (parent_field)
> > > + && (DECL_NAME (decl) == DECL_NAME (parent_field)))
> > > + return parent_field;
> >
> > Following our discussion about an earlier revision, this will often
> > produce the right using-declaration, but might not if two functions of
> > the same name are imported from different bases. If I split the
> > using-declaration in using9.C into two, i.e.
> >
> > > using A2::i; // { dg-message "declared" }
> > > using A::i;
> >
> > then I get
> >
> > > wa.ii: In member function ‘void C::f()’:
> > > wa.ii:28:7: error: ‘int A::i()’ is private within this context
> > > 28 | i();
> > > | ^
> > > wa.ii:20:13: note: declared private here
> > > 20 | using A2::i;
> >
> > pointing out the wrong using-declaration because it happens to be
> > first.
> > You could bring back your lookup from the earlier patch and look
> > through the result to see if the function we're complaining about is
> > part of what the particular using-decl brings in.
> >
> > > I tried both:
> > > (SAME_BINFO_TYPE_P (context_for_name_lookup (decl), parent_binfo))
> > > and
> > > (SAME_BINFO_TYPE_P (TYPE_BINFO (context_for_name_lookup (decl)),
> > parent_binfo))
> >
> > I think you want
> >
> > SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
> > BINFO_TYPE (parent_binfo))
> >
> > i.e. just change same_type_p to SAME_BINFO_TYPE_P.
> >
> > > tree parent_binfo = get_parent_with_private_access (decl,
> > > -
> > basetype_path);
> > > +
> > basetype_path);
> >
> > This line was indented properly before, and is changed to be indented
> > one space too far.
> >
> > > + diag_location = get_class_access_diagnostic_decl
> > (parent_binfo,
> > > +
> > diag_decl);
> >
> > This line needs one more space.
> >
> > > complain_about_access (decl, diag_decl, diag_location,
> > true,
> > > - parent_access);
> > > + access_failure_reason);
> >
> > This line, too.
> >
> > > +};
> > > \ No newline at end of file
> >
> > Let's add a newline.
> >
> > Jason
> >
>
/* { dg-do compile } */
// Created for c++ PR19377
class A1
{
protected:
int separate();
int comma();
};
class A2
{
protected:
int separate(int a);
int comma(int a);
int alone;
};
class B:private A1, private A2
{
// Using decls in a comma-separated list.
using A2::comma, A1::comma; // { dg-message "declared" }
// Separate using statements.
using A2::separate;
using A1::separate; // { dg-message "declared" }
// No ambiguity, just for the sake of it.
using A2::alone;
};
class C:public B
{
void f()
{
comma(); // { dg-error "private" }
separate(); // { dg-error "private" }
alone = 5; // { dg-error "private" }
}
};
/* This function may seem dumb, but actually it is possible for two decls that
are equal to really be slightly different. For example, they might be
declared in different places, such as is the case when a using statement
inherits a decl.
Scan the fields of BINFO for an exact match of DECL. If found, return
DECL. Otherwise, return NULL_TREE. DECL is really the type "tree". */
tree
lookup_decl_exact_match(tree binfo, void *decl)
{
for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (binfo));
parent_field;
parent_field = DECL_CHAIN (parent_field))
{
if(cp_tree_equal ((tree)decl, parent_field))
return parent_field;
}
return NULL_TREE;
}
/* Return the decl that DECL (which is a non-stripped using decl) inherits.
DECL is a member of DECLS_CLASS, which is a BINFO. On failure, return
NULL_TREE. */
tree
find_decl_using_decl_inherits (tree decl, tree decls_class)
{
/* Strip DECL to extract the actual decl. Contrary to the name, it only
returns 1 decl. */
tree decl_stripped = strip_using_decl (decl);
/* The only way to find the decl is to walk all the members of all the
bases. */
tree result = dfs_walk_once (decls_class, lookup_decl_exact_match, NULL,
decl_stripped);
/* If nothing found. */
if (!result)
return NULL_TREE;
return result;
}
/* Called from enforce_access. A class has attempted (but failed) to access
DECL. It is already established that a baseclass of that class,
PARENT_BINFO, has private access to DECL. Examine certain sepcial cases
to find the decl that is the source of the problem. If no special cases
are found, simply return DECL. */
static tree
get_class_access_diagnostic_decl (tree parent_binfo, tree decl)
{
/* When a class is denied access to a decl in a baseclass, most of the
time it is because the decl itself was declared as private at the point
of declaration. So, by default, DECL is at fault.
However, in C++, there are (at least) two situations in which a decl
can be private even though it was not originally defined as such. */
/* These two situations only apply if a baseclass had private access to
DECL (this function is only called if that is the case). We must however
also ensure that the reason the parent had private access wasn't simply
because it was declared as private in that parent. */
if (SAME_BINFO_TYPE_P (context_for_name_lookup (decl),
BINFO_TYPE (parent_binfo)))
return decl;
/* 1. If the "using" keyword is used to inherit DECL within a baseclass,
this may cause DECL to be private, so we return the using statement as
the source of the problem.
Scan the fields of PARENT_BINFO and see if there are any using decls. If
there are, see if they inherit DECL. If they do, that's where DECL must
have been declared private. */
for (tree parent_field = TYPE_FIELDS (BINFO_TYPE (parent_binfo));
parent_field;
parent_field = DECL_CHAIN (parent_field))
{
if (TREE_CODE (parent_field) == USING_DECL
&& TREE_PRIVATE (parent_field))
{
/* Find what this using decl inherited. */
tree original_decl =
find_decl_using_decl_inherits (parent_field, parent_binfo);
/* If parent_field inherits DECL, it is the source of the problem,
so return it. */
if (cp_tree_equal(original_decl, decl))
return parent_field;
}
}
/* 2. If decl was privately inherited by a baseclass of the current class,
then decl will be inaccessible, even though it may originally have
been accessible to deriving classes. In that case, the fault lies with
the baseclass that used a private inheritance, so we return the
baseclass type as the source of the problem.
Since this is the last check, we just assume it's true. */
return TYPE_NAME (BINFO_TYPE (parent_binfo));
}
/* If the current scope isn't allowed to access DECL along
BASETYPE_PATH, give an error, or if we're parsing a function or class
template, defer the access check to be performed at instantiation time.
The most derived class in BASETYPE_PATH is the one used to qualify DECL.
DIAG_DECL is the declaration to use in the error diagnostic. */
static bool
enforce_access (tree basetype_path, tree decl, tree diag_decl,
tsubst_flags_t complain, access_failure_info *afi = NULL)
{
gcc_assert (TREE_CODE (basetype_path) == TREE_BINFO);
if (flag_new_inheriting_ctors
&& DECL_INHERITED_CTOR (decl))
{
/* 7.3.3/18: The additional constructors are accessible if they would be
accessible when used to construct an object of the corresponding base
class. */
decl = strip_inheriting_ctors (decl);
basetype_path = lookup_base (basetype_path, DECL_CONTEXT (decl),
ba_any, NULL, complain);
}
tree cs = current_scope ();
if (processing_template_decl
&& (CLASS_TYPE_P (cs) || TREE_CODE (cs) == FUNCTION_DECL))
if (tree template_info = get_template_info (cs))
{
/* When parsing a function or class template, we in general need to
defer access checks until template instantiation time, since a friend
declaration may grant access only to a particular specialization of
the template. */
if (accessible_p (basetype_path, decl, /*consider_local_p=*/true))
/* But if the member is deemed accessible at parse time, then we can
assume it'll be accessible at instantiation time. */
return true;
/* Access of a dependent decl should be rechecked after tsubst'ing
into the user of the decl, rather than explicitly deferring the
check here. */
gcc_assert (!uses_template_parms (decl));
if (TREE_CODE (decl) == FIELD_DECL)
gcc_assert (!uses_template_parms (DECL_CONTEXT (decl)));
/* Defer this access check until instantiation time. */
deferred_access_check access_check;
access_check.binfo = basetype_path;
access_check.decl = decl;
access_check.diag_decl = diag_decl;
access_check.loc = input_location;
vec_safe_push (TI_DEFERRED_ACCESS_CHECKS (template_info), access_check);
return true;
}
if (!accessible_p (basetype_path, decl, /*consider_local_p=*/true))
{
if (flag_new_inheriting_ctors)
diag_decl = strip_inheriting_ctors (diag_decl);
if (complain & tf_error)
{
access_kind access_failure_reason = ak_none;
/* By default, using the decl as the source of the problem will
usually give correct results. */
tree diag_location = diag_decl;
/* However, if a parent of BASETYPE_PATH had private access to decl,
then it actually might be the case that the source of the problem
is not DECL. */
tree parent_binfo = get_parent_with_private_access (decl,
basetype_path);
/* So if a parent did have private access, then we need to do
special checks to obtain the best diagnostic location decl. */
if (parent_binfo != NULL_TREE)
{
diag_location = get_class_access_diagnostic_decl (parent_binfo,
diag_decl);
/* We also at this point know that the reason access failed was
because decl was private. */
access_failure_reason = ak_private;
}
/* Finally, generate an error message. */
complain_about_access (decl, diag_decl, diag_location, true,
access_failure_reason);
}
if (afi)
afi->record_access_failure (basetype_path, decl, diag_decl);
return false;
}
return true;
}
