On Fri, 22 Jan 2021, Jason Merrill wrote:
> On 1/22/21 12:59 PM, Patrick Palka wrote:
> > On Fri, 22 Jan 2021, Patrick Palka wrote:
> >
> > > On Fri, 22 Jan 2021, Patrick Palka wrote:
> > >
> > > > On Thu, 21 Jan 2021, Jason Merrill wrote:
> > > >
> > > > > On 1/21/21 11:22 AM, Patrick Palka wrote:
> > > > > > Here at parse time finish_qualified_id_expr adds an implicit
> > > > > > 'this->' to
> > > > > > the expression tmp::integral<T> (because it's type-dependent, and
> > > > > > also
> > > > > > current_class_ptr is set) within the trailing return type, and later
> > > > > > during substitution we can't resolve the 'this' since
> > > > > > tsubst_function_type does inject_this_parm only for non-static
> > > > > > member
> > > > > > functions which tmp::func is not.
> > > > > >
> > > > > > It seems the root of the problem is that we set current_class_ptr
> > > > > > when
> > > > > > parsing the signature of a static member function. Since explicit
> > > > > > uses
> > > > > > of 'this' are already not allowed in this context, we probably
> > > > > > shouldn't
> > > > > > be doing inject_this_parm either.
> > > > >
> > > > > Hmm, 'this' is defined in a static member function, it's just
> > > > > ill-formed to
> > > > > use it:
> > > > >
> > > > > 7.5.2/2: "... [this] shall not appear within the declaration of a
> > > > > static
> > > > > member function (although its type and value category are defined
> > > > > within a
> > > > > static member function as they are within a non-static member
> > > > > function).
> > > > > [Note: This is because declaration matching does not occur until the
> > > > > complete
> > > > > declarator is known. — end note]"
> > > >
> > > > Ah, I see.
> > > >
> > > > >
> > > > > Perhaps maybe_dummy_object needs to be smarter about recognizing
> > > > > static
> > > > > context. Or perhaps there's no way to tell the difference between the
> > > > > specified behavior above and the behavior with your patch, but:
> > > > >
> > > > > The note suggests that we need to test the case of an out-of-class
> > > > > definition
> > > > > of a static member function (template); we must inject_this_parm when
> > > > > parsing
> > > > > such a declaration, since we don't know it's static until we match it
> > > > > to the
> > > > > declaration in the class. I'm guessing that this would lead to the
> > > > > same
> > > > > problem.
> > > >
> > > > Good point, we do get a signature mismatch error in this case, due to
> > > > 'this->' appearing in the trailing return type out-of-class declaration
> > > > but not in that of the in-class declaration, so the trailing return
> > > > types don't match up. In light of this case, it doesn't seem possible
> > > > for maybe_dummy_object to distinguish between a static and non-static
> > > > context when parsing the trailing return type of the declaration.
> > > >
> > > > So perhaps we should mirror at substitution time what we do at parse
> > > > time, and inject 'this' also when substituting into the function type of
> > > > a static member function? That way, we'll resolve the use of 'this->'
> > > > and then discard it the RHS of -> is a static member function, or
> > > > complain that 'this' is not a constant expression if the RHS is a
> > > > non-static member function.
> > > >
> > > > The following patch performs that, and seems to pass light testing.
> > > > Full testing in progress. Revised commit message is still a WIP.
> > > > How does this approach look?
> >
> > Sorry for the spam, but I'd also like to propose a more conservative and
> > targeted approach that basically undoes part of the r9-5972 patch which
> > caused the regression.
> >
> > According to the email thread at
> > https://gcc.gnu.org/pipermail/gcc-patches/2019-February/516421.html
> > the hunk from r9-5972
> >
> > --- a/gcc/cp/semantics.c
> > +++ b/gcc/cp/semantics.c
> > @@ -2096,7 +2096,8 @@ finish_qualified_id_expr (tree qualifying_class,
> > {
> > /* See if any of the functions are non-static members. */
> > /* If so, the expression may be relative to 'this'. */
> > - if (!shared_member_p (expr)
> > + if ((type_dependent_expression_p (expr)
> > + || !shared_member_p (expr))
> > && current_class_ptr
> > && DERIVED_FROM_P (qualifying_class,
> > current_nonlambda_class_type ()))
> >
> > was added to avoid calling shared_member_p here when the overload set
> > contains a dependent USING_DECL. But according to
> > https://gcc.gnu.org/pipermail/gcc-patches/2018-December/513237.html if
> > an overload set contains a dependent USING_DECL, then it'll be the first
> > in the overload set. So we could partially revert the r9-5972 patch and
> > do:
> >
> > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> > index c6b4c70dc0f..6b0d68ae4bf 100644
> > --- a/gcc/cp/semantics.c
> > +++ b/gcc/cp/semantics.c
> > @@ -2214,7 +2214,7 @@ finish_qualified_id_expr (tree qualifying_class,
> > {
> > /* See if any of the functions are non-static members. */
> > /* If so, the expression may be relative to 'this'. */
> > - if ((type_dependent_expression_p (expr)
> > + if ((TREE_CODE (get_first_fn (expr)) == USING_DECL
> > || !shared_member_p (expr))
> > && current_class_ptr
> > && DERIVED_FROM_P (qualifying_class,
> >
> > The above pases 'make check-c++', and allows us to compile pr97399a.C
> > successfully, but we'd still ICE on the invalid pr97399b.C.
>
> What if we flipped the sense of the type_dependent_expression_p check, so we
> never add this-> if the function operand is dependent?
Looks like this doesn't survive 'make check-c++', we crash on the
testcase g++.dg/cpp1y/lambda-generic-this1a.C:
gcc/testsuite/g++.dg/cpp1y/lambda-generic-this1a.C: In instantiation of
‘MyType::crash()::<lambda(auto:1)> [with auto:1 = A]’:
gcc/testsuite/g++.dg/cpp1y/lambda-generic-this1a.C:12:10: required from here
gcc/testsuite/g++.dg/cpp1y/lambda-generic-this1a.C:9:28: internal compiler
error: trying to capture ‘this’ in instantiation of generic lambda
9 | MyType::make_crash<i>(); // Line (1)
| ~~~~~~~~~~~~~~~~~~~~~^~
0x9bab23 add_capture(tree_node*, tree_node*, tree_node*, bool, bool)
/home/patrick/gcc-master/gcc/cp/lambda.c:633
0x9bac75 add_default_capture(tree_node*, tree_node*, tree_node*)
/home/patrick/gcc-master/gcc/cp/lambda.c:693
0x9bb3a1 lambda_expr_this_capture(tree_node*, int)
/home/patrick/gcc-master/gcc/cp/lambda.c:811
0x9bb4a0 maybe_resolve_dummy(tree_node*, bool)
/home/patrick/gcc-master/gcc/cp/lambda.c:894
0x8de510 build_new_method_call_1
/home/patrick/gcc-master/gcc/cp/call.c:10503
0x8df04f build_new_method_call(tree_node*, tree_node*, vec<tree_node*, va_gc,
vl_embed>**, tree_node*, int, tree_node**, int)
Presumably because we no longer add 'this->' to the dependent call
MyType::make_crash<i> inside the generic lambda, so we don't capture it
when we should have?
To be clear, this is with
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -2214,8 +2214,8 @@ finish_qualified_id_expr (tree qualifying_class,
{
/* See if any of the functions are non-static members. */
/* If so, the expression may be relative to 'this'. */
- if ((type_dependent_expression_p (expr)
- || !shared_member_p (expr))
+ if (!type_dependent_expression_p (expr)
+ && !shared_member_p (expr)
&& current_class_ptr
&& DERIVED_FROM_P (qualifying_class,
current_nonlambda_class_type ()))
>
> > > > -- >8 --
> > > >
> > > > Subject: [PATCH] c++: 'this' injection and static member functions
> > > > [PR97399]
> > > >
> > > > gcc/cp/ChangeLog:
> > > >
> > > > PR c++/97399
> > > > * parser.c (cp_parser_init_declarator): If the storage class
> > > > specifier is sc_static, pass true for static_p to
> > > > cp_parser_declarator.
>
> I don't see this change in the patch; it seems like a good cleanup even if it
> isn't needed for this bug.
Sounds good. I'll add it back in (and the testcase cpp0x/this2.C) in the next
iteration of the patch.
>
> > > > (cp_parser_direct_declarator): Don't do inject_this_parm when
> > > > the function is specified as a friend.
> > > > * pt.c (tsubst_function_type): Also inject 'this' when
> > > > substituting into the function type of a static member
> > > > function.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > PR c++/88548
> > > > PR c++/97399
> > > > * g++.dg/cpp0x/this2.C: New test.
> > > > * g++.dg/gomp/this-1.C: Adjust expected error for use of 'this'
> > > > in signature of static member function.
> > > > * g++.dg/template/pr97399a.C: New test.
> > > > * g++.dg/template/pr97399b.C: New test.
> > > > ---
> > > > gcc/cp/parser.c | 2 +-
> > > > gcc/cp/pt.c | 13 +++++++++++--
> > > > gcc/testsuite/g++.dg/template/pr97399a.C | 23 +++++++++++++++++++++++
> > > > gcc/testsuite/g++.dg/template/pr97399b.C | 23 +++++++++++++++++++++++
> > > > 4 files changed, 58 insertions(+), 3 deletions(-)
> > > > create mode 100644 gcc/testsuite/g++.dg/template/pr97399a.C
> > > > create mode 100644 gcc/testsuite/g++.dg/template/pr97399b.C
> > > >
> > > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> > > > index 48437f23175..a0efa55d21e 100644
> > > > --- a/gcc/cp/parser.c
> > > > +++ b/gcc/cp/parser.c
> > > > @@ -22122,7 +22122,7 @@ cp_parser_direct_declarator (cp_parser* parser,
> > > > tree save_ccp = current_class_ptr;
> > > > tree save_ccr = current_class_ref;
> > > > - if (memfn)
> > > > + if (memfn && !friend_p)
> > > > /* DR 1207: 'this' is in scope after the cv-quals.
> > > > */
> > > > inject_this_parameter (current_class_type,
> > > > cv_quals);
> > > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > > > index ad855dbde72..d4763325e25 100644
> > > > --- a/gcc/cp/pt.c
> > > > +++ b/gcc/cp/pt.c
> > > > @@ -15072,8 +15072,17 @@ tsubst_function_type (tree t,
> > > > tree save_ccp = current_class_ptr;
> > > > tree save_ccr = current_class_ref;
> > > > - tree this_type = (TREE_CODE (t) == METHOD_TYPE
> > > > - ? TREE_TYPE (TREE_VALUE (arg_types)) :
> > > > NULL_TREE);
> > > > + tree this_type = NULL_TREE;
> > > > + if (TREE_CODE (t) == METHOD_TYPE)
> > > > + this_type = TREE_TYPE (TREE_VALUE (arg_types));
> > > > + else if (in_decl
> > > > + && DECL_FUNCTION_MEMBER_P (in_decl)
> > >
> > > Oops, this line should instead be checking DECL_STATIC_FUNCTION_P.
> > > Consider it changed.
> > >
> > > > + && t == TREE_TYPE (in_decl))
> > > > + /* Also inject 'this' when substituting into the function type
> > > > + of a static member function, as we may have conservatively
> > > > + added 'this->' to a dependent member function call in its
> > > > + trailing return type which we might need to resolve. */
> > > > + this_type = DECL_CONTEXT (in_decl);
> > > > bool do_inject = this_type && CLASS_TYPE_P (this_type);
> > > > if (do_inject)
> > > > {
> > > > diff --git a/gcc/testsuite/g++.dg/template/pr97399a.C
> > > > b/gcc/testsuite/g++.dg/template/pr97399a.C
> > > > new file mode 100644
> > > > index 00000000000..4bb818908fd
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/g++.dg/template/pr97399a.C
> > > > @@ -0,0 +1,23 @@
> > > > +// PR c++/97399
> > > > +// { dg-do compile { target c++11 } }
> > > > +
> > > > +template <bool> struct enable_if_t {};
> > > > +
> > > > +struct tmp {
> > > > + template <class> static constexpr bool is_integral();
> > > > + template <class T> static auto f()
> > > > + -> enable_if_t<tmp::is_integral<T>()>;
> > > > + template <class T> friend auto g(tmp, T)
> > > > + -> enable_if_t<!tmp::is_integral<T>()>;
> > > > +};
> > > > +
> > > > +template <class> constexpr bool tmp::is_integral() { return true; }
> > > > +
> > > > +template <class T> auto tmp::f()
> > > > + -> enable_if_t<tmp::is_integral<T>()> { return {}; }
> > > > +
> > > > +int main()
> > > > +{
> > > > + tmp::f<int>();
> > > > + g(tmp{}, 0);
> > > > +}
> > > > diff --git a/gcc/testsuite/g++.dg/template/pr97399b.C
> > > > b/gcc/testsuite/g++.dg/template/pr97399b.C
> > > > new file mode 100644
> > > > index 00000000000..673ba6624e3
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/g++.dg/template/pr97399b.C
> > > > @@ -0,0 +1,23 @@
> > > > +// PR c++/97399
> > > > +// { dg-do compile { target c++11 } }
> > > > +
> > > > +template <bool> struct enable_if_t {};
> > > > +
> > > > +struct tmp {
> > > > + template <class> constexpr bool is_integral(); // non-static
> > > > + template <class T> static auto f()
> > > > + -> enable_if_t<tmp::is_integral<T>()>; // { dg-message "in template
> > > > argument" }
> > > > + template <class T> friend auto g(tmp, T)
> > > > + -> enable_if_t<!tmp::is_integral<T>()>; // { dg-error "without
> > > > object" }
> > > > +};
> > > > +
> > > > +template <class> constexpr bool tmp::is_integral() { return true; }
> > > > +
> > > > +template <class T> auto tmp::f() // { dg-error "'this' is not a
> > > > constant" }
> > > > + -> enable_if_t<tmp::is_integral<T>()> { return {}; }
> > > > +
> > > > +int main()
> > > > +{
> > > > + tmp::f<int>(); // { dg-error "no match" }
> > > > + g(tmp{}, 0); // { dg-error "no match" }
> > > > +}
> > > > --
> > > > 2.30.0.155.g66e871b664
>
>
