diff mbox series

[v3] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions

Message ID CAARDWz_wSSDLGXNW3p8-cqL+Z=i1Oap_cqGty4KYxVQonhut-w@mail.gmail.com
State New
Headers show
Series [v3] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions | expand

Commit Message

Barrett Adair Sept. 14, 2021, 4:31 a.m. UTC 
I reworked the fix today based on feedback from Jason and Jakub (thank
you), and the subject line is now outdated. I added another test for a
closely related bug that's also fixed here (dependent-expr11.C -- this one
would even fail without the second declaration). All the new tests in the
patch succeed with the change (only two of them succeed with trunk). On my
box, the bootstrap succeeds, the g++ test suite passes (matching today's
posted results anyway), and the libstdc++ test suite is looking good but is
still running after a long time. I'll leave the full "make check" running
overnight.

Some potentially controversial changes here:

1. Adding new bool member to cp_parser. I'd like to avoid this, any tips?
2. Relaxing an assert in tsubst_copy. This change feels correct to me, but
maybe I'm missing something.
3. Pushing a function scope in PARM_DECL case in tsubst_copy_and_build to
make process_outer_var_ref happy for trailing return types. I don't yet
fully appreciate the consequences of these changes, so this needs some eyes.
4. Traversing each template arg's tree in
any_template_arguments_need_structural_equality_p to identify dependent
expressions in trailing return types. This could probably be done better. I
check current_function_decl here as an optimization (since it's NULL in the
only place that "needs" this), but that seems brittle. Also, the new
find_dependent_parm_decl_r callback implementation may have the unintended
consequence of forcing structural comparison on member function trailing
return types that depend on class template parameters. I think I really
only want to force structural comparison for "arg tree has a dependent parm
decl and we're in a trailing return type" -- is there a better way to do
this?

Also note that I found another related bug which I have not yet solved:

template<int i>
struct foo {
  constexpr operator int() { return i; }
};
void bar() {
  [](auto i) -> foo<i> {
    return {};
  }(foo<1>{});
}

With the attached patch, failure occurs at invocation, while trunk fails to
parse the return type. This seems like a step in the right direction, but
we should consider whether such an incomplete fix introduces more issues
than it solves (e.g. unfriendlier error messages, or perhaps something more
sinister).

Thanks,
Barrett

Comments

Jason Merrill Sept. 15, 2021, 7:49 p.m. UTC | #1 
On 9/14/21 00:31, Barrett Adair wrote:
> I reworked the fix today based on feedback from Jason and Jakub (thank 
> you), and the subject line is now outdated. I added another test for a 
> closely related bug that's also fixed here (dependent-expr11.C -- this 
> one would even fail without the second declaration). All the new tests 
> in the patch succeed with the change (only two of them succeed with 
> trunk). On my box, the bootstrap succeeds, the g++ test suite passes 
> (matching today's posted results anyway), and the libstdc++ test suite 
> is looking good but is still running after a long time. I'll leave the 
> full "make check" running overnight.
> 
> Some potentially controversial changes here:
> 
> 1. Adding new bool member to cp_parser. I'd like to avoid this, any tips?
> 2. Relaxing an assert in tsubst_copy. This change feels correct to me, 
> but maybe I'm missing something.
> 3. Pushing a function scope in PARM_DECL case in tsubst_copy_and_build 
> to make process_outer_var_ref happy for trailing return types. I don't 
> yet fully appreciate the consequences of these changes, so this needs 
> some eyes.

These all are to support dependent-expr11.C, right?  This seems like a 
separate issue, that should be a separate patch.

And I don't think there's anything special about a trailing return type. 
  I am surprised to discover that I don't see anything prohibiting that 
use, but I similarly don't see anything prohibiting

template<class T> auto bar(T t, bool_c<t()>) -> bool_c<t()>;

or even

template <class T> using FP = void (*)(T t, int (*)[t()]);

So I guess the "use of parameter outside function body" code in 
finish_id_expression_1 is obsolete with constexpr; removing that should 
address #1.

One way to approach #2 might be to

   begin_scope (sk_function_parms, NULL_TREE);

in tsubst_function_type, so that parsing_function_declarator (which I'm 
about to check in) is true, and change the assert to also check that.

Maybe that will also help with #3.  Really, outer_var_p should be false 
for t, so we shouldn't ever get to process_outer_var_ref.

-----

OK, now for the part of the patch that corresponds to the subject line:

> 4. Traversing each template arg's tree in 
> any_template_arguments_need_structural_equality_p to identify dependent 
> expressions in trailing return types. This could probably be done 
> better. I check current_function_decl here as an optimization (since 
> it's NULL in the only place that "needs" this), but that seems brittle. 

I think that optimization makes sense; within a function we shouldn't 
need structural comparison, only for comparing two template declarations.

> Also, the new find_dependent_parm_decl_r callback implementation may 
> have the unintended consequence of forcing structural comparison on 
> member function trailing return types that depend on class template 
> parameters. I think I really only want to force structural comparison 
> for "arg tree has a dependent parm decl and we're in a trailing return 
> type" -- is there a better way to do this?

I don't think whether the parm is dependent is important: the case we 
want to catch is if the argument as a whole is dependent, and contains a 
mention of a parameter.

> Also note that I found another related bug which I have not yet solved:
> 
> template<int i>
> struct foo {
>    constexpr operator int() { return i; }
> };
> void bar() {
>    [](auto i) -> foo<i> {
>      return {};
>    }(foo<1>{});
> }
> 
> With the attached patch, failure occurs at invocation, while trunk fails 
> to parse the return type. This seems like a step in the right direction, 
> but we should consider whether such an incomplete fix introduces more 
> issues than it solves (e.g. unfriendlier error messages, or perhaps 
> something more sinister).

This would also be related to the separate change under 1-3 above.

Jason
diff mbox series

Patch

From 0470bdc5b2b4ddff2d2ee9db11a8c5895abda50f Mon Sep 17 00:00:00 2001
From: Barrett Adair <barrettellisadair@gmail.com>
Date: Fri, 20 Aug 2021 15:37:36 -0500
Subject: [PATCH] Fix trailing return type bugs

---
 gcc/cp/cp-tree.h                              |  2 +-
 gcc/cp/parser.c                               | 13 ++++-
 gcc/cp/parser.h                               |  3 +
 gcc/cp/pt.c                                   | 58 +++++++++++++++++--
 gcc/cp/semantics.c                            |  9 ++-
 gcc/testsuite/g++.dg/template/canon-type-15.C |  7 +++
 gcc/testsuite/g++.dg/template/canon-type-16.C |  6 ++
 gcc/testsuite/g++.dg/template/canon-type-17.C |  5 ++
 gcc/testsuite/g++.dg/template/canon-type-18.C |  6 ++
 .../g++.dg/template/dependent-expr11.C        |  6 ++
 .../g++.dg/template/dependent-name15.C        | 18 ++++++
 .../g++.dg/template/dependent-name16.C        | 14 +++++
 12 files changed, 136 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/template/canon-type-15.C
 create mode 100644 gcc/testsuite/g++.dg/template/canon-type-16.C
 create mode 100644 gcc/testsuite/g++.dg/template/canon-type-17.C
 create mode 100644 gcc/testsuite/g++.dg/template/canon-type-18.C
 create mode 100644 gcc/testsuite/g++.dg/template/dependent-expr11.C
 create mode 100644 gcc/testsuite/g++.dg/template/dependent-name15.C
 create mode 100644 gcc/testsuite/g++.dg/template/dependent-name16.C

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index a82747ca627..b93455aebff 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7537,7 +7537,7 @@  extern tree process_outer_var_ref		(tree, tsubst_flags_t, bool force_use = false
 extern cp_expr finish_id_expression		(tree, tree, tree,
 						 cp_id_kind *,
 						 bool, bool, bool *,
-						 bool, bool, bool, bool,
+						 bool, bool, bool, bool, bool,
 						 const char **,
                                                  location_t);
 extern tree finish_typeof			(tree);
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index e44c5c6b57c..4b95103eb2b 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6011,7 +6011,8 @@  cp_parser_primary_expression (cp_parser *parser,
 		 parser->integral_constant_expression_p,
 		 parser->allow_non_integral_constant_expression_p,
 		 &parser->non_integral_constant_expression_p,
-		 template_p, done, address_p,
+		 template_p, parser->in_trailing_return_type_p,
+		 done, address_p,
 		 template_arg_p,
 		 &error_msg,
 		 id_expression.get_location ()));
@@ -11256,6 +11257,7 @@  cp_parser_lambda_introducer (cp_parser* parser, tree lambda_expr)
                  /*allow_non_integral_constant_expression_p=*/false,
                  /*non_integral_constant_expression_p=*/NULL,
                  /*template_p=*/false,
+                 /*trailing_return_type_p=*/false,
                  /*done=*/true,
                  /*address_p=*/false,
                  /*template_arg_p=*/false,
@@ -16192,6 +16194,7 @@  cp_parser_decltype_expr (cp_parser *parser,
                    /*allow_non_integral_constant_expression_p=*/true,
                    &non_integral_constant_expression_p,
                    /*template_p=*/false,
+                   parser->in_trailing_return_type_p,
                    /*done=*/true,
                    /*address_p=*/false,
                    /*template_arg_p=*/false,
@@ -24078,8 +24081,11 @@  cp_parser_template_type_arg (cp_parser *parser)
 static tree
 cp_parser_trailing_type_id (cp_parser *parser)
 {
-  return cp_parser_type_id_1 (parser, CP_PARSER_FLAGS_TYPENAME_OPTIONAL,
+  parser->in_trailing_return_type_p = true;
+  tree result = cp_parser_type_id_1 (parser, CP_PARSER_FLAGS_TYPENAME_OPTIONAL,
 			      false, true, NULL);
+  parser->in_trailing_return_type_p = false;
+  return result;
 }
 
 /* Parse a type-specifier-seq.
@@ -44635,7 +44641,8 @@  cp_finish_omp_declare_variant (cp_parser *parser, cp_token *pragma_tok,
 	= finish_id_expression (varid, variant, parser->scope,
 				&idk, false, true,
 				&parser->non_integral_constant_expression_p,
-				template_p, true, false, false, &error_msg,
+				template_p, parser->in_trailing_return_type_p,
+				true, false, false, &error_msg,
 				varid.get_location ());
       if (error_msg)
 	cp_parser_error (parser, error_msg);
diff --git a/gcc/cp/parser.h b/gcc/cp/parser.h
index 3669587cebd..a4f99e3f80c 100644
--- a/gcc/cp/parser.h
+++ b/gcc/cp/parser.h
@@ -419,6 +419,9 @@  struct GTY(()) cp_parser {
      member definition using a generic type, it is the sk_class scope.  */
   cp_binding_level* implicit_template_scope;
 
+  /* True if parsing a trailing return type.*/
+  bool in_trailing_return_type_p;
+
   /* True if parsing a result type in a compound requirement. This permits
      constrained-type-specifiers inside what would normally be a trailing
      return type. */
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 224dd9ebd2b..20942c62a39 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -16593,10 +16593,9 @@  tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
 	  if (DECL_NAME (t) == this_identifier && current_class_ptr)
 	    return current_class_ptr;
 
-	  /* This can happen for a parameter name used later in a function
-	     declaration (such as in a late-specified return type).  Just
-	     make a dummy decl, since it's only used for its type.  */
-	  gcc_assert (cp_unevaluated_operand != 0);
+	  gcc_assert (cp_unevaluated_operand != 0
+	      || TYPE_HAS_LATE_RETURN_TYPE (TREE_TYPE (DECL_CONTEXT (t))));
+
 	  r = tsubst_decl (t, args, complain);
 	  /* Give it the template pattern as its context; its true context
 	     hasn't been instantiated yet and this is good enough for
@@ -19643,6 +19642,7 @@  tsubst_copy_and_build (tree t,
           /*allow_non_integral_constant_expression_p=*/(cxx_dialect >= cxx11),
 				     &non_integral_constant_expression_p,
 				     /*template_p=*/false,
+				     /*in_trailing_return_type_p=*/false,
 				     /*done=*/true,
 				     /*address_p=*/false,
 				     /*template_arg_p=*/false,
@@ -20859,6 +20859,25 @@  tsubst_copy_and_build (tree t,
     case PARM_DECL:
       {
 	tree r = tsubst_copy (t, args, complain, in_decl);
+	tree context = r != error_mark_node
+	  && tree_contains_struct[TREE_CODE (r)][TS_DECL_MINIMAL]
+	  ? DECL_CONTEXT (r) : NULL_TREE;
+
+	/* if we're instantiating a template in a trailing return type,
+	   we need the function's parameters in scope */
+	bool needs_context_push = context && TREE_TYPE (context)
+	    && DECL_DECLARES_FUNCTION_P (context)
+	    && TYPE_HAS_LATE_RETURN_TYPE (TREE_TYPE (context));
+
+	if (needs_context_push)
+	  {
+	    push_access_scope (context);
+	    push_deferring_access_checks (dk_no_deferred);
+	    if (LAMBDA_FUNCTION_P (context))
+	      start_lambda_scope (r);
+	    ++function_depth;
+	  }
+
 	/* ??? We're doing a subset of finish_id_expression here.  */
 	if (tree wrap = maybe_get_tls_wrapper_call (r))
 	  /* Replace an evaluated use of the thread_local variable with
@@ -20871,6 +20890,16 @@  tsubst_copy_and_build (tree t,
 	  /* If the original type was a reference, we'll be wrapped in
 	     the appropriate INDIRECT_REF.  */
 	  r = convert_from_reference (r);
+
+	if (needs_context_push)
+	  {
+	    --function_depth;
+	    if (LAMBDA_FUNCTION_P (context))
+	      finish_lambda_scope ();
+	    pop_deferring_access_checks ();
+	    pop_access_scope (context);
+	  }
+
 	RETURN (r);
       }
 
@@ -27766,6 +27795,21 @@  dependent_template_arg_p (tree arg)
     return value_dependent_expression_p (arg);
 }
 
+/* Identify any expressions dependent on a function
+   template's PARM_DECL */
+static tree
+find_dependent_parm_decl_r (tree *tp, int *walk_subtrees, void*)
+{
+  tree t = *tp;
+  if (PACK_EXPANSION_P (t) || (TREE_CODE (t) == PARM_DECL
+      && TREE_CODE (TREE_TYPE (t)) == TEMPLATE_TYPE_PARM))
+    {
+      *walk_subtrees = 0;
+      return t;
+    }
+  return NULL_TREE;
+}
+
 /* Returns true if ARGS (a collection of template arguments) contains
    any types that require structural equality testing.  */
 
@@ -27810,6 +27854,12 @@  any_template_arguments_need_structural_equality_p (tree args)
 	      else if (!TYPE_P (arg) && TREE_TYPE (arg)
 		       && TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (arg)))
 		return true;
+	      /* if dependent instantiation inside trailing return type,
+	         require structural comparison, at least until canonical
+	         type is made to work for this case */
+	      else if (!current_function_decl &&
+		  cp_walk_tree_without_duplicates (&arg, find_dependent_parm_decl_r, NULL))
+		return true;
 	    }
 	}
     }
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 94e6b181d5d..d9cc52879b9 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -3932,6 +3932,7 @@  finish_id_expression_1 (tree id_expression,
 			bool allow_non_integral_constant_expression_p,
 			bool *non_integral_constant_expression_p,
 			bool template_p,
+			bool trailing_return_type_p,
 			bool done,
 			bool address_p,
 			bool template_arg_p,
@@ -4007,7 +4008,7 @@  finish_id_expression_1 (tree id_expression,
 	 body, except inside an unevaluated context (i.e. decltype).  */
       if (TREE_CODE (decl) == PARM_DECL
 	  && DECL_CONTEXT (decl) == NULL_TREE
-	  && !cp_unevaluated_operand)
+	  && !cp_unevaluated_operand && !trailing_return_type_p)
 	{
 	  *error_msg = G_("use of parameter outside function body");
 	  return error_mark_node;
@@ -4259,6 +4260,7 @@  finish_id_expression (tree id_expression,
 		      bool allow_non_integral_constant_expression_p,
 		      bool *non_integral_constant_expression_p,
 		      bool template_p,
+		      bool trailing_return_type_p,
 		      bool done,
 		      bool address_p,
 		      bool template_arg_p,
@@ -4270,7 +4272,8 @@  finish_id_expression (tree id_expression,
 			      integral_constant_expression_p,
 			      allow_non_integral_constant_expression_p,
 			      non_integral_constant_expression_p,
-			      template_p, done, address_p, template_arg_p,
+			      template_p, trailing_return_type_p,
+			      done, address_p, template_arg_p,
 			      error_msg, location);
   return result.maybe_add_location_wrapper ();
 }
@@ -5737,7 +5740,7 @@  omp_reduction_lookup (location_t loc, tree id, tree type, tree *baselinkp,
       if (decl == NULL_TREE)
 	decl = error_mark_node;
       id = finish_id_expression (id, decl, NULL_TREE, &idk, false, true,
-				 &nonint_cst_expression_p, false, true, false,
+				 &nonint_cst_expression_p, false, false, true, false,
 				 false, &error_msg, loc);
       if (idk == CP_ID_KIND_UNQUALIFIED
 	  && identifier_p (id))
diff --git a/gcc/testsuite/g++.dg/template/canon-type-15.C b/gcc/testsuite/g++.dg/template/canon-type-15.C
new file mode 100644
index 00000000000..b001b5c841d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-15.C
@@ -0,0 +1,7 @@ 
+// { dg-do compile { target c++11 } }
+template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
+namespace g {
+template<class T> auto return_size(T t) -> size_c<sizeof(t)>;
+template<class T> auto return_size(T t) -> size_c<sizeof(t)>;
+}
+static_assert(decltype(g::return_size('a'))::value == 1u, "");
diff --git a/gcc/testsuite/g++.dg/template/canon-type-16.C b/gcc/testsuite/g++.dg/template/canon-type-16.C
new file mode 100644
index 00000000000..99361cbac30
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-16.C
@@ -0,0 +1,6 @@ 
+// { dg-do compile { target c++11 } }
+template<bool u> struct bool_c{ static constexpr bool value = u; };
+template<class T> auto noexcepty(T t) -> bool_c<noexcept(t())>;
+template<class T> auto noexcepty(T t) -> bool_c<noexcept(t())>;
+struct foo { void operator()() noexcept; };
+static_assert(decltype(noexcepty(foo{}))::value, "");
diff --git a/gcc/testsuite/g++.dg/template/canon-type-17.C b/gcc/testsuite/g++.dg/template/canon-type-17.C
new file mode 100644
index 00000000000..0555c8d0a42
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-17.C
@@ -0,0 +1,5 @@ 
+// { dg-do compile { target c++11 } }
+template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
+template<class... T> auto return_size(T... t) -> size_c<sizeof...(t)>;
+template<class... T> auto return_size(T... t) -> size_c<sizeof...(t)>;
+static_assert(decltype(return_size('a'))::value == 1u, "");
diff --git a/gcc/testsuite/g++.dg/template/canon-type-18.C b/gcc/testsuite/g++.dg/template/canon-type-18.C
new file mode 100644
index 00000000000..2510181725c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-18.C
@@ -0,0 +1,6 @@ 
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wno-pedantic" }
+template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
+template<class T> auto get_align(T t) -> size_c<alignof(t)>;
+template<class T> auto get_align(T t) -> size_c<alignof(t)>;
+static_assert(decltype(get_align('a'))::value == 1u, "");
diff --git a/gcc/testsuite/g++.dg/template/dependent-expr11.C b/gcc/testsuite/g++.dg/template/dependent-expr11.C
new file mode 100644
index 00000000000..d1fa63fc792
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/dependent-expr11.C
@@ -0,0 +1,6 @@ 
+// { dg-do compile { target c++11 } }
+template<bool u> struct bool_c{ static constexpr bool value = u; };
+struct foo { constexpr bool operator()() const { return true; } };
+template<class T> auto bar(T t) -> bool_c<t()>;
+template<class T> auto bar(T t) -> bool_c<t()>;
+static_assert(decltype(bar(foo{}))::value, "");
diff --git a/gcc/testsuite/g++.dg/template/dependent-name15.C b/gcc/testsuite/g++.dg/template/dependent-name15.C
new file mode 100644
index 00000000000..1c34bc704f9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/dependent-name15.C
@@ -0,0 +1,18 @@ 
+// { dg-do compile { target c++11 } }
+template <int N> struct A { static void foo(){} };
+template <> struct A<sizeof(char)> { using foo = int; };
+
+template <class T> void f(T t1) { 
+    A<sizeof(t1)>::foo();
+}
+
+template <class T> void g(T t2) { 
+    /* if the comparing_specializations check breaks in cp_tree_equal
+    case PARM_DECL, the error will incorrectly report A<sizeof (t1)> */
+    A<sizeof(t2)>::foo(); // { dg-error "dependent-name .A<sizeof .t2.>::foo" }
+}
+
+void h() {
+    f(0);
+    g('0');
+}
diff --git a/gcc/testsuite/g++.dg/template/dependent-name16.C b/gcc/testsuite/g++.dg/template/dependent-name16.C
new file mode 100644
index 00000000000..ef8c4f23077
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/dependent-name16.C
@@ -0,0 +1,14 @@ 
+// { dg-do compile { target c++11 } }
+template <int N> struct A { static void foo(){} };
+template <> struct A<sizeof(char)> { using foo = int; };
+
+template<class T1> auto f(T1 t1) -> decltype(A<sizeof(t1)>::foo());
+
+/* if the comparing_specializations check breaks in cp_tree_equal
+case PARM_DECL, the error will incorrectly report A<sizeof (t1)> */
+template<class T2> auto g(T2 t2) -> decltype(A<sizeof(t2)>::foo()); // { dg-error "dependent-name .A<sizeof .t2.>::foo" }
+
+void h() {
+    f(0);
+    g('0'); // { dg-error "no matching function" }
+}
-- 
2.30.2