| Message ID | 20221202154512.310755-1-jason@redhat.com |
|---|---|
| State | New |
| Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9674C3858410 for <patchwork@sourceware.org>; Fri, 2 Dec 2022 15:46:04 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 9674C3858410 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1669995964; bh=rqAQSziCZeC5acNStvSupyx5v4FT8lUU3M0ysF/Hh8g=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=hJ0XhTRMO1wsugs7kPLV7XAuIw+2GWVL24pzzFVwmJzcHo7U3WoNBZ4F8m900ptvs j9wqYa399aj5mWpq+uwydcXPajpbGhthWjHe4vfjZchB4fV1oQSc+sDePbjZcMAre1 g9QNVoQXj4BPX5sPISfj972fKf0EWf+9CYdzLznk= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 88C933858C39 for <gcc-patches@gcc.gnu.org>; Fri, 2 Dec 2022 15:45:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 88C933858C39 Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-298-cG9IPfasMZuki7ZnL3YQOQ-1; Fri, 02 Dec 2022 10:45:17 -0500 X-MC-Unique: cG9IPfasMZuki7ZnL3YQOQ-1 Received: by mail-qv1-f71.google.com with SMTP id og17-20020a056214429100b004c6ae186493so17246898qvb.3 for <gcc-patches@gcc.gnu.org>; Fri, 02 Dec 2022 07:45:17 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=rqAQSziCZeC5acNStvSupyx5v4FT8lUU3M0ysF/Hh8g=; b=vsBeEmxTPpl4pW9DBIY6Gso0wBnJOHog94w3t06h7tsO7U9IFI1hVUq+522SuKmoyr 7YgaBfxQUGqpbq4323P7ymP0lS1PbxBI5tFBnuJunstA/hlKnDSA1bNnQ6pAYNEb5opl NCha9gLi240slPMHwsgyiLKorT6hkSsYp1a5DF2Nw3Y4T9eKpc3zPrWx8FUMTvDFPUnS nvsv4CWqz6UtO+yiS0BzFZv4QatFx9k2gxIrhl+pJT8hKKE3ACKM1GdtnfmONR/2no3X 3g8pI22BSGmWFVyBemBP87nlahIJixSrqRhJmSX7WJb1a1CtlgUp+zQ2UnJyih/xafOh TX2A== X-Gm-Message-State: ANoB5pkipQwjODyF3ORuxbXIbE6v8JUD+vujRuT6EsQkjoLdkIx/CJmT RH8xuKxJlev2DWdPW656wbgvntZGYpAv2KNN5+V0/AcT9IR7YUjG0YWpoPfgWQrIGIza9MzmBYO YAATOaXa1vYbzEgUrIHBJqVAUgaw2TFkpiwJIgDnLrnZnvrYRNktYAAC5SjaN4S+lpQ== X-Received: by 2002:a05:622a:5c8f:b0:3a6:8b30:d8a7 with SMTP id ge15-20020a05622a5c8f00b003a68b30d8a7mr12460928qtb.132.1669995916461; Fri, 02 Dec 2022 07:45:16 -0800 (PST) X-Google-Smtp-Source: AA0mqf65f656femZD9RjmYV/VcmTbKxZcKC57fLImMVFQhVnql2tc5xlaLOwjX9QXXga0pr0qB+E+Q== X-Received: by 2002:a05:622a:5c8f:b0:3a6:8b30:d8a7 with SMTP id ge15-20020a05622a5c8f00b003a68b30d8a7mr12460899qtb.132.1669995915998; Fri, 02 Dec 2022 07:45:15 -0800 (PST) Received: from jason.com (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id dt4-20020a05620a478400b006fc9847d207sm5581520qkb.79.2022.12.02.07.45.14 for <gcc-patches@gcc.gnu.org> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Dec 2022 07:45:15 -0800 (PST) To: gcc-patches@gcc.gnu.org Subject: [PATCH RFA(tree)] c++: source position of lambda captures [PR84471] Date: Fri, 2 Dec 2022 10:45:12 -0500 Message-Id: <20221202154512.310755-1-jason@redhat.com> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jason Merrill <jason@redhat.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
| Series |
[RFA(tree)] c++: source position of lambda captures [PR84471]
|
|
Commit Message
Jason Merrill
Dec. 2, 2022, 3:45 p.m. UTC
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 base-commit: 70596a0fb2a2ec072e1e97e37616e05041dfa4e5
Comments
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;