This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[v2 of PATCH 03/14] C++: add location_t wrapper nodes during parsing (minimal impl)


On Fri, 2017-12-15 at 13:58 -0500, Jason Merrill wrote:
> On Fri, Dec 15, 2017 at 11:35 AM, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > On Fri, 2017-12-15 at 10:01 -0500, Jason Merrill wrote:
> > > On Thu, Dec 14, 2017 at 2:25 PM, David Malcolm <dmalcolm@redhat.c
> > > om>
> > > wrote:
> > > > On Mon, 2017-12-11 at 21:10 -0500, Jason Merrill wrote:
> > > > > On 11/10/2017 04:45 PM, David Malcolm wrote:
> > > > > > The initial version of the patch kit added location wrapper
> > > > > > nodes
> > > > > > around constants and uses-of-declarations, along with some
> > > > > > other
> > > > > > places in the parser (typeid, alignof, sizeof, offsetof).
> > > > > > 
> > > > > > This version takes a much more minimal approach: it only
> > > > > > adds
> > > > > > location wrapper nodes around the arguments at callsites,
> > > > > > thus
> > > > > > not adding wrapper nodes around uses of constants and decls
> > > > > > in
> > > > > > other
> > > > > > locations.
> > > > > > 
> > > > > > It keeps them for the other places in the parser (typeid,
> > > > > > alignof,
> > > > > > sizeof, offsetof).
> > > > > > 
> > > > > > In addition, for now, each site that adds wrapper nodes is
> > > > > > guarded
> > > > > > with !processing_template_decl, suppressing the creation of
> > > > > > wrapper
> > > > > > nodes when processing template declarations.  This is to
> > > > > > simplify
> > > > > > the patch kit so that we don't have to support wrapper
> > > > > > nodes
> > > > > > during
> > > > > > template expansion.
> > > > > 
> > > > > Hmm, it should be easy to support them, since NON_LVALUE_EXPR
> > > > > and
> > > > > VIEW_CONVERT_EXPR don't otherwise appear in template trees.
> > > > > 
> > > > > Jason
> > > > 
> > > > I don't know if it's "easy"; it's at least non-trivial.
> > > > 
> > > > I attempted to support them in the obvious way by adding the
> > > > two
> > > > codes
> > > > to the switch statement tsubst_copy, reusing the case used by
> > > > NOP_EXPR
> > > > and others, but ran into a issue when dealing with template
> > > > parameter
> > > > packs.
> > > > Attached is the reproducer I've been testing with (minimized
> > > > using
> > > > "delta" from a stdlib reproducer); my code was failing with:
> > > > 
> > > > ../../src/cp-stdlib.ii: In instantiation of ‘struct
> > > > allocator_traits<allocator<char> >’:
> > > > ../../src/cp-stdlib.ii:31:8:   required from ‘struct
> > > > __alloc_traits<allocator<char>, char>’
> > > > ../../src/cp-stdlib.ii:43:75:   required from ‘class
> > > > basic_string<char, allocator<char> >’
> > > > ../../src/cp-stdlib.ii:47:58:   required from here
> > > > ../../src/cp-stdlib.ii:27:55: sorry, unimplemented: use of
> > > > ‘type_pack_expansion’ in template
> > > >      -> decltype(_S_construct(__a, __p,
> > > > forward<_Args>(__args)...))  {   }
> > > >                                                        ^~~~~~
> > > > 
> > > > The issue is that normally "__args" would be a PARM_DECL of
> > > > type
> > > > TYPE_PACK_EXPANSION, and that's handled by tsubst_decl, but on
> > > > adding a
> > > > wrapper node we now have a VIEW_CONVERT_EXPR of the same type
> > > > i.e.
> > > > TYPE_PACK_EXPANSION wrapping the PARM_DECL.
> > > > 
> > > > When tsubst traverses the tree, the VIEW_CONVERT_EXPR is
> > > > reached
> > > > first,
> > > > and it attempts to substitute the type TYPE_PACK_EXPANSION,
> > > > which
> > > > leads
> > > > to the "sorry".
> > > > 
> > > > If I understand things right, during substitution, only
> > > > tsubst_decl
> > > > on
> > > > PARM_DECL can handle nodes with type with code
> > > > TYPE_PACK_EXPANSION.
> > > > 
> > > > The simplest approach seems to be to not create wrapper nodes
> > > > for
> > > > decls
> > > > of type TYPE_PACK_EXPANSION, and that seems to fix the issue.
> > > 
> > > That does seem simplest.
> > > 
> > > > Alternatively I can handle TYPE_PACK_EXPANSION for
> > > > VIEW_CONVERT_EXPR in
> > > > tsubst by remapping the type to that of what they wrap after
> > > > substitution; doing so also fixes the issue.
> > > 
> > > This will be more correct.  For the wrappers you don't need all
> > > the
> > > handling that we currently have for NOP_EXPR and such; since we
> > > know
> > > they don't change the type, we can substitute what they wrap, and
> > > then
> > > rewrap the result.
> > 
> > (nods; I have this working)
> > 
> > I've been debugging the other issues that I ran into when removing
> > the
> > "!processing_template_decl" filter on making wrapper nodes (ICEs
> > and
> > other errors on valid code).  They turn out to relate to wrappers
> > around decls of type TEMPLATE_TYPE_PARM; having these wrappers
> > leads to
> > such VIEW_CONVERT_EXPRs turning up in unexpected places.
> 
> Hmm, that's odd.  What kind of decls?  A variable which happens to
> have a template parameter for a type shouldn't be a problem.

Sorry.  This turned out to be a bug in my implementation of tsubst_*.

> > I could try to track all those places down, but it seems much
> > simpler
> > to just add an exclusion to adding wrapper nodes around decls of
> > type
> > TEMPLATE_TYPE_PARM.  On doing that my smoketests with the C++
> > stdlib
> > work again.  Does that sound reasonable?
> 
> Jason

Here's an updated version of the patch which fixes the issues above.

As before, it adds locations wrapper nodes around the arguments at
callsites, along with some other places in the C++ parser (typeid,
alignof, sizeof, offsetof).

It removes the guard on !processing_template_decl when creating
wrapper nodes that was present in the earlier version of the
patch.  Doing so required fixes for handling location wrapper nodes
to tsubst_copy, tsubst_copy_and_build, build_non_dependent_expr, and
to name-mangling.

One issue I ran into was that fold_for_warn doesn't eliminate
location wrappers when processing_template_decl, leading to
failures of the template-based cases in
g++.dg/warn/Wmemset-transposed-args-1.C.

This is due to the early bailout when processing_template_decl
within cp_fold:

2078	  if (processing_template_decl
2079	      || (EXPR_P (x) && (!TREE_TYPE (x) || TREE_TYPE (x) == error_mark_node)))
2080	    return x;

which dates back to the merger of the C++ delayed folding branch.

I've fixed that in this version of the patch by removing that
"processing_template_decl ||" condition from that cp_fold early
bailout.  It's not clear to me that that's the right fix - but if
we're going to add location wrapper nodes for such templated code,
presumably we need some way to look through the wrappers for these
warnings, and it seems like you favor fold_for_warn as the way to
do this.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

OK for trunk in conjunction with the rest of the kit?

BTW, apart from this patch, the only other part of the kit that still
needs review is:
 
  "[v3 of PATCH 13/14] c-format.c: handle location wrappers"
     https://gcc.gnu.org/ml/gcc-patches/2017-12/msg01494.html

Thanks
Dave

gcc/cp/ChangeLog:
	* cp-gimplify.c (cp_fold): Remove the early bailout when
	processing_template_decl.
	* cp-lang.c (selftest::run_cp_tests): Call
	selftest::cp_pt_c_tests.
	* cp-tree.h (selftest::cp_pt_c_tests): New decl.
	* mangle.c (write_expression): Skip location wrapper nodes.
	* parser.c (cp_parser_postfix_expression): Call
	maybe_add_location_wrapper on the result for RID_TYPEID. Pass true
	for new "wrap_locations_p" param of
	cp_parser_parenthesized_expression_list.
	(cp_parser_parenthesized_expression_list): Add "wrap_locations_p"
	param, defaulting to false.  Convert "expr" to a cp_expr, and call
	maybe_add_location_wrapper on it when wrap_locations_p is true.
	(cp_parser_unary_expression): Call maybe_add_location_wrapper on
	the result for RID_ALIGNOF and RID_SIZEOF.
	(cp_parser_builtin_offsetof): Likewise.
	* pt.c: Include "selftest.h".
	(tsubst_copy): Handle location wrappers.
	(tsubst_copy_and_build): Likewise.
	(build_non_dependent_expr): Likewise.
	(selftest::test_build_non_dependent_expr): New function.
	(selftest::cp_pt_c_tests): New function.
---
 gcc/cp/cp-gimplify.c |  5 ++--
 gcc/cp/cp-lang.c     |  1 +
 gcc/cp/cp-tree.h     | 10 ++++++++
 gcc/cp/mangle.c      |  1 +
 gcc/cp/parser.c      | 25 +++++++++++++++-----
 gcc/cp/pt.c          | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 97 insertions(+), 9 deletions(-)

diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 934f674..9bdaafc 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -2058,7 +2058,7 @@ clear_fold_cache (void)
 
 /*  This function tries to fold an expression X.
     To avoid combinatorial explosion, folding results are kept in fold_cache.
-    If we are processing a template or X is invalid, we don't fold at all.
+    If X is invalid, we don't fold at all.
     For performance reasons we don't cache expressions representing a
     declaration or constant.
     Function returns X or its folded variant.  */
@@ -2075,8 +2075,7 @@ cp_fold (tree x)
   if (!x || x == error_mark_node)
     return x;
 
-  if (processing_template_decl
-      || (EXPR_P (x) && (!TREE_TYPE (x) || TREE_TYPE (x) == error_mark_node)))
+  if (EXPR_P (x) && (!TREE_TYPE (x) || TREE_TYPE (x) == error_mark_node))
     return x;
 
   /* Don't bother to cache DECLs or constants.  */
diff --git a/gcc/cp/cp-lang.c b/gcc/cp/cp-lang.c
index 805319a..2c53740 100644
--- a/gcc/cp/cp-lang.c
+++ b/gcc/cp/cp-lang.c
@@ -247,6 +247,7 @@ run_cp_tests (void)
   c_family_tests ();
 
   /* Additional C++-specific tests.  */
+  cp_pt_c_tests ();
 }
 
 } // namespace selftest
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 9879e16..43a4eab 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7451,6 +7451,16 @@ named_decl_hash::equal (const value_type existing, compare_type candidate)
   return candidate == name;
 }
 
+#if CHECKING_P
+namespace selftest {
+  extern void run_cp_tests (void);
+
+  /* Declarations for specific families of tests within cp,
+     by source file, in alphabetical order.  */
+  extern void cp_pt_c_tests ();
+} // namespace selftest
+#endif /* #if CHECKING_P */
+
 /* -- end of C++ */
 
 #endif /* ! GCC_CP_TREE_H */
diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c
index 6d4e591..837796b 100644
--- a/gcc/cp/mangle.c
+++ b/gcc/cp/mangle.c
@@ -2834,6 +2834,7 @@ write_expression (tree expr)
   /* Skip NOP_EXPR and CONVERT_EXPR.  They can occur when (say) a pointer
      argument is converted (via qualification conversions) to another type.  */
   while (CONVERT_EXPR_CODE_P (code)
+	 || location_wrapper_p (expr)
 	 /* Parentheses aren't mangled.  */
 	 || code == PAREN_EXPR
 	 || code == NON_LVALUE_EXPR)
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 891f742..359572c 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -2047,7 +2047,8 @@ static tree cp_parser_postfix_open_square_expression
 static tree cp_parser_postfix_dot_deref_expression
   (cp_parser *, enum cpp_ttype, cp_expr, bool, cp_id_kind *, location_t);
 static vec<tree, va_gc> *cp_parser_parenthesized_expression_list
-  (cp_parser *, int, bool, bool, bool *, location_t * = NULL);
+  (cp_parser *, int, bool, bool, bool *, location_t * = NULL,
+   bool = false);
 /* Values for the second parameter of cp_parser_parenthesized_expression_list.  */
 enum { non_attr = 0, normal_attr = 1, id_attr = 2 };
 static void cp_parser_pseudo_destructor_name
@@ -6831,6 +6832,7 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 	    location_t typeid_loc
 	      = make_location (start_loc, start_loc, close_paren->location);
 	    postfix_expression.set_location (typeid_loc);
+	    postfix_expression.maybe_add_location_wrapper ();
 	  }
       }
       break;
@@ -7088,7 +7090,8 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 		    (parser, non_attr,
 		     /*cast_p=*/false, /*allow_expansion_p=*/true,
 		     /*non_constant_p=*/NULL,
-		     /*close_paren_loc=*/&close_paren_loc));
+		     /*close_paren_loc=*/&close_paren_loc,
+		     /*wrap_locations_p=*/true));
 	    if (is_builtin_constant_p)
 	      {
 		parser->integral_constant_expression_p
@@ -7621,6 +7624,10 @@ cp_parser_postfix_dot_deref_expression (cp_parser *parser,
    ALLOW_EXPANSION_P is true if this expression allows expansion of an
    argument pack.
 
+   WRAP_LOCATIONS_P is true if expressions within this list for which
+   CAN_HAVE_LOCATION_P is false should be wrapped with nodes expressing
+   their source locations.
+
    Returns a vector of trees.  Each element is a representation of an
    assignment-expression.  NULL is returned if the ( and or ) are
    missing.  An empty, but allocated, vector is returned on no
@@ -7640,7 +7647,8 @@ cp_parser_parenthesized_expression_list (cp_parser* parser,
 					 bool cast_p,
                                          bool allow_expansion_p,
 					 bool *non_constant_p,
-					 location_t *close_paren_loc)
+					 location_t *close_paren_loc,
+					 bool wrap_locations_p)
 {
   vec<tree, va_gc> *expression_list;
   bool fold_expr_p = is_attribute_list != non_attr;
@@ -7663,12 +7671,12 @@ cp_parser_parenthesized_expression_list (cp_parser* parser,
     = parser->greater_than_is_operator_p;
   parser->greater_than_is_operator_p = true;
 
+  cp_expr expr (NULL_TREE);
+
   /* Consume expressions until there are no more.  */
   if (cp_lexer_next_token_is_not (parser->lexer, CPP_CLOSE_PAREN))
     while (true)
       {
-	tree expr;
-
 	/* At the beginning of attribute lists, check to see if the
 	   next token is an identifier.  */
 	if (is_attribute_list == id_attr
@@ -7722,11 +7730,14 @@ cp_parser_parenthesized_expression_list (cp_parser* parser,
                 expr = make_pack_expansion (expr);
               }
 
+	    if (wrap_locations_p)
+	      expr.maybe_add_location_wrapper ();
+
 	     /* Add it to the list.  We add error_mark_node
 		expressions to the list, so that we can still tell if
 		the correct form for a parenthesized expression-list
 		is found. That gives better errors.  */
-	    vec_safe_push (expression_list, expr);
+	    vec_safe_push (expression_list, expr.get_value ());
 
 	    if (expr == error_mark_node)
 	      goto skip_comma;
@@ -7992,6 +8003,7 @@ cp_parser_unary_expression (cp_parser *parser, cp_id_kind * pidk,
 
 	    cp_expr ret_expr (ret);
 	    ret_expr.set_location (compound_loc);
+	    ret_expr = ret_expr.maybe_add_location_wrapper ();
 	    return ret_expr;
 	  }
 
@@ -9831,6 +9843,7 @@ cp_parser_builtin_offsetof (cp_parser *parser)
   parser->integral_constant_expression_p = save_ice_p;
   parser->non_integral_constant_expression_p = save_non_ice_p;
 
+  expr = expr.maybe_add_location_wrapper ();
   return expr;
 }
 
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 252712e..2266e1d 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -41,6 +41,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "type-utils.h"
 #include "gimplify.h"
 #include "gcc-rich-location.h"
+#include "selftest.h"
 
 /* The type of functions taking a tree, and some additional data, and
    returning an int.  */
@@ -14928,6 +14929,8 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
 	/* Ordinary template template argument.  */
 	return t;
 
+    case VIEW_CONVERT_EXPR:
+    case NON_LVALUE_EXPR:
     case CAST_EXPR:
     case REINTERPRET_CAST_EXPR:
     case CONST_CAST_EXPR:
@@ -14937,6 +14940,15 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
     case CONVERT_EXPR:
     case NOP_EXPR:
       {
+	if (location_wrapper_p (t))
+	  {
+	    /* Handle location wrappers by substituting the wrapped node
+	       first, *then* reusing the resulting type.  Doing the type
+	       first ensures that we handle template parameters and
+	       parameter pack expansions.  */
+	    tree op0 = tsubst_copy (TREE_OPERAND (t, 0), args, complain, in_decl);
+	    return build1 (code, TREE_TYPE (op0), op0);
+	  }
 	tree type = tsubst (TREE_TYPE (t), args, complain, in_decl);
 	tree op0 = tsubst_copy (TREE_OPERAND (t, 0), args, complain, in_decl);
 	return build1 (code, type, op0);
@@ -18282,6 +18294,14 @@ tsubst_copy_and_build (tree t,
     case REQUIRES_EXPR:
       RETURN (tsubst_requires_expr (t, args, complain, in_decl));
 
+    case VIEW_CONVERT_EXPR:
+    case NON_LVALUE_EXPR:
+      if (location_wrapper_p (t))
+	{
+	  RETURN (RECUR (TREE_OPERAND (t, 0)));
+	}
+      /* fallthrough.  */
+
     default:
       /* Handle Objective-C++ constructs, if appropriate.  */
       {
@@ -24998,6 +25018,8 @@ build_non_dependent_expr (tree expr)
       && !expanding_concept ())
     fold_non_dependent_expr (expr);
 
+  STRIP_ANY_LOCATION_WRAPPER (expr);
+
   /* Preserve OVERLOADs; the functions must be available to resolve
      types.  */
   inner_expr = expr;
@@ -26601,4 +26623,46 @@ print_template_statistics (void)
 	   type_specializations->collisions ());
 }
 
+#if CHECKING_P
+
+namespace selftest {
+
+/* Verify that build_non_dependent_expr () works, for various expressions,
+   and that location wrappers don't affect the results.  */
+
+static void
+test_build_non_dependent_expr ()
+{
+  location_t loc = BUILTINS_LOCATION;
+
+  /* Verify constants, without and with location wrappers.  */
+  tree int_cst = build_int_cst (integer_type_node, 42);
+  ASSERT_EQ (int_cst, build_non_dependent_expr (int_cst));
+
+  tree wrapped_int_cst = maybe_wrap_with_location (int_cst, loc);
+  ASSERT_TRUE (location_wrapper_p (wrapped_int_cst));
+  ASSERT_EQ (int_cst, build_non_dependent_expr (wrapped_int_cst));
+
+  tree string_lit = build_string (4, "foo");
+  TREE_TYPE (string_lit) = char_array_type_node;
+  string_lit = fix_string_type (string_lit);
+  ASSERT_EQ (string_lit, build_non_dependent_expr (string_lit));
+
+  tree wrapped_string_lit = maybe_wrap_with_location (string_lit, loc);
+  ASSERT_TRUE (location_wrapper_p (wrapped_string_lit));
+  ASSERT_EQ (string_lit, build_non_dependent_expr (wrapped_string_lit));
+}
+
+/* Run all of the selftests within this file.  */
+
+void
+cp_pt_c_tests ()
+{
+  test_build_non_dependent_expr ();
+}
+
+} // namespace selftest
+
+#endif /* #if CHECKING_P */
+
 #include "gt-cp-pt.h"
-- 
1.8.5.3


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]