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

[Jason Merrill <jason at redhat dot com>]

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)
> >    https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00304.html
> >
> > [PATCH 2/2] C++: improvements to binary operator diagnostics (PR c++/87504)
> >    https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00303.html
> >
> >
> > Thanks
> > Dave
>
> > > [1] I've split them up for ease of review; they could be reworked to
> > > be
> > > fully independent, but there's some churn in the results for
> > > -Wtautological-compare introduced by the 1st patch which the 2nd
> > > patch addresses.
> > >
> > > gcc/ChangeLog:
> > > 	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
> > > 	wrappers.
> > > 	(convert_to_real_maybe_fold): Likewise.
> > > 	(convert_to_integer_1): Handle location wrappers when checking
> > > for
> > > 	INTEGER_CST.
> > > 	(convert_to_integer_maybe_fold): Update to handle location
> > > 	wrappers.
> > > 	(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::convert_c_tests.
> > > 	* 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.
> > >
> > > gcc/c-family/ChangeLog:
> > > 	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.
> > > 	Handle CONST_DECL.
> > > 	(fold_offsetof): Strip any location wrapper.
> > > 	(complete_array_type): Likewise for initial_value.
> > > 	(convert_vector_to_array_for_subscript): Call fold_for_warn on
> > > the
> > > 	index before checking for INTEGER_CST.
> > > 	* c-pretty-print.c (c_pretty_printer::primary_expression):
> > > Don't
> > > 	print parentheses around location wrappers.
> > > 	* c-warn.c (warn_logical_operator): Call fold_for_warn on
> > > op_right
> > > 	before checking for INTEGER_CST.
> > > 	(warn_tautological_bitwise_comparison): Call
> > > 	tree_strip_any_location_wrapper on lhs, rhs, and bitop's
> > > operand
> > > 	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
> > > if
> > > 	available.
> > >
> > > gcc/cp/ChangeLog:
> > > 	PR c++/43064
> > > 	PR c++/43486
> > > 	* call.c (build_conditional_expr_1): Strip location wrappers
> > > when
> > > 	checking for CONST_DECL.
> > > 	(conversion_null_warnings): Use location of "expr" if
> > > available.
> > > 	* 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
> > > INTEGER_CST.
> > > 	* 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
> > > 	DECL_P.
> > > 	(warn_placement_new_too_small): Call fold_for_warn on adj
> > > before
> > > 	checking for CONSTANT_CLASS_P, and on nelts.  Strip any
> > > location
> > > 	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
> > > from
> > > 	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
> > > 	checking for DECL_IS_BUILTIN_CONSTANT_P.
> > > 	(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
> > > 	id-expression.
> > > 	(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
> > > within
> > > 	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
> > > 	DECL_P.
> > > 	* 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
> > > before
> > > 	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
> > > 	FUNCTION_DECL.
> > > 	(cp_build_modify_expr): Likewise for newrhs when checking for
> > > 	STRING_CST.
> > > 	(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.
> > >
> > > gcc/objc/ChangeLog:
> > > 	PR c++/43064
> > > 	PR c++/43486
> > > 	* objc-act.c (objc_maybe_build_component_ref): Strip any
> > > location
> > > 	wrapper before checking for UOBJC_SUPER_decl and self_decl.
> > > 	(objc_finish_message_expr): Strip any location wrapper.
> > >
> > > gcc/testsuite/ChangeLog:
> > > 	PR c++/43064
> > > 	PR c++/43486
> > > 	* c-c++-common/pr51712.c (valid2): Mark xfail as passing on
> > > C++.
> > > 	* 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
> > > the
> > > 	initializer.
> > > 	* 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)
> +    {
> +      STRIP_ANY_LOCATION_WRAPPER (index);
> +      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;
>  
> +    case VIEW_CONVERT_EXPR:
> +      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.

> +	    STRIP_ANY_LOCATION_WRAPPER (from);
> +	    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)
> +    STRIP_ANY_LOCATION_WRAPPER (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;
>      }
> +
> +  STRIP_ANY_LOCATION_WRAPPER (initializer);

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,
> +  STRIP_ANY_LOCATION_WRAPPER (fn_no_ptr);

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)
> +  STRIP_ANY_LOCATION_WRAPPER (e);
> @@ -1754,6 +1756,8 @@ cxx_alignof_expr (tree e, tsubst_flags_t complain)
> +  STRIP_ANY_LOCATION_WRAPPER (e);

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.

Jason