Re: [patch] beat location sense into fold (PR/40435)

[Richard Henderson <rth at redhat dot com>]

>  expand_builtin_strstr (tree exp, rtx target, enum machine_mode mode)
>  {
>    if (validate_arglist (exp, POINTER_TYPE, POINTER_TYPE, VOID_TYPE))
>      {
>        tree type = TREE_TYPE (exp);
> -      tree result = fold_builtin_strstr (CALL_EXPR_ARG (exp, 0),
> +      tree result = fold_builtin_strstr (BUILTINS_LOCATION,
> +                                        CALL_EXPR_ARG (exp, 0),
>                                          CALL_EXPR_ARG (exp, 1), type);

This usage of BUILTINS_LOCATION is incorrect.  The only thing that 
should have this loc is a builtin/library decl itself.  Here you should 
be using the location from EXP.

Similarly for the other occurrences in builtins.c.

> @@ -5018,11 +5055,12 @@ gimplify_va_arg_expr (tree *expr_p, gimp
>        if (warned)
>         inform (loc, "if this code is reached, the program will abort");
>        /* Before the abort, allow the evaluation of the va_list
>          expression to exit or longjmp.  */
>        gimplify_and_add (valist, pre_p);
> -      t = build_call_expr (implicit_built_in_decls[BUILT_IN_TRAP], 0);
> +      t = build_call_expr_loc (input_location,
> +                          implicit_built_in_decls[BUILT_IN_TRAP], 0);

You've already computed a loc in gimplify_va_arg_expr, but failed to use 
it here.  That said it brings up a related problem -- the fact that the 
targetm.gimplify_va_arg_expr callback doesn't have a loc to work with. 
I suggest either modifying that callback, or setting input_location 
(with save/restore) around the call.  The later would certainly be 
easiest, and you've already used input_location in std_gimplify_va_arg_expr.

>  emutls_var_address (tree var)
>  {
>    tree emuvar = emutls_decl (var);
>    tree fn = built_in_decls [BUILT_IN_EMUTLS_GET_ADDRESS];
> -  tree arg = build_fold_addr_expr_with_type (emuvar, ptr_type_node);
> +  tree arg = build_fold_addr_expr_with_type_loc (BUILTINS_LOCATION,
> +                                            emuvar, ptr_type_node);
>    tree arglist = build_tree_list (NULL_TREE, arg);
> -  tree call = build_function_call_expr (fn, arglist);
> -  return fold_convert (build_pointer_type (TREE_TYPE (var)), call);
> +  tree call = build_function_call_expr (BUILTINS_LOCATION, fn, arglist);
> +  return fold_convert_loc (BUILTINS_LOCATION,
> +                          build_pointer_type (TREE_TYPE (var)), call);
>  }

Ideally you'd pass in a location, derived from the context in which this 
address is being requested.  Failing that, UNKNOWN_LOCATION.

> @@ -390,11 +390,12 @@ ifcombine_ifandif (basic_block inner_con
>      {
>        enum tree_code code1 = gimple_cond_code (inner_cond);
>        enum tree_code code2 = gimple_cond_code (outer_cond);
>        tree t;
>
> -      if (!(t = combine_comparisons (TRUTH_ANDIF_EXPR, code1, code2,
> +      if (!(t = combine_comparisons (input_location,
> +                                    TRUTH_ANDIF_EXPR, code1, code2,

Guenther already mentioned this one.  Definitely wrong for 
input_location to appear (just about) anywhere within the optimizers. 
The only exception is with some forms of walk_gimple_{stmt,seq} with
wi->want_locations set.

> @@ -1313,16 +1313,16 @@ gimplify_vla_decl (tree decl, gimple_seq
>       replacement to use.  Second, it lets the debug info know
>       where to find the value.  */
>    ptr_type = build_pointer_type (TREE_TYPE (decl));
>    addr = create_tmp_var (ptr_type, get_name (decl));
>    DECL_IGNORED_P (addr) = 0;
> -  t = build_fold_indirect_ref (addr);
> +  t = build_fold_indirect_ref_loc (input_location, addr);

Here I think you should use the UNKNOWN_LOCATION.  The only other thing 
that would make sense is for DECL_VALUE_EXPRs to be given a new location 
wherever they are used.  Because, in a way, the computation of that 
value is caused by whatever code used the decl.  Further, it would be 
very distracting within the debugger for "c = a+b" to jump around to the 
declaration points of a and b while single-stepping through that statement.

> @@ -579,19 +579,21 @@ mf_build_check_statement_for (tree base,
>    mf_base = make_rename_temp (mf_uintptr_type, "__mf_base");
>    mf_limit = make_rename_temp (mf_uintptr_type, "__mf_limit");
>
>    /* Build: __mf_base = (uintptr_t) <base address expression>.  */
>    seq = gimple_seq_alloc ();
> -  t = fold_convert (mf_uintptr_type, unshare_expr (base));
> +  t = fold_convert_loc (BUILTINS_LOCATION, mf_uintptr_type,
> +                       unshare_expr (base));

More incorrect BUILTINS_LOCATION usage.

>  gimple_build_call_1 (tree fn, unsigned nargs)
>  {
>    gimple s = gimple_build_with_ops (GIMPLE_CALL, ERROR_MARK, nargs + 3);
>    if (TREE_CODE (fn) == FUNCTION_DECL)
> -    fn = build_fold_addr_expr (fn);
> +    fn = build_fold_addr_expr_loc (input_location, fn);

Either pass down a proper location or use UNKNOWN.



r~