Message ID | 20221202154512.310755-1-jason@redhat.com |
---|---|
State | New |
Headers | show |
Series | [RFA(tree)] c++: source position of lambda captures [PR84471] | expand |
Ping. On 12/2/22 10:45, Jason Merrill 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); > + > h = ggc_alloc<tree_decl_map> (); > h->base.from = from; > h->to = to; > > base-commit: 70596a0fb2a2ec072e1e97e37616e05041dfa4e5
On 12/2/22 10:45, Jason Merrill 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); > + > h = ggc_alloc<tree_decl_map> (); > h->base.from = from; > h->to = to; > > base-commit: 70596a0fb2a2ec072e1e97e37616e05041dfa4e5
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 >
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.
> 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. > >
On 12/20/22 14:39, Richard Biener wrote: > > >> 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, here's what I pushed.
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); + h = ggc_alloc<tree_decl_map> (); h->base.from = from; h->to = to;