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

2021-09-13 Thread Barrett Adair via Gcc-patches
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
struct foo {
  constexpr operator int() { return i; }
};
void bar() {
  [](auto i) -> foo {
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
From 0470bdc5b2b4ddff2d2ee9db11a8c5895abda50f Mon Sep 17 00:00:00 2001
From: Barrett Adair 
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,
  /*templa

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

2021-09-15 Thread Jason Merrill via Gcc-patches

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 auto bar(T t, bool_c) -> bool_c;

or even

template  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
struct foo {
   constexpr operator int() { return i; }
};
void bar() {
   [](auto i) -> foo {
     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