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

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

> Am 20.12.2022 um 18:38 schrieb Jason Merrill <jason@redhat.com>:
> 
> On 12/20/22 07:07, Richard Biener wrote:
>>> 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.
> 
> I think these expressions aren't ever shared in practice, but I agree it's safer to use the _unshare variant.  OK with that change?
> 
>> Maybe it would be easier to handle this in the consumers of the
>> DECL_VALUE_EXPR?  gimplify_var_or_parm_decl does
> 
> I don't see how auditing all the (many) users of DECL_VALUE_EXPR would be easier than doing it in this one place

It might do less unsharing.  But OK with the _unshare variant.

Thanks,
Richard 

>>   /* 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.
> 
>