This is the mail archive of the 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]

Re: [PING] Re: [PATCH 1/2] C++: more location wrapper nodes (PR c++/43064, PR c++/43486)

On 12/3/18 5:10 PM, Jeff Law wrote:
On 11/19/18 9:51 AM, David Malcolm wrote:
Ping, for these patches:

[PATCH 1/2] C++: more location wrapper nodes (PR c++/43064, PR c++/43486)

[PATCH 2/2] C++: improvements to binary operator diagnostics (PR c++/87504)


[1] I've split them up for ease of review; they could be reworked to
fully independent, but there's some churn in the results for
-Wtautological-compare introduced by the 1st patch which the 2nd
patch addresses.

	PR c++/43064
	PR c++/43486
	* convert.c: Include "selftest.h".
	(preserve_any_location_wrapper): New function.
	(convert_to_pointer_maybe_fold): Update to handle location
	(convert_to_real_maybe_fold): Likewise.
	(convert_to_integer_1): Handle location wrappers when checking
	(convert_to_integer_maybe_fold): Update to handle location
	(convert_to_complex_maybe_fold): Likewise.
	(selftest::test_convert_to_integer_maybe_fold): New functions.
	(selftest::convert_c_tests): New function.
	* fold-const.c (operand_equal_p): Strip any location wrappers.
	* selftest-run-tests.c (selftest::run_tests): Call
	* selftest.h (selftest::convert_c_tests): New decl.
	* tree.c (tree_int_cst_equal): Strip any location wrappers.
	(maybe_wrap_with_location): Don't create wrappers if any
	auto_suppress_location_wrappers are active.
	(suppress_location_wrappers): New variable.
	* tree.h (CONSTANT_CLASS_OR_WRAPPER_P): New macro.
	(suppress_location_wrappers): New decl.
	(class auto_suppress_location_wrappers): New class.

	PR c++/43064
	PR c++/43486
	* c-common.c (unsafe_conversion_p): Strip any location wrapper.
	(verify_tree): Handle location wrappers.
	(c_common_truthvalue_conversion): Strip any location wrapper.
	(fold_offsetof): Strip any location wrapper.
	(complete_array_type): Likewise for initial_value.
	(convert_vector_to_array_for_subscript): Call fold_for_warn on
	index before checking for INTEGER_CST.
	* c-pretty-print.c (c_pretty_printer::primary_expression):
	print parentheses around location wrappers.
	* c-warn.c (warn_logical_operator): Call fold_for_warn on
	before checking for INTEGER_CST.
	(warn_tautological_bitwise_comparison): Call
	tree_strip_any_location_wrapper on lhs, rhs, and bitop's
	before checking for INTEGER_CST.
	(readonly_error): Strip any location wrapper.
	(warn_array_subscript_with_type_char): Strip location wrappers
	before checking for INTEGER_CST.  Use the location of the index

	PR c++/43064
	PR c++/43486
	* call.c (build_conditional_expr_1): Strip location wrappers
	checking for CONST_DECL.
	(conversion_null_warnings): Use location of "expr" if
	* class.c (fixed_type_or_null): Handle location wrappers.
	* constexpr.c (potential_constant_expression_1): Likewise.
	* cvt.c (ignore_overflows): Strip location wrappers when
	checking for INTEGER_CST, and re-wrap the result if present.
	(ocp_convert): Call fold_for_warn before checking for
	* decl.c (reshape_init_r): Strip any location wrapper.
	(undeduced_auto_decl): Likewise.
	* decl2.c (grokbitfield): Likewise for width.
	* expr.c (mark_discarded_use): Likewise for expr.
	* init.c (build_aggr_init): Likewise before checking init for
	(warn_placement_new_too_small): Call fold_for_warn on adj
	checking for CONSTANT_CLASS_P, and on nelts.  Strip any
	wrapper from op0 and on oper before checking for VAR_P.
	* lambda.c (add_capture): Strip any location from initializer.
	* name-lookup.c (handle_namespace_attrs): Strip any location
	x before checking for STRING_CST.
	* parser.c (cp_parser_primary_expression): Call
	maybe_add_location_wrapper on numeric and string literals.
	(cp_parser_postfix_expression): Strip any location wrapper when
	(cp_parser_binary_expression): Strip any location wrapper when
	checking for DECL_P on the lhs.
	(cp_parser_decltype_expr): Suppress location wrappers in the
	(cp_parser_mem_initializer): Add location wrappers to the
	parenthesized expression list.
	(cp_parser_template_parameter_list): Don't create wrapper nodes
	within a template-parameter-list.
	(cp_parser_template_argument_list): Don't create wrapper nodes
	within a template-argument-list.
	(cp_parser_parameter_declaration): Strip location wrappers from
	default arguments.
	(cp_parser_gnu_attribute_list): Don't create wrapper nodes
	an attribute.
	(cp_parser_late_parsing_default_args): Strip location wrappers
	from default arguments.
	(cp_parser_omp_all_clauses): Don't create wrapper nodes within
	OpenMP clauses.
	(cp_parser_omp_for_loop): Likewise.
	(cp_parser_omp_declare_reduction_exprs): Likewise.
	* pt.c (convert_nontype_argument_function): Strip location
	wrappers from fn_no_ptr before checking for FUNCTION_DECL.
	(do_auto_deduction): Likewise from init before checking for
	* semantics.c (force_paren_expr): Likewise from expr before
	checking for DECL_P.
	(finish_parenthesized_expr): Likewise from expr before
	checking for STRING_CST.
	(perform_koenig_lookup): Likewise from fn.
	(finish_call_expr): Likewise.
	(finish_id_expression): Rename to...
	(finish_id_expression_1): ...this, calling
	maybe_add_location_wrapper on the result.
	* tree.c (cp_stabilize_reference): Strip any location wrapper.
	(builtin_valid_in_constant_expr_p): Likewise.
	(is_overloaded_fn): Likewise.
	(maybe_get_fns): Likewise.
	(selftest::test_lvalue_kind): Verify lvalue_p.
	* typeck.c (cxx_sizeof_expr): Strip any location wrapper.
	(cxx_alignof_expr): Likewise.
	(is_bitfield_expr_with_lowered_type): Handle location wrappers.
	(cp_build_array_ref): Strip location wrappers from idx before
	checking for INTEGER_CST.
	(cp_build_binary_op): Strip location wrapper from first_arg
	checking for PARM_DECL.  Likewise for op1 before checking for
	INTEGER_CST in two places.  Likewise for orig_op0 and orig_op1
	when checking for STRING_CST.
	(cp_build_addr_expr_1): Likewise for arg when checking for
	(cp_build_modify_expr): Likewise for newrhs when checking for
	(convert_for_assignment): Don't strip location wrappers when
	stripping NON_LVALUE_EXPR.
	(maybe_warn_about_returning_address_of_local): Strip location
	wrapper from whats_returned before checking for DECL_P.
	(can_do_nrvo_p): Strip location wrapper from retval.
	(treat_lvalue_as_rvalue_p): Likewise.
	(check_return_expr): Likewise.
	* typeck2.c (cxx_incomplete_type_diagnostic): Strip location
	wrapper from value before checking for VAR_P or PARM_DECL.
	(digest_init_r): Strip location wrapper from init.  When
	copying "init", also copy the wrapped node.

	PR c++/43064
	PR c++/43486
	* objc-act.c (objc_maybe_build_component_ref): Strip any
	wrapper before checking for UOBJC_SUPER_decl and self_decl.
	(objc_finish_message_expr): Strip any location wrapper.

	PR c++/43064
	PR c++/43486
	* c-c++-common/pr51712.c (valid2): Mark xfail as passing on
	* g++.dg/cpp1z/decomp48.C: Update expected location of warning
	for named local variables to use that of the local variable.
	* g++.dg/init/array43.C: Update expected column to be that of
	* g++.dg/init/initializer-string-too-long.C: New test.
	* g++.dg/init/pr43064-1.C: New test.
	* g++.dg/init/pr43064-2.C: New test.
	* g++.dg/init/pr43064-3.C: New test.
	* g++.dg/wrappers/Wparentheses.C: New test.
Well, you probably know my biggest worry with this -- the unwrappign
wack-a-mole problem.  It looks like additional memory usage is
measurable, but tiny.

Have you received any feedback from Jason on the C++ side?  That's the
bulk of this patch AFAICT.

I don't see anything objectionable in the c-family/ or generic bits.
But again I worry more about the need to sprinkle unwrapping all over
the place.

If we're adding wrappers, any place that wants to look at particular tree codes will need to deal with unwrapping. The question then is whether to just remove location wrappers or do fold_for_warn, which will remove location wrappers and also try to provide a constant value. In general, the former is better for things that deal with the forms of the code, and the latter for things that deal with values.

@@ -1236,6 +1236,8 @@ unsafe_conversion_p (location_t loc, tree type, tree expr, tree result,
loc = expansion_point_location_if_in_system_header (loc); + STRIP_ANY_LOCATION_WRAPPER (expr);
    if (TREE_CODE (expr) == REAL_CST || TREE_CODE (expr) == INTEGER_CST)

Why do the former here, when this function is interested in constants?

 warn_array_subscript_with_type_char (location_t loc, tree index)
-  if (TYPE_MAIN_VARIANT (TREE_TYPE (index)) == char_type_node
-      && TREE_CODE (index) != INTEGER_CST)
-    warning_at (loc, OPT_Wchar_subscripts,
-		"array subscript has type %<char%>");
+  if (TYPE_MAIN_VARIANT (TREE_TYPE (index)) == char_type_node)
+    {
+      if (TREE_CODE (index) != INTEGER_CST)
+	/* If INDEX has a location, use it; otherwise use LOC (the location
+	   of the subscripting expression as a whole).  */
+	warning_at (EXPR_LOC_OR_LOC (index, loc), OPT_Wchar_subscripts,
+		    "array subscript has type %<char%>");

Here you strip a location wrapper and then check whether the expression has a location, which will give suboptimal results if the expression is a variable.

@@ -7375,6 +7375,12 @@ fixed_type_or_null (tree instance, int *nonnull, int *cdtorp)
       return NULL_TREE;
+      if (location_wrapper_p (instance))
+	return RECUR (TREE_OPERAND (instance, 0));
+      else
+	return NULL_TREE;

I think recursion is probably right for some non-location-wrapper uses of VIEW_CONVERT_EXPR, as well, but for now let's just add a comment to that effect.

+	    if (TREE_CODE (from) == INTEGER_CST
+		&& !integer_zerop (from))
+	      {
+		if (flags & tf_error)
+		  error_at (loc, "reinterpret_cast from integer to pointer");

This might be another place where folding is the right answer.

 ignore_overflows (tree expr, tree orig)
-  if (TREE_CODE (expr) == INTEGER_CST
-      && TREE_CODE (orig) == INTEGER_CST
-      && TREE_OVERFLOW (expr) != TREE_OVERFLOW (orig))
+  tree stripped_expr = tree_strip_any_location_wrapper (expr);
+  tree stripped_orig = tree_strip_any_location_wrapper (orig);
+  if (TREE_CODE (stripped_expr) == INTEGER_CST
+      && TREE_CODE (stripped_orig) == INTEGER_CST
+      && TREE_OVERFLOW (stripped_expr) != TREE_OVERFLOW (stripped_orig))
-      gcc_assert (!TREE_OVERFLOW (orig));
+      gcc_assert (!TREE_OVERFLOW (stripped_orig));
       /* Ensure constant sharing.  */
-      expr = wide_int_to_tree (TREE_TYPE (expr), wi::to_wide (expr));
+      stripped_expr = wide_int_to_tree (TREE_TYPE (stripped_expr),
+					wi::to_wide (stripped_expr));
-  return expr;
+  if (location_wrapper_p (expr))
+    return maybe_wrap_with_location (stripped_expr, EXPR_LOCATION (expr));

Maybe use preserve_any_location_wrapper here?

@@ -1058,6 +1058,9 @@ grokbitfield (const cp_declarator *declarator,
       return NULL_TREE;
+ if (width)

Why is this needed? We should already be reducing width to an unwrapped constant value somewhere along the line.

@@ -656,6 +656,9 @@ add_capture (tree lambda, tree id, tree orig_init, bool by_reference_p,
       listmem = make_pack_expansion (member);
       initializer = orig_init;

Why is this needed? What cares about the tree codes of the capture initializer?

@@ -14122,6 +14126,7 @@ cp_parser_decltype_expr (cp_parser *parser,
+	  auto_suppress_location_wrappers sentinel;

Why do we want to suppress them in decltype?

@@ -21832,6 +21847,9 @@ cp_parser_parameter_declaration (cp_parser *parser,
+  if (default_argument)
+    STRIP_ANY_LOCATION_WRAPPER (default_argument);
+      /* Since default args are effectively part of the function type,
+	 strip location wrappers here, since otherwise the location of
+	 one function's default arguments is arbitrarily chosen for
+	 all functions with similar signature (due to canonicalization
+	 of function types).  */
+      STRIP_ANY_LOCATION_WRAPPER (parsed_arg);

Hmm, I imagine this is already an issue with default arguments that are location-carrying expressions.

It would be nice to avoid this unification, which I suppose would mean needing to distinguish better between identical and equivalent in various places.

But for now I guess this is fine.

+  /* Don't create location wrapper nodes within a template-argument-list.  */
+  auto_suppress_location_wrappers sentinel;

You probably also want to remove location wrappers in strip_typedefs_expr.

@@ -6257,6 +6257,7 @@ convert_nontype_argument_function (tree type, tree expr,

Is this necessary with the above suppression?

+	      /* Don't create wrapper nodes within an attribute: the
+		 handlers don't know how to handle them.  */
+	      auto_suppress_location_wrappers sentinel;

Do we still need to strip them in handle_namespace_attrs, then?

@@ -1682,6 +1682,8 @@ cxx_sizeof_expr (tree e, tsubst_flags_t complain)
@@ -1754,6 +1756,8 @@ cxx_alignof_expr (tree e, tsubst_flags_t complain)

We should probably grab the location first, and use it in the later diagnostics.

@@ -3404,11 +3414,13 @@ cp_build_array_ref (location_t loc, tree array, tree idx,
 	 pointer arithmetic.)  */
       idx = cp_perform_integral_promotions (idx, complain);
+ tree stripped_idx = tree_strip_any_location_wrapper (idx);

Perhaps this should be maybe_constant_value, to avoid marking the array as addressable in more cases.


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