Re: [PATCH RFA(tree)] c++: source position of lambda captures [PR84471]

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Dec 2, 2022 at 4:46 PM Jason Merrill via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Tested x86_64-pc-linux-gnu, OK for trunk?
>
> -- 8< --
>
> If the DECL_VALUE_EXPR of a VAR_DECL has EXPR_LOCATION set, then any use of
> that variable looks like it has that location, which leads to the debugger
> jumping back and forth for both lambdas and structured bindings.
>
> Rather than fix all the uses, it seems simplest to remove any EXPR_LOCATION
> when setting DECL_VALUE_EXPR.  So the cp/ hunks aren't necessary, but it
> seems cleaner not to work to add a location that will immediately get
> stripped.
>
>         PR c++/84471
>         PR c++/107504
>
> gcc/cp/ChangeLog:
>
>         * coroutines.cc (transform_local_var_uses): Don't
>         specify a location for DECL_VALUE_EXPR.
>         * decl.cc (cp_finish_decomp): Likewise.
>
> gcc/ChangeLog:
>
>         * tree.cc (decl_value_expr_insert): Clear EXPR_LOCATION.
>
> gcc/testsuite/ChangeLog:
>
>         * g++.dg/tree-ssa/value-expr1.C: New test.
>         * g++.dg/tree-ssa/value-expr2.C: New test.
>         * g++.dg/analyzer/pr93212.C: Move warning.
> ---
>  gcc/cp/coroutines.cc                        |  4 ++--
>  gcc/cp/decl.cc                              | 12 +++-------
>  gcc/testsuite/g++.dg/analyzer/pr93212.C     |  4 ++--
>  gcc/testsuite/g++.dg/tree-ssa/value-expr1.C | 16 +++++++++++++
>  gcc/testsuite/g++.dg/tree-ssa/value-expr2.C | 26 +++++++++++++++++++++
>  gcc/tree.cc                                 |  3 +++
>  6 files changed, 52 insertions(+), 13 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/tree-ssa/value-expr1.C
>  create mode 100644 gcc/testsuite/g++.dg/tree-ssa/value-expr2.C
>
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 01a3e831ee5..a72bd6bbef0 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -2047,8 +2047,8 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
>             = lookup_member (lvd->coro_frame_type, local_var.field_id,
>                              /*protect=*/1, /*want_type=*/0,
>                              tf_warning_or_error);
> -         tree fld_idx = build3_loc (lvd->loc, COMPONENT_REF, TREE_TYPE (lvar),
> -                                    lvd->actor_frame, fld_ref, NULL_TREE);
> +         tree fld_idx = build3 (COMPONENT_REF, TREE_TYPE (lvar),
> +                                lvd->actor_frame, fld_ref, NULL_TREE);
>           local_var.field_idx = fld_idx;
>           SET_DECL_VALUE_EXPR (lvar, fld_idx);
>           DECL_HAS_VALUE_EXPR_P (lvar) = true;
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index 7af0b05d5f8..59e21581503 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -9133,9 +9133,7 @@ cp_finish_decomp (tree decl, tree first, unsigned int count)
>           if (processing_template_decl)
>             continue;
>           tree t = unshare_expr (dexp);
> -         t = build4_loc (DECL_SOURCE_LOCATION (v[i]), ARRAY_REF,
> -                         eltype, t, size_int (i), NULL_TREE,
> -                         NULL_TREE);
> +         t = build4 (ARRAY_REF, eltype, t, size_int (i), NULL_TREE, NULL_TREE);
>           SET_DECL_VALUE_EXPR (v[i], t);
>           DECL_HAS_VALUE_EXPR_P (v[i]) = 1;
>         }
> @@ -9154,9 +9152,7 @@ cp_finish_decomp (tree decl, tree first, unsigned int count)
>           if (processing_template_decl)
>             continue;
>           tree t = unshare_expr (dexp);
> -         t = build1_loc (DECL_SOURCE_LOCATION (v[i]),
> -                         i ? IMAGPART_EXPR : REALPART_EXPR, eltype,
> -                         t);
> +         t = build1 (i ? IMAGPART_EXPR : REALPART_EXPR, eltype, t);
>           SET_DECL_VALUE_EXPR (v[i], t);
>           DECL_HAS_VALUE_EXPR_P (v[i]) = 1;
>         }
> @@ -9180,9 +9176,7 @@ cp_finish_decomp (tree decl, tree first, unsigned int count)
>           tree t = unshare_expr (dexp);
>           convert_vector_to_array_for_subscript (DECL_SOURCE_LOCATION (v[i]),
>                                                  &t, size_int (i));
> -         t = build4_loc (DECL_SOURCE_LOCATION (v[i]), ARRAY_REF,
> -                         eltype, t, size_int (i), NULL_TREE,
> -                         NULL_TREE);
> +         t = build4 (ARRAY_REF, eltype, t, size_int (i), NULL_TREE, NULL_TREE);
>           SET_DECL_VALUE_EXPR (v[i], t);
>           DECL_HAS_VALUE_EXPR_P (v[i]) = 1;
>         }
> diff --git a/gcc/testsuite/g++.dg/analyzer/pr93212.C b/gcc/testsuite/g++.dg/analyzer/pr93212.C
> index 41507e2b837..1029e8d547b 100644
> --- a/gcc/testsuite/g++.dg/analyzer/pr93212.C
> +++ b/gcc/testsuite/g++.dg/analyzer/pr93212.C
> @@ -4,8 +4,8 @@
>  auto lol()
>  {
>      int aha = 3;
> -    return [&aha] { // { dg-warning "dereferencing pointer '.*' to within stale stack frame" }
> -        return aha;
> +    return [&aha] {
> +        return aha; // { dg-warning "dereferencing pointer '.*' to within stale stack frame" }
>      };
>      /* TODO: may be worth special-casing the reporting of dangling
>         references from lambdas, to highlight the declaration, and maybe fix
> diff --git a/gcc/testsuite/g++.dg/tree-ssa/value-expr1.C b/gcc/testsuite/g++.dg/tree-ssa/value-expr1.C
> new file mode 100644
> index 00000000000..946ccc3bd97
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/tree-ssa/value-expr1.C
> @@ -0,0 +1,16 @@
> +// PR c++/84471
> +// { dg-do compile { target c++11 } }
> +// { dg-additional-options -fdump-tree-gimple-lineno }
> +// { dg-final { scan-tree-dump-not {value-expr: \[} "gimple" } }
> +
> +int main(int argc, char**)
> +{
> +  int x = 1;
> +  auto f = [&x, &argc](const char* i) {
> +    i += x;
> +    i -= argc;
> +    i += argc - x;
> +    return i;
> +  };
> +  f("          ");
> +}
> diff --git a/gcc/testsuite/g++.dg/tree-ssa/value-expr2.C b/gcc/testsuite/g++.dg/tree-ssa/value-expr2.C
> new file mode 100644
> index 00000000000..4d00498f214
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/tree-ssa/value-expr2.C
> @@ -0,0 +1,26 @@
> +// PR c++/107504
> +// { dg-do compile { target c++17 } }
> +// { dg-additional-options -fdump-tree-gimple-lineno }
> +// { dg-final { scan-tree-dump-not {value-expr: \[} "gimple" } }
> +
> +struct S
> +{
> +  void* i;
> +  int j;
> +};
> +
> +S f(char* p)
> +{
> +  return {p, 1};
> +}
> +
> +int main()
> +{
> +  char buf[1];
> +  auto [x, y] = f(buf);
> +  if (x != buf)
> +    throw 1;
> +  if (y != 1)
> +    throw 2;
> +  return 0;
> +}
> diff --git a/gcc/tree.cc b/gcc/tree.cc
> index 254b2373dcf..836c51cd4d5 100644
> --- a/gcc/tree.cc
> +++ b/gcc/tree.cc
> @@ -5862,6 +5862,9 @@ decl_value_expr_insert (tree from, tree to)
>  {
>    struct tree_decl_map *h;
>
> +  /* Uses of FROM shouldn't look like they happen at the location of TO.  */
> +  protected_set_expr_location (to, UNKNOWN_LOCATION);
> +

Doesn't that mean we'd eventually want unshare_expr_without_location
or similar here?  Or rather maybe set the location of TO to that of
FROM?  That said, this modifies FROM in place - we have
protected_set_expr_location_unshare (would need to be exported
from fold-const.cc) to avoid clobbering a possibly shared tree.

Maybe it would be easier to handle this in the consumers of the
DECL_VALUE_EXPR?  gimplify_var_or_parm_decl does

  /* If the decl is an alias for another expression, substitute it now.  */
  if (DECL_HAS_VALUE_EXPR_P (decl))
    {
      *expr_p = unshare_expr (DECL_VALUE_EXPR (decl));
      return GS_OK;

it could also unshare without location.

>    h = ggc_alloc<tree_decl_map> ();
>    h->base.from = from;
>    h->to = to;
>
> base-commit: 70596a0fb2a2ec072e1e97e37616e05041dfa4e5
> --
> 2.31.1
>