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.
next prev parent 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).