diff mbox series

[2/1] c++: more non-static memfn call dependence cleanup [PR106086]

Message ID 20230926154013.2413300-1-ppalka@redhat.com
State New
Headers show
Series c++: non-static memfn call dependence cleanup | expand

Commit Message

Patrick Palka Sept. 26, 2023, 3:40 p.m. UTC
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK
for trunk?

-- >8 --

This follow-up patch removes some more repetition of the type-dependent
case of finish_call_expr, this time in of tsubst_copy_and_build.  This
allows us to easily fix PR106086 -- which is about us failing to capture
'this' when we resolve a use of a non-static member function of the
current instantiation only at lambda regeneration time and neglect to
capture 'this' -- by moving the call to maybe_generic_this_capture from
the parser to finish_call_expr so that we attempt to capture 'this' at
regeneration time as well.

	PR c++/106086

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_postfix_expression): Don't call
	maybe_generic_this_capture here.
	* pt.cc (tsubst_copy_and_build) <case CALL_EXPR>: Remove
	COMPONENT_REF callee handling.
	* semantics.cc (finish_call_expr): In the type-dependent case,
	call maybe_generic_this_capture here instead.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp1y/lambda-generic-this5.C: New test.
---
 gcc/cp/parser.cc                              |  8 ------
 gcc/cp/pt.cc                                  | 25 -------------------
 gcc/cp/semantics.cc                           | 12 ++++++---
 .../g++.dg/cpp1y/lambda-generic-this5.C       | 22 ++++++++++++++++
 4 files changed, 30 insertions(+), 37 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C

Comments

Patrick Palka Oct. 12, 2023, 6:49 p.m. UTC | #1
On Tue, 26 Sep 2023, Patrick Palka wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK
> for trunk?
> 
> -- >8 --
> 
> This follow-up patch removes some more repetition of the type-dependent

On second thought there's no good reason to split these patches into a two
part series, so here's a single squashed patch:

-- >8 --

Subject: [PATCH] c++: non-static memfn call dependence cleanup [PR106086]

In cp_parser_postfix_expression and in the CALL_EXPR case of
tsubst_copy_and_build, we essentially repeat the type-dependent and
COMPONENT_REF callee cases of finish_call_expr.  This patch deduplicates
this logic by making both spots consistently go through finish_call_expr.

This allows us to easily fix PR106086 -- which is about us neglecting to
capture 'this' when we resolve a use of a non-static member function of
the current instantiation only at lambda regeneration time -- by moving
the call to maybe_generic_this_capture from the parser to finish_call_expr
so that we consider capturing 'this' at regeneration time as well.

	PR c++/106086

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_postfix_expression): Consolidate three
	calls to finish_call_expr, one to build_new_method_call and
	one to build_min_nt_call_vec into one call to finish_call_expr.
	Don't call maybe_generic_this_capture here.
	* pt.cc (tsubst_copy_and_build) <case CALL_EXPR>: Remove
	COMPONENT_REF callee handling.
	(type_dependent_expression_p): Use t_d_object_e_p instead of
	t_d_e_p for COMPONENT_REF and OFFSET_REF.
	* semantics.cc (finish_call_expr): In the type-dependent case,
	call maybe_generic_this_capture here instead.

gcc/testsuite/ChangeLog:

	* g++.dg/template/crash127.C: Expect additional error due to
	being able to check the member access expression ahead of time.
	Strengthen the test by not instantiating the class template.
	* g++.dg/cpp1y/lambda-generic-this5.C: New test.
---
 gcc/cp/parser.cc                              | 52 +++----------------
 gcc/cp/pt.cc                                  | 27 +---------
 gcc/cp/semantics.cc                           | 12 +++--
 .../g++.dg/cpp1y/lambda-generic-this5.C       | 22 ++++++++
 gcc/testsuite/g++.dg/template/crash127.C      |  3 +-
 5 files changed, 38 insertions(+), 78 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C

diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index f3abae716fe..b00ef36b831 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -8047,54 +8047,12 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 					    close_paren_loc);
 	    iloc_sentinel ils (combined_loc);
 
-	    if (TREE_CODE (postfix_expression) == COMPONENT_REF)
-	      {
-		tree instance = TREE_OPERAND (postfix_expression, 0);
-		tree fn = TREE_OPERAND (postfix_expression, 1);
-
-		if (processing_template_decl
-		    && (type_dependent_object_expression_p (instance)
-			|| (!BASELINK_P (fn)
-			    && TREE_CODE (fn) != FIELD_DECL)
-			|| type_dependent_expression_p (fn)
-			|| any_type_dependent_arguments_p (args)))
-		  {
-		    maybe_generic_this_capture (instance, fn);
-		    postfix_expression
-		      = build_min_nt_call_vec (postfix_expression, args);
-		  }
-		else if (BASELINK_P (fn))
-		  {
-		  postfix_expression
-		    = (build_new_method_call
-		       (instance, fn, &args, NULL_TREE,
-			(idk == CP_ID_KIND_QUALIFIED
-			 ? LOOKUP_NORMAL|LOOKUP_NONVIRTUAL
-			 : LOOKUP_NORMAL),
-			/*fn_p=*/NULL,
-			complain));
-		  }
-		else
-		  postfix_expression
-		    = finish_call_expr (postfix_expression, &args,
-					/*disallow_virtual=*/false,
-					/*koenig_p=*/false,
-					complain);
-	      }
-	    else if (TREE_CODE (postfix_expression) == OFFSET_REF
-		     || TREE_CODE (postfix_expression) == MEMBER_REF
-		     || TREE_CODE (postfix_expression) == DOTSTAR_EXPR)
+	    if (TREE_CODE (postfix_expression) == OFFSET_REF
+		|| TREE_CODE (postfix_expression) == MEMBER_REF
+		|| TREE_CODE (postfix_expression) == DOTSTAR_EXPR)
 	      postfix_expression = (build_offset_ref_call_from_tree
 				    (postfix_expression, &args,
 				     complain));
-	    else if (idk == CP_ID_KIND_QUALIFIED)
-	      /* A call to a static class member, or a namespace-scope
-		 function.  */
-	      postfix_expression
-		= finish_call_expr (postfix_expression, &args,
-				    /*disallow_virtual=*/true,
-				    koenig_p,
-				    complain);
 	    else
 	      /* All other function calls.  */
 	      {
@@ -8107,12 +8065,14 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 				   "not permitted in intervening code");
 		    parser->omp_for_parse_state->fail = true;
 		  }
+		bool disallow_virtual = (idk == CP_ID_KIND_QUALIFIED);
 		postfix_expression
 		  = finish_call_expr (postfix_expression, &args,
-				      /*disallow_virtual=*/false,
+				      disallow_virtual,
 				      koenig_p,
 				      complain);
 	      }
+
 	    if (close_paren_loc != UNKNOWN_LOCATION)
 	      postfix_expression.set_location (combined_loc);
 
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 382db4dd01d..4400d429b6f 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -21364,31 +21364,6 @@ tsubst_copy_and_build (tree t,
 		 || TREE_CODE (function) == MEMBER_REF)
 	  ret = build_offset_ref_call_from_tree (function, &call_args,
 						 complain);
-	else if (TREE_CODE (function) == COMPONENT_REF)
-	  {
-	    tree instance = TREE_OPERAND (function, 0);
-	    tree fn = TREE_OPERAND (function, 1);
-
-	    if (processing_template_decl
-		&& (type_dependent_expression_p (instance)
-		    || (!BASELINK_P (fn)
-			&& TREE_CODE (fn) != FIELD_DECL)
-		    || type_dependent_expression_p (fn)
-		    || any_type_dependent_arguments_p (call_args)))
-	      ret = build_min_nt_call_vec (function, call_args);
-	    else if (!BASELINK_P (fn))
-	      ret = finish_call_expr (function, &call_args,
-				       /*disallow_virtual=*/false,
-				       /*koenig_p=*/false,
-				       complain);
-	    else
-	      ret = (build_new_method_call
-		      (instance, fn,
-		       &call_args, NULL_TREE,
-		       qualified_p ? LOOKUP_NONVIRTUAL : LOOKUP_NORMAL,
-		       /*fn_p=*/NULL,
-		       complain));
-	  }
 	else if (concept_check_p (function))
 	  {
 	    /* FUNCTION is a template-id referring to a concept definition.  */
@@ -28585,7 +28560,7 @@ type_dependent_expression_p (tree expression)
       if (TREE_CODE (expression) == COMPONENT_REF
 	  || TREE_CODE (expression) == OFFSET_REF)
 	{
-	  if (type_dependent_expression_p (TREE_OPERAND (expression, 0)))
+	  if (type_dependent_object_expression_p (TREE_OPERAND (expression, 0)))
 	    return true;
 	  expression = TREE_OPERAND (expression, 1);
 	  if (identifier_p (expression))
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 1d478f0781f..412eaa12851 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -2793,18 +2793,19 @@ finish_call_expr (tree fn, vec<tree, va_gc> **args, bool disallow_virtual,
 	     (c++/89780, c++/107363).  This also suppresses the
 	     -Wredundant-move warning.  */
 	  suppress_warning (result, OPT_Wpessimizing_move);
-	  if (is_overloaded_fn (fn))
-	    fn = get_fns (fn);
 
 	  if (cfun)
 	    {
 	      bool abnormal = true;
-	      for (lkp_iterator iter (fn); abnormal && iter; ++iter)
+	      for (lkp_iterator iter (maybe_get_fns (fn)); iter; ++iter)
 		{
 		  tree fndecl = STRIP_TEMPLATE (*iter);
 		  if (TREE_CODE (fndecl) != FUNCTION_DECL
 		      || !TREE_THIS_VOLATILE (fndecl))
-		    abnormal = false;
+		    {
+		      abnormal = false;
+		      break;
+		    }
 		}
 	      /* FIXME: Stop warning about falling off end of non-void
 		 function.   But this is wrong.  Even if we only see
@@ -2814,6 +2815,9 @@ finish_call_expr (tree fn, vec<tree, va_gc> **args, bool disallow_virtual,
 	      if (abnormal)
 		current_function_returns_abnormally = 1;
 	    }
+	  if (TREE_CODE (fn) == COMPONENT_REF)
+	    maybe_generic_this_capture (TREE_OPERAND (fn, 0),
+					TREE_OPERAND (fn, 1));
 	  return result;
 	}
       orig_args = make_tree_vector_copy (*args);
diff --git a/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C b/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
new file mode 100644
index 00000000000..42f917094d9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
@@ -0,0 +1,22 @@
+// PR c++/106086
+// { dg-do compile { target c++14 } }
+
+template<class T>
+struct A {
+  void f(int) const;
+  static void g(int);
+};
+
+template<class T>
+struct B : A<T> {
+  auto f() const {
+    auto l1 = [&](auto x) { A<T>::f(x); };
+    auto l2 = [&](auto x) { A<T>::g(x); };
+    static_assert(sizeof(l1) == sizeof(this), "");
+    static_assert(sizeof(l2) == 1, "");
+    l1(0);
+    l2(0);
+  }
+};
+
+template struct B<void>;
diff --git a/gcc/testsuite/g++.dg/template/crash127.C b/gcc/testsuite/g++.dg/template/crash127.C
index b7c03251f8c..fcf72d871db 100644
--- a/gcc/testsuite/g++.dg/template/crash127.C
+++ b/gcc/testsuite/g++.dg/template/crash127.C
@@ -16,7 +16,6 @@ struct C : public A
   {
     B < &A::A > b;  // { dg-error "taking address of constructor 'A::A" "" { target c++98_only } }
     // { dg-error "taking address of constructor 'constexpr A::A" "" { target c++11 } .-1 }
+    // { dg-error "template argument 1 is invalid" "" { target *-*-* } .-2 }
   }
 };
-
-template class C < int >;
Jason Merrill Oct. 19, 2023, 2:24 p.m. UTC | #2
On 10/12/23 14:49, Patrick Palka wrote:
> On Tue, 26 Sep 2023, Patrick Palka wrote:
> 
>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK
>> for trunk?
>>
>> -- >8 --
>>
>> This follow-up patch removes some more repetition of the type-dependent
> 
> On second thought there's no good reason to split these patches into a two
> part series, so here's a single squashed patch:

OK.

> -- >8 --
> 
> Subject: [PATCH] c++: non-static memfn call dependence cleanup [PR106086]
> 
> In cp_parser_postfix_expression and in the CALL_EXPR case of
> tsubst_copy_and_build, we essentially repeat the type-dependent and
> COMPONENT_REF callee cases of finish_call_expr.  This patch deduplicates
> this logic by making both spots consistently go through finish_call_expr.
> 
> This allows us to easily fix PR106086 -- which is about us neglecting to
> capture 'this' when we resolve a use of a non-static member function of
> the current instantiation only at lambda regeneration time -- by moving
> the call to maybe_generic_this_capture from the parser to finish_call_expr
> so that we consider capturing 'this' at regeneration time as well.
> 
> 	PR c++/106086
> 
> gcc/cp/ChangeLog:
> 
> 	* parser.cc (cp_parser_postfix_expression): Consolidate three
> 	calls to finish_call_expr, one to build_new_method_call and
> 	one to build_min_nt_call_vec into one call to finish_call_expr.
> 	Don't call maybe_generic_this_capture here.
> 	* pt.cc (tsubst_copy_and_build) <case CALL_EXPR>: Remove
> 	COMPONENT_REF callee handling.
> 	(type_dependent_expression_p): Use t_d_object_e_p instead of
> 	t_d_e_p for COMPONENT_REF and OFFSET_REF.
> 	* semantics.cc (finish_call_expr): In the type-dependent case,
> 	call maybe_generic_this_capture here instead.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/template/crash127.C: Expect additional error due to
> 	being able to check the member access expression ahead of time.
> 	Strengthen the test by not instantiating the class template.
> 	* g++.dg/cpp1y/lambda-generic-this5.C: New test.
> ---
>   gcc/cp/parser.cc                              | 52 +++----------------
>   gcc/cp/pt.cc                                  | 27 +---------
>   gcc/cp/semantics.cc                           | 12 +++--
>   .../g++.dg/cpp1y/lambda-generic-this5.C       | 22 ++++++++
>   gcc/testsuite/g++.dg/template/crash127.C      |  3 +-
>   5 files changed, 38 insertions(+), 78 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
> 
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index f3abae716fe..b00ef36b831 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -8047,54 +8047,12 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
>   					    close_paren_loc);
>   	    iloc_sentinel ils (combined_loc);
>   
> -	    if (TREE_CODE (postfix_expression) == COMPONENT_REF)
> -	      {
> -		tree instance = TREE_OPERAND (postfix_expression, 0);
> -		tree fn = TREE_OPERAND (postfix_expression, 1);
> -
> -		if (processing_template_decl
> -		    && (type_dependent_object_expression_p (instance)
> -			|| (!BASELINK_P (fn)
> -			    && TREE_CODE (fn) != FIELD_DECL)
> -			|| type_dependent_expression_p (fn)
> -			|| any_type_dependent_arguments_p (args)))
> -		  {
> -		    maybe_generic_this_capture (instance, fn);
> -		    postfix_expression
> -		      = build_min_nt_call_vec (postfix_expression, args);
> -		  }
> -		else if (BASELINK_P (fn))
> -		  {
> -		  postfix_expression
> -		    = (build_new_method_call
> -		       (instance, fn, &args, NULL_TREE,
> -			(idk == CP_ID_KIND_QUALIFIED
> -			 ? LOOKUP_NORMAL|LOOKUP_NONVIRTUAL
> -			 : LOOKUP_NORMAL),
> -			/*fn_p=*/NULL,
> -			complain));
> -		  }
> -		else
> -		  postfix_expression
> -		    = finish_call_expr (postfix_expression, &args,
> -					/*disallow_virtual=*/false,
> -					/*koenig_p=*/false,
> -					complain);
> -	      }
> -	    else if (TREE_CODE (postfix_expression) == OFFSET_REF
> -		     || TREE_CODE (postfix_expression) == MEMBER_REF
> -		     || TREE_CODE (postfix_expression) == DOTSTAR_EXPR)
> +	    if (TREE_CODE (postfix_expression) == OFFSET_REF
> +		|| TREE_CODE (postfix_expression) == MEMBER_REF
> +		|| TREE_CODE (postfix_expression) == DOTSTAR_EXPR)
>   	      postfix_expression = (build_offset_ref_call_from_tree
>   				    (postfix_expression, &args,
>   				     complain));
> -	    else if (idk == CP_ID_KIND_QUALIFIED)
> -	      /* A call to a static class member, or a namespace-scope
> -		 function.  */
> -	      postfix_expression
> -		= finish_call_expr (postfix_expression, &args,
> -				    /*disallow_virtual=*/true,
> -				    koenig_p,
> -				    complain);
>   	    else
>   	      /* All other function calls.  */
>   	      {
> @@ -8107,12 +8065,14 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
>   				   "not permitted in intervening code");
>   		    parser->omp_for_parse_state->fail = true;
>   		  }
> +		bool disallow_virtual = (idk == CP_ID_KIND_QUALIFIED);
>   		postfix_expression
>   		  = finish_call_expr (postfix_expression, &args,
> -				      /*disallow_virtual=*/false,
> +				      disallow_virtual,
>   				      koenig_p,
>   				      complain);
>   	      }
> +
>   	    if (close_paren_loc != UNKNOWN_LOCATION)
>   	      postfix_expression.set_location (combined_loc);
>   
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 382db4dd01d..4400d429b6f 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -21364,31 +21364,6 @@ tsubst_copy_and_build (tree t,
>   		 || TREE_CODE (function) == MEMBER_REF)
>   	  ret = build_offset_ref_call_from_tree (function, &call_args,
>   						 complain);
> -	else if (TREE_CODE (function) == COMPONENT_REF)
> -	  {
> -	    tree instance = TREE_OPERAND (function, 0);
> -	    tree fn = TREE_OPERAND (function, 1);
> -
> -	    if (processing_template_decl
> -		&& (type_dependent_expression_p (instance)
> -		    || (!BASELINK_P (fn)
> -			&& TREE_CODE (fn) != FIELD_DECL)
> -		    || type_dependent_expression_p (fn)
> -		    || any_type_dependent_arguments_p (call_args)))
> -	      ret = build_min_nt_call_vec (function, call_args);
> -	    else if (!BASELINK_P (fn))
> -	      ret = finish_call_expr (function, &call_args,
> -				       /*disallow_virtual=*/false,
> -				       /*koenig_p=*/false,
> -				       complain);
> -	    else
> -	      ret = (build_new_method_call
> -		      (instance, fn,
> -		       &call_args, NULL_TREE,
> -		       qualified_p ? LOOKUP_NONVIRTUAL : LOOKUP_NORMAL,
> -		       /*fn_p=*/NULL,
> -		       complain));
> -	  }
>   	else if (concept_check_p (function))
>   	  {
>   	    /* FUNCTION is a template-id referring to a concept definition.  */
> @@ -28585,7 +28560,7 @@ type_dependent_expression_p (tree expression)
>         if (TREE_CODE (expression) == COMPONENT_REF
>   	  || TREE_CODE (expression) == OFFSET_REF)
>   	{
> -	  if (type_dependent_expression_p (TREE_OPERAND (expression, 0)))
> +	  if (type_dependent_object_expression_p (TREE_OPERAND (expression, 0)))
>   	    return true;
>   	  expression = TREE_OPERAND (expression, 1);
>   	  if (identifier_p (expression))
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 1d478f0781f..412eaa12851 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -2793,18 +2793,19 @@ finish_call_expr (tree fn, vec<tree, va_gc> **args, bool disallow_virtual,
>   	     (c++/89780, c++/107363).  This also suppresses the
>   	     -Wredundant-move warning.  */
>   	  suppress_warning (result, OPT_Wpessimizing_move);
> -	  if (is_overloaded_fn (fn))
> -	    fn = get_fns (fn);
>   
>   	  if (cfun)
>   	    {
>   	      bool abnormal = true;
> -	      for (lkp_iterator iter (fn); abnormal && iter; ++iter)
> +	      for (lkp_iterator iter (maybe_get_fns (fn)); iter; ++iter)
>   		{
>   		  tree fndecl = STRIP_TEMPLATE (*iter);
>   		  if (TREE_CODE (fndecl) != FUNCTION_DECL
>   		      || !TREE_THIS_VOLATILE (fndecl))
> -		    abnormal = false;
> +		    {
> +		      abnormal = false;
> +		      break;
> +		    }
>   		}
>   	      /* FIXME: Stop warning about falling off end of non-void
>   		 function.   But this is wrong.  Even if we only see
> @@ -2814,6 +2815,9 @@ finish_call_expr (tree fn, vec<tree, va_gc> **args, bool disallow_virtual,
>   	      if (abnormal)
>   		current_function_returns_abnormally = 1;
>   	    }
> +	  if (TREE_CODE (fn) == COMPONENT_REF)
> +	    maybe_generic_this_capture (TREE_OPERAND (fn, 0),
> +					TREE_OPERAND (fn, 1));
>   	  return result;
>   	}
>         orig_args = make_tree_vector_copy (*args);
> diff --git a/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C b/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
> new file mode 100644
> index 00000000000..42f917094d9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
> @@ -0,0 +1,22 @@
> +// PR c++/106086
> +// { dg-do compile { target c++14 } }
> +
> +template<class T>
> +struct A {
> +  void f(int) const;
> +  static void g(int);
> +};
> +
> +template<class T>
> +struct B : A<T> {
> +  auto f() const {
> +    auto l1 = [&](auto x) { A<T>::f(x); };
> +    auto l2 = [&](auto x) { A<T>::g(x); };
> +    static_assert(sizeof(l1) == sizeof(this), "");
> +    static_assert(sizeof(l2) == 1, "");
> +    l1(0);
> +    l2(0);
> +  }
> +};
> +
> +template struct B<void>;
> diff --git a/gcc/testsuite/g++.dg/template/crash127.C b/gcc/testsuite/g++.dg/template/crash127.C
> index b7c03251f8c..fcf72d871db 100644
> --- a/gcc/testsuite/g++.dg/template/crash127.C
> +++ b/gcc/testsuite/g++.dg/template/crash127.C
> @@ -16,7 +16,6 @@ struct C : public A
>     {
>       B < &A::A > b;  // { dg-error "taking address of constructor 'A::A" "" { target c++98_only } }
>       // { dg-error "taking address of constructor 'constexpr A::A" "" { target c++11 } .-1 }
> +    // { dg-error "template argument 1 is invalid" "" { target *-*-* } .-2 }
>     }
>   };
> -
> -template class C < int >;
Patrick Palka Oct. 19, 2023, 3:07 p.m. UTC | #3
On Thu, 19 Oct 2023, Jason Merrill wrote:

> On 10/12/23 14:49, Patrick Palka wrote:
> > On Tue, 26 Sep 2023, Patrick Palka wrote:
> > 
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK
> > > for trunk?
> > > 
> > > -- >8 --
> > > 
> > > This follow-up patch removes some more repetition of the type-dependent
> > 
> > On second thought there's no good reason to split these patches into a two
> > part series, so here's a single squashed patch:
> 
> OK.

Thanks.  It turns out this patch slightly depends on the
NON_DEPENDENT_EXPR removal patches, since without them finish_call_expr
in a template context will undesirably do build_non_dependent_expr on
the fn/args before its COMPONENT_REF branch that dispatches to
build_new_method_call, but this latter function expects to be called
with unwrapped fn/args.  This (seemingly latent bug) can trivially be
fixed by moving finish_call_expr's build_non_dependent_expr calls to
happen after the COMPONENT_REF branch, but I reckon I'll just wait until
the NON_DEPENDENT_EXPR removal patches are in before pushing this one.

> 
> > -- >8 --
> > 
> > Subject: [PATCH] c++: non-static memfn call dependence cleanup [PR106086]
> > 
> > In cp_parser_postfix_expression and in the CALL_EXPR case of
> > tsubst_copy_and_build, we essentially repeat the type-dependent and
> > COMPONENT_REF callee cases of finish_call_expr.  This patch deduplicates
> > this logic by making both spots consistently go through finish_call_expr.
> > 
> > This allows us to easily fix PR106086 -- which is about us neglecting to
> > capture 'this' when we resolve a use of a non-static member function of
> > the current instantiation only at lambda regeneration time -- by moving
> > the call to maybe_generic_this_capture from the parser to finish_call_expr
> > so that we consider capturing 'this' at regeneration time as well.
> > 
> > 	PR c++/106086
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* parser.cc (cp_parser_postfix_expression): Consolidate three
> > 	calls to finish_call_expr, one to build_new_method_call and
> > 	one to build_min_nt_call_vec into one call to finish_call_expr.
> > 	Don't call maybe_generic_this_capture here.
> > 	* pt.cc (tsubst_copy_and_build) <case CALL_EXPR>: Remove
> > 	COMPONENT_REF callee handling.
> > 	(type_dependent_expression_p): Use t_d_object_e_p instead of
> > 	t_d_e_p for COMPONENT_REF and OFFSET_REF.
> > 	* semantics.cc (finish_call_expr): In the type-dependent case,
> > 	call maybe_generic_this_capture here instead.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/template/crash127.C: Expect additional error due to
> > 	being able to check the member access expression ahead of time.
> > 	Strengthen the test by not instantiating the class template.
> > 	* g++.dg/cpp1y/lambda-generic-this5.C: New test.
> > ---
> >   gcc/cp/parser.cc                              | 52 +++----------------
> >   gcc/cp/pt.cc                                  | 27 +---------
> >   gcc/cp/semantics.cc                           | 12 +++--
> >   .../g++.dg/cpp1y/lambda-generic-this5.C       | 22 ++++++++
> >   gcc/testsuite/g++.dg/template/crash127.C      |  3 +-
> >   5 files changed, 38 insertions(+), 78 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
> > 
> > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> > index f3abae716fe..b00ef36b831 100644
> > --- a/gcc/cp/parser.cc
> > +++ b/gcc/cp/parser.cc
> > @@ -8047,54 +8047,12 @@ cp_parser_postfix_expression (cp_parser *parser,
> > bool address_p, bool cast_p,
> >   					    close_paren_loc);
> >   	    iloc_sentinel ils (combined_loc);
> >   -	    if (TREE_CODE (postfix_expression) == COMPONENT_REF)
> > -	      {
> > -		tree instance = TREE_OPERAND (postfix_expression, 0);
> > -		tree fn = TREE_OPERAND (postfix_expression, 1);
> > -
> > -		if (processing_template_decl
> > -		    && (type_dependent_object_expression_p (instance)
> > -			|| (!BASELINK_P (fn)
> > -			    && TREE_CODE (fn) != FIELD_DECL)
> > -			|| type_dependent_expression_p (fn)
> > -			|| any_type_dependent_arguments_p (args)))
> > -		  {
> > -		    maybe_generic_this_capture (instance, fn);
> > -		    postfix_expression
> > -		      = build_min_nt_call_vec (postfix_expression, args);
> > -		  }
> > -		else if (BASELINK_P (fn))
> > -		  {
> > -		  postfix_expression
> > -		    = (build_new_method_call
> > -		       (instance, fn, &args, NULL_TREE,
> > -			(idk == CP_ID_KIND_QUALIFIED
> > -			 ? LOOKUP_NORMAL|LOOKUP_NONVIRTUAL
> > -			 : LOOKUP_NORMAL),
> > -			/*fn_p=*/NULL,
> > -			complain));
> > -		  }
> > -		else
> > -		  postfix_expression
> > -		    = finish_call_expr (postfix_expression, &args,
> > -					/*disallow_virtual=*/false,
> > -					/*koenig_p=*/false,
> > -					complain);
> > -	      }
> > -	    else if (TREE_CODE (postfix_expression) == OFFSET_REF
> > -		     || TREE_CODE (postfix_expression) == MEMBER_REF
> > -		     || TREE_CODE (postfix_expression) == DOTSTAR_EXPR)
> > +	    if (TREE_CODE (postfix_expression) == OFFSET_REF
> > +		|| TREE_CODE (postfix_expression) == MEMBER_REF
> > +		|| TREE_CODE (postfix_expression) == DOTSTAR_EXPR)
> >   	      postfix_expression = (build_offset_ref_call_from_tree
> >   				    (postfix_expression, &args,
> >   				     complain));
> > -	    else if (idk == CP_ID_KIND_QUALIFIED)
> > -	      /* A call to a static class member, or a namespace-scope
> > -		 function.  */
> > -	      postfix_expression
> > -		= finish_call_expr (postfix_expression, &args,
> > -				    /*disallow_virtual=*/true,
> > -				    koenig_p,
> > -				    complain);
> >   	    else
> >   	      /* All other function calls.  */
> >   	      {
> > @@ -8107,12 +8065,14 @@ cp_parser_postfix_expression (cp_parser *parser,
> > bool address_p, bool cast_p,
> >   				   "not permitted in intervening code");
> >   		    parser->omp_for_parse_state->fail = true;
> >   		  }
> > +		bool disallow_virtual = (idk == CP_ID_KIND_QUALIFIED);
> >   		postfix_expression
> >   		  = finish_call_expr (postfix_expression, &args,
> > -				      /*disallow_virtual=*/false,
> > +				      disallow_virtual,
> >   				      koenig_p,
> >   				      complain);
> >   	      }
> > +
> >   	    if (close_paren_loc != UNKNOWN_LOCATION)
> >   	      postfix_expression.set_location (combined_loc);
> >   diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index 382db4dd01d..4400d429b6f 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -21364,31 +21364,6 @@ tsubst_copy_and_build (tree t,
> >   		 || TREE_CODE (function) == MEMBER_REF)
> >   	  ret = build_offset_ref_call_from_tree (function, &call_args,
> >   						 complain);
> > -	else if (TREE_CODE (function) == COMPONENT_REF)
> > -	  {
> > -	    tree instance = TREE_OPERAND (function, 0);
> > -	    tree fn = TREE_OPERAND (function, 1);
> > -
> > -	    if (processing_template_decl
> > -		&& (type_dependent_expression_p (instance)
> > -		    || (!BASELINK_P (fn)
> > -			&& TREE_CODE (fn) != FIELD_DECL)
> > -		    || type_dependent_expression_p (fn)
> > -		    || any_type_dependent_arguments_p (call_args)))
> > -	      ret = build_min_nt_call_vec (function, call_args);
> > -	    else if (!BASELINK_P (fn))
> > -	      ret = finish_call_expr (function, &call_args,
> > -				       /*disallow_virtual=*/false,
> > -				       /*koenig_p=*/false,
> > -				       complain);
> > -	    else
> > -	      ret = (build_new_method_call
> > -		      (instance, fn,
> > -		       &call_args, NULL_TREE,
> > -		       qualified_p ? LOOKUP_NONVIRTUAL : LOOKUP_NORMAL,
> > -		       /*fn_p=*/NULL,
> > -		       complain));
> > -	  }
> >   	else if (concept_check_p (function))
> >   	  {
> >   	    /* FUNCTION is a template-id referring to a concept definition.
> > */
> > @@ -28585,7 +28560,7 @@ type_dependent_expression_p (tree expression)
> >         if (TREE_CODE (expression) == COMPONENT_REF
> >   	  || TREE_CODE (expression) == OFFSET_REF)
> >   	{
> > -	  if (type_dependent_expression_p (TREE_OPERAND (expression, 0)))
> > +	  if (type_dependent_object_expression_p (TREE_OPERAND (expression,
> > 0)))
> >   	    return true;
> >   	  expression = TREE_OPERAND (expression, 1);
> >   	  if (identifier_p (expression))
> > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> > index 1d478f0781f..412eaa12851 100644
> > --- a/gcc/cp/semantics.cc
> > +++ b/gcc/cp/semantics.cc
> > @@ -2793,18 +2793,19 @@ finish_call_expr (tree fn, vec<tree, va_gc> **args,
> > bool disallow_virtual,
> >   	     (c++/89780, c++/107363).  This also suppresses the
> >   	     -Wredundant-move warning.  */
> >   	  suppress_warning (result, OPT_Wpessimizing_move);
> > -	  if (is_overloaded_fn (fn))
> > -	    fn = get_fns (fn);
> >     	  if (cfun)
> >   	    {
> >   	      bool abnormal = true;
> > -	      for (lkp_iterator iter (fn); abnormal && iter; ++iter)
> > +	      for (lkp_iterator iter (maybe_get_fns (fn)); iter; ++iter)
> >   		{
> >   		  tree fndecl = STRIP_TEMPLATE (*iter);
> >   		  if (TREE_CODE (fndecl) != FUNCTION_DECL
> >   		      || !TREE_THIS_VOLATILE (fndecl))
> > -		    abnormal = false;
> > +		    {
> > +		      abnormal = false;
> > +		      break;
> > +		    }
> >   		}
> >   	      /* FIXME: Stop warning about falling off end of non-void
> >   		 function.   But this is wrong.  Even if we only see
> > @@ -2814,6 +2815,9 @@ finish_call_expr (tree fn, vec<tree, va_gc> **args,
> > bool disallow_virtual,
> >   	      if (abnormal)
> >   		current_function_returns_abnormally = 1;
> >   	    }
> > +	  if (TREE_CODE (fn) == COMPONENT_REF)
> > +	    maybe_generic_this_capture (TREE_OPERAND (fn, 0),
> > +					TREE_OPERAND (fn, 1));
> >   	  return result;
> >   	}
> >         orig_args = make_tree_vector_copy (*args);
> > diff --git a/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
> > b/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
> > new file mode 100644
> > index 00000000000..42f917094d9
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
> > @@ -0,0 +1,22 @@
> > +// PR c++/106086
> > +// { dg-do compile { target c++14 } }
> > +
> > +template<class T>
> > +struct A {
> > +  void f(int) const;
> > +  static void g(int);
> > +};
> > +
> > +template<class T>
> > +struct B : A<T> {
> > +  auto f() const {
> > +    auto l1 = [&](auto x) { A<T>::f(x); };
> > +    auto l2 = [&](auto x) { A<T>::g(x); };
> > +    static_assert(sizeof(l1) == sizeof(this), "");
> > +    static_assert(sizeof(l2) == 1, "");
> > +    l1(0);
> > +    l2(0);
> > +  }
> > +};
> > +
> > +template struct B<void>;
> > diff --git a/gcc/testsuite/g++.dg/template/crash127.C
> > b/gcc/testsuite/g++.dg/template/crash127.C
> > index b7c03251f8c..fcf72d871db 100644
> > --- a/gcc/testsuite/g++.dg/template/crash127.C
> > +++ b/gcc/testsuite/g++.dg/template/crash127.C
> > @@ -16,7 +16,6 @@ struct C : public A
> >     {
> >       B < &A::A > b;  // { dg-error "taking address of constructor 'A::A" ""
> > { target c++98_only } }
> >       // { dg-error "taking address of constructor 'constexpr A::A" "" {
> > target c++11 } .-1 }
> > +    // { dg-error "template argument 1 is invalid" "" { target *-*-* } .-2
> > }
> >     }
> >   };
> > -
> > -template class C < int >;
> 
>
diff mbox series

Patch

diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 78082ee7284..b00ef36b831 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -8071,14 +8071,6 @@  cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 				      disallow_virtual,
 				      koenig_p,
 				      complain);
-
-		if (type_dependent_expression_p (postfix_expression))
-		  {
-		    tree fn = CALL_EXPR_FN (postfix_expression);
-		    if (TREE_CODE (fn) == COMPONENT_REF)
-		      maybe_generic_this_capture (TREE_OPERAND (fn, 0),
-						  TREE_OPERAND (fn, 1));
-		  }
 	      }
 
 	    if (close_paren_loc != UNKNOWN_LOCATION)
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index b19b634690a..4400d429b6f 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -21364,31 +21364,6 @@  tsubst_copy_and_build (tree t,
 		 || TREE_CODE (function) == MEMBER_REF)
 	  ret = build_offset_ref_call_from_tree (function, &call_args,
 						 complain);
-	else if (TREE_CODE (function) == COMPONENT_REF)
-	  {
-	    tree instance = TREE_OPERAND (function, 0);
-	    tree fn = TREE_OPERAND (function, 1);
-
-	    if (processing_template_decl
-		&& (type_dependent_expression_p (instance)
-		    || (!BASELINK_P (fn)
-			&& TREE_CODE (fn) != FIELD_DECL)
-		    || type_dependent_expression_p (fn)
-		    || any_type_dependent_arguments_p (call_args)))
-	      ret = build_min_nt_call_vec (function, call_args);
-	    else if (!BASELINK_P (fn))
-	      ret = finish_call_expr (function, &call_args,
-				       /*disallow_virtual=*/false,
-				       /*koenig_p=*/false,
-				       complain);
-	    else
-	      ret = (build_new_method_call
-		      (instance, fn,
-		       &call_args, NULL_TREE,
-		       qualified_p ? LOOKUP_NONVIRTUAL : LOOKUP_NORMAL,
-		       /*fn_p=*/NULL,
-		       complain));
-	  }
 	else if (concept_check_p (function))
 	  {
 	    /* FUNCTION is a template-id referring to a concept definition.  */
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 1d478f0781f..412eaa12851 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -2793,18 +2793,19 @@  finish_call_expr (tree fn, vec<tree, va_gc> **args, bool disallow_virtual,
 	     (c++/89780, c++/107363).  This also suppresses the
 	     -Wredundant-move warning.  */
 	  suppress_warning (result, OPT_Wpessimizing_move);
-	  if (is_overloaded_fn (fn))
-	    fn = get_fns (fn);
 
 	  if (cfun)
 	    {
 	      bool abnormal = true;
-	      for (lkp_iterator iter (fn); abnormal && iter; ++iter)
+	      for (lkp_iterator iter (maybe_get_fns (fn)); iter; ++iter)
 		{
 		  tree fndecl = STRIP_TEMPLATE (*iter);
 		  if (TREE_CODE (fndecl) != FUNCTION_DECL
 		      || !TREE_THIS_VOLATILE (fndecl))
-		    abnormal = false;
+		    {
+		      abnormal = false;
+		      break;
+		    }
 		}
 	      /* FIXME: Stop warning about falling off end of non-void
 		 function.   But this is wrong.  Even if we only see
@@ -2814,6 +2815,9 @@  finish_call_expr (tree fn, vec<tree, va_gc> **args, bool disallow_virtual,
 	      if (abnormal)
 		current_function_returns_abnormally = 1;
 	    }
+	  if (TREE_CODE (fn) == COMPONENT_REF)
+	    maybe_generic_this_capture (TREE_OPERAND (fn, 0),
+					TREE_OPERAND (fn, 1));
 	  return result;
 	}
       orig_args = make_tree_vector_copy (*args);
diff --git a/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C b/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
new file mode 100644
index 00000000000..42f917094d9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/lambda-generic-this5.C
@@ -0,0 +1,22 @@ 
+// PR c++/106086
+// { dg-do compile { target c++14 } }
+
+template<class T>
+struct A {
+  void f(int) const;
+  static void g(int);
+};
+
+template<class T>
+struct B : A<T> {
+  auto f() const {
+    auto l1 = [&](auto x) { A<T>::f(x); };
+    auto l2 = [&](auto x) { A<T>::g(x); };
+    static_assert(sizeof(l1) == sizeof(this), "");
+    static_assert(sizeof(l2) == 1, "");
+    l1(0);
+    l2(0);
+  }
+};
+
+template struct B<void>;