public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jason Merrill <jason@redhat.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH RFA(tree)] c++: source position of lambda captures [PR84471]
Date: Tue, 20 Dec 2022 12:38:20 -0500	[thread overview]
Message-ID: <c75934a6-1894-c711-6c5d-5e4de2742bec@redhat.com> (raw)
In-Reply-To: <CAFiYyc1shqzgx_5y=F=1RaEiMQyQft8uL8DiEXDpHGNqyx8XcQ@mail.gmail.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?

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



  reply	other threads:[~2022-12-20 17:38 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-02 15:45 Jason Merrill
2022-12-08 18:30 ` Jason Merrill
2022-12-19 16:00 ` [PATCH PING 2 (tree)] " Jason Merrill
2022-12-20 12:07 ` [PATCH RFA(tree)] " Richard Biener
2022-12-20 17:38   ` Jason Merrill [this message]
2022-12-20 19:39     ` Richard Biener
2022-12-21  2:14       ` Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c75934a6-1894-c711-6c5d-5e4de2742bec@redhat.com \
    --to=jason@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=richard.guenther@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).