On 12/20/22 14:39, Richard Biener wrote: > > >> Am 20.12.2022 um 18:38 schrieb Jason Merrill : >> >> On 12/20/22 07:07, Richard Biener wrote: >>>> On Fri, Dec 2, 2022 at 4:46 PM Jason Merrill via Gcc-patches >>>> 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.