Message ID | ory20ivw8z.fsf@lxoliva.fsfla.org |
---|---|
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 E99653858C50 for <patchwork@sourceware.org>; Wed, 6 Apr 2022 19:38:24 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E99653858C50 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1649273904; bh=E4I4vlkPwYNekiSEyYIVdsxtSiNTkEG4VSCfOH1iDRc=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=HiYBlu1CLzOC7kkvGorwRUb2U+3F1Vf/XMeRaBkh6GjMltQVEyBxNqjalJu3wXm0v x8+AHYp55HvWIR9UDz916uU2ve6YUeUWWueXVGW9Vr0q9813nnOEOpHAgqgT1K0nVF qEwjD7TjC2P1PeR9nLhGx0t1eGgEx/rzdzSya5e8= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from rock.gnat.com (rock.gnat.com [IPv6:2620:20:4000:0:a9e:1ff:fe9b:1d1]) by sourceware.org (Postfix) with ESMTPS id 8E580385842E for <gcc-patches@gcc.gnu.org>; Wed, 6 Apr 2022 19:37:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8E580385842E Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 6770E116D38; Wed, 6 Apr 2022 15:37:39 -0400 (EDT) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 5dO356GdMr9N; Wed, 6 Apr 2022 15:37:39 -0400 (EDT) Received: from free.home (tron.gnat.com [IPv6:2620:20:4000:0:46a8:42ff:fe0e:e294]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id 0ACFD116D33; Wed, 6 Apr 2022 15:37:38 -0400 (EDT) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTPS id 236JbWAf1913138 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 6 Apr 2022 16:37:32 -0300 To: gcc-patches@gcc.gnu.org Subject: set loc on call even if result is discarded Organization: Free thinker, does not speak for AdaCore Date: Wed, 06 Apr 2022 16:37:32 -0300 Message-ID: <ory20ivw8z.fsf@lxoliva.fsfla.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.84 X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Alexandre Oliva via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Alexandre Oliva <oliva@adacore.com> Cc: nathan@acm.org Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
set loc on call even if result is discarded
|
|
Commit Message
Alexandre Oliva
April 6, 2022, 7:37 p.m. UTC
This patch fixes a divergence in line numbers in diagnostics and, presumably, debug information, between targets whose cdtors return this and those that don't. The problem was visible in g++.dg/cpp2a/constexpr-dtor3.C: while the dtor call in the cleanup for f4 was expected at the closing brace, on returning-this targets it came up at the assignment. The reason is convoluted: statements in cleanups have their location information removed, to avoid bumpy debugger behavior, and then set to the location of the end of the scope. The cleanup dtor call has its locus cleared in both kinds of targets, but the end-of-scope locus doesn't make it on returning-this targets. The calls are wrapped with a cast-to-void to discard the unused return value, and the existing logic only attached the locus to the conversion NOP_EXPR. The call thus remains locus-less. When constexpr logic copies and evals the body, it sets unset locations; while copying cleanups, the locus is taken from the cleanup expression, rather than matching the end-of-scope locus set by the parser. So we end up with different locations. This patch sets the locus of the call even when it's wrapped by a convert-to-void NOP_EXPR, so it won't diverge any more. Regstrapped on x86_64-linux-gnu, also verified the testcase fix on arm-eabi. Ok to install? for gcc/ChangeLog * tree.cc (protected_set_expr_location): Propagate locus to call wrapped in cast-to-void. --- gcc/tree.cc | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
Comments
On 4/6/22 15:37, Alexandre Oliva wrote: Need to adjust this subject line, as well. > This patch fixes a divergence in line numbers in diagnostics and, > presumably, debug information, between targets whose cdtors return > this and those that don't. > > The problem was visible in g++.dg/cpp2a/constexpr-dtor3.C: while the > dtor call in the cleanup for f4 was expected at the closing brace, on > returning-this targets it came up at the assignment. > > The reason is convoluted: statements in cleanups have their location > information removed, to avoid bumpy debugger behavior, and then set to > the location of the end of the scope. > > The cleanup dtor call has its locus cleared in both kinds of targets, > but the end-of-scope locus doesn't make it on returning-this targets. > The calls are wrapped with a cast-to-void to discard the unused return > value, and the existing logic only attached the locus to the > conversion NOP_EXPR. > > The call thus remains locus-less. When constexpr logic copies and > evals the body, it sets unset locations; while copying cleanups, the > locus is taken from the cleanup expression, rather than matching the > end-of-scope locus set by the parser. So we end up with different > locations. > > This patch sets the locus of the call even when it's wrapped by a > convert-to-void NOP_EXPR, so it won't diverge any more. > > Regstrapped on x86_64-linux-gnu, also verified the testcase fix on > arm-eabi. Ok to install? > > > for gcc/ChangeLog > > * tree.cc (protected_set_expr_location): Propagate locus to > call wrapped in cast-to-void. I'm reluctant to put this C++-specific change in a simple function shared by all languages; how about handling it in set_cleanup_locs instead? > --- > gcc/tree.cc | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/gcc/tree.cc b/gcc/tree.cc > index ec200e9a7eb43..228d279ab0aa1 100644 > --- a/gcc/tree.cc > +++ b/gcc/tree.cc > @@ -5372,7 +5372,18 @@ void > protected_set_expr_location (tree t, location_t loc) > { > if (CAN_HAVE_LOCATION_P (t)) > - SET_EXPR_LOCATION (t, loc); > + { > + SET_EXPR_LOCATION (t, loc); > + /* Avoid locus differences for C++ cdtor calls depending on whether > + cdtor_returns_this: a conversion to void is added to discard the return > + value, and this conversion ends up carrying the location, and when it > + gets discarded, the location is lost. So hold it in the call as > + well. */ > + if (TREE_CODE (t) == NOP_EXPR > + && TREE_TYPE (t) == void_type_node > + && TREE_CODE (TREE_OPERAND (t, 0)) == CALL_EXPR) > + SET_EXPR_LOCATION (TREE_OPERAND (t, 0), loc); > + } > else if (t && TREE_CODE (t) == STATEMENT_LIST) > { > t = expr_single (t); > >
On Apr 6, 2022, Jason Merrill <jason@redhat.com> wrote: > On 4/6/22 15:37, Alexandre Oliva wrote: > Need to adjust this subject line, as well. *nod*, thanks >> * tree.cc (protected_set_expr_location): Propagate locus to >> call wrapped in cast-to-void. > I'm reluctant to put this C++-specific change in a simple function > shared by all languages; Perhaps it benefits other languages as well? The effect is presumably desirable on other languages too: setting a cast-to-void's location seems completely ineffective, as it's eventually thrown away, and perhaps propagating the location to any operand (rather than just calls) would carry out the intent of [protected_]set_expr_location more effectively. It doesn't feel right to require every caller to worry about that. > how about handling it in set_cleanup_locs instead? Like this? That seems reasonable to me. I'll give it a spin. diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index a7f6449dafd2e..43627ed30afcb 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -609,7 +609,17 @@ set_cleanup_locs (tree stmts, location_t loc) { if (TREE_CODE (stmts) == CLEANUP_STMT) { - protected_set_expr_location (CLEANUP_EXPR (stmts), loc); + tree t = CLEANUP_EXPR (stmts); + protected_set_expr_location (t, loc); + /* Avoid locus differences for C++ cdtor calls depending on whether + cdtor_returns_this: a conversion to void is added to discard the return + value, and this conversion ends up carrying the location, and when it + gets discarded, the location is lost. So hold it in the call as + well. */ + if (TREE_CODE (t) == NOP_EXPR + && TREE_TYPE (t) == void_type_node + && TREE_CODE (TREE_OPERAND (t, 0)) == CALL_EXPR) + protected_set_expr_location (TREE_OPERAND (t, 0), loc); set_cleanup_locs (CLEANUP_BODY (stmts), loc); } else if (TREE_CODE (stmts) == STATEMENT_LIST)
On Fri, Apr 8, 2022 at 12:49 AM Alexandre Oliva via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Apr 6, 2022, Jason Merrill <jason@redhat.com> wrote: > > > On 4/6/22 15:37, Alexandre Oliva wrote: > > Need to adjust this subject line, as well. > > *nod*, thanks > > >> * tree.cc (protected_set_expr_location): Propagate locus to > >> call wrapped in cast-to-void. > > > I'm reluctant to put this C++-specific change in a simple function > > shared by all languages; > > Perhaps it benefits other languages as well? The effect is presumably > desirable on other languages too: setting a cast-to-void's location > seems completely ineffective, as it's eventually thrown away, and > perhaps propagating the location to any operand (rather than just calls) > would carry out the intent of [protected_]set_expr_location more > effectively. It doesn't feel right to require every caller to worry > about that. Hmm, but then maybe it would be better the task of the code actually throwing away the cast? I agree the original proposed location to fix this looks bad. > > how about handling it in set_cleanup_locs instead? > > Like this? That seems reasonable to me. I'll give it a spin. > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index a7f6449dafd2e..43627ed30afcb 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -609,7 +609,17 @@ set_cleanup_locs (tree stmts, location_t loc) > { > if (TREE_CODE (stmts) == CLEANUP_STMT) > { > - protected_set_expr_location (CLEANUP_EXPR (stmts), loc); > + tree t = CLEANUP_EXPR (stmts); > + protected_set_expr_location (t, loc); > + /* Avoid locus differences for C++ cdtor calls depending on whether > + cdtor_returns_this: a conversion to void is added to discard the return > + value, and this conversion ends up carrying the location, and when it > + gets discarded, the location is lost. So hold it in the call as > + well. */ > + if (TREE_CODE (t) == NOP_EXPR > + && TREE_TYPE (t) == void_type_node > + && TREE_CODE (TREE_OPERAND (t, 0)) == CALL_EXPR) > + protected_set_expr_location (TREE_OPERAND (t, 0), loc); > set_cleanup_locs (CLEANUP_BODY (stmts), loc); > } > else if (TREE_CODE (stmts) == STATEMENT_LIST) > > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Disinformation flourishes because many people care deeply about injustice > but very few check the facts. Ask me about <https://stallmansupport.org>
On 4/7/22 18:48, Alexandre Oliva wrote: > On Apr 6, 2022, Jason Merrill <jason@redhat.com> wrote: > >> On 4/6/22 15:37, Alexandre Oliva wrote: >> Need to adjust this subject line, as well. > > *nod*, thanks > >>> * tree.cc (protected_set_expr_location): Propagate locus to >>> call wrapped in cast-to-void. > >> I'm reluctant to put this C++-specific change in a simple function >> shared by all languages; > > Perhaps it benefits other languages as well? The effect is presumably > desirable on other languages too: setting a cast-to-void's location > seems completely ineffective, as it's eventually thrown away, and > perhaps propagating the location to any operand (rather than just calls) > would carry out the intent of [protected_]set_expr_location more > effectively. It doesn't feel right to require every caller to worry > about that. > >> how about handling it in set_cleanup_locs instead? > > Like this? That seems reasonable to me. I'll give it a spin. Yes, or perhaps STRIP_NOPS and set the location on whatever is left. OK either way. > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index a7f6449dafd2e..43627ed30afcb 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -609,7 +609,17 @@ set_cleanup_locs (tree stmts, location_t loc) > { > if (TREE_CODE (stmts) == CLEANUP_STMT) > { > - protected_set_expr_location (CLEANUP_EXPR (stmts), loc); > + tree t = CLEANUP_EXPR (stmts); > + protected_set_expr_location (t, loc); > + /* Avoid locus differences for C++ cdtor calls depending on whether > + cdtor_returns_this: a conversion to void is added to discard the return > + value, and this conversion ends up carrying the location, and when it > + gets discarded, the location is lost. So hold it in the call as > + well. */ > + if (TREE_CODE (t) == NOP_EXPR > + && TREE_TYPE (t) == void_type_node > + && TREE_CODE (TREE_OPERAND (t, 0)) == CALL_EXPR) > + protected_set_expr_location (TREE_OPERAND (t, 0), loc); > set_cleanup_locs (CLEANUP_BODY (stmts), loc); > } > else if (TREE_CODE (stmts) == STATEMENT_LIST) > >
On Apr 9, 2022, Jason Merrill <jason@redhat.com> wrote: >>> how about handling it in set_cleanup_locs instead? >> Like this? That seems reasonable to me. I'll give it a spin. > Yes, or perhaps STRIP_NOPS and set the location on whatever is left. > OK either way. Hmm, I'm not sure leaving the loc unset on the NOP_EXPR won't have ill effects, so here's what I'm installing. Thanks! c++: Set loc on call even if result is discarded This patch fixes a divergence in line numbers in diagnostics and, presumably, debug information, between targets whose cdtors return this and those that don't. The problem was visible in g++.dg/cpp2a/constexpr-dtor3.C: while the dtor call in the cleanup for f4 was expected at the closing brace, on returning-this targets it came up at the assignment. The reason is convoluted: statements in cleanups have their location information removed, to avoid bumpy debugger behavior, and then set to the location of the end of the scope. The cleanup dtor call has its locus cleared in both kinds of targets, but the end-of-scope locus doesn't make it on returning-this targets. The calls are wrapped with a cast-to-void to discard the unused return value, and the existing logic only attached the locus to the conversion NOP_EXPR. The call thus remains locus-less. When constexpr logic copies and evals the body, it sets unset locations; while copying cleanups, the locus is taken from the cleanup expression, rather than matching the end-of-scope locus set by the parser. So we end up with different locations. This patch sets the locus of the call even when it's wrapped by a convert-to-void NOP_EXPR, so it won't diverge any more. for gcc/cp/ChangeLog * semantics.cc (set_cleanup_locs): Propagate locus to call wrapped in cast-to-void. --- gcc/cp/semantics.cc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index a7f6449dafd2e..43627ed30afcb 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -609,7 +609,17 @@ set_cleanup_locs (tree stmts, location_t loc) { if (TREE_CODE (stmts) == CLEANUP_STMT) { - protected_set_expr_location (CLEANUP_EXPR (stmts), loc); + tree t = CLEANUP_EXPR (stmts); + protected_set_expr_location (t, loc); + /* Avoid locus differences for C++ cdtor calls depending on whether + cdtor_returns_this: a conversion to void is added to discard the return + value, and this conversion ends up carrying the location, and when it + gets discarded, the location is lost. So hold it in the call as + well. */ + if (TREE_CODE (t) == NOP_EXPR + && TREE_TYPE (t) == void_type_node + && TREE_CODE (TREE_OPERAND (t, 0)) == CALL_EXPR) + protected_set_expr_location (TREE_OPERAND (t, 0), loc); set_cleanup_locs (CLEANUP_BODY (stmts), loc); } else if (TREE_CODE (stmts) == STATEMENT_LIST)
diff --git a/gcc/tree.cc b/gcc/tree.cc index ec200e9a7eb43..228d279ab0aa1 100644 --- a/gcc/tree.cc +++ b/gcc/tree.cc @@ -5372,7 +5372,18 @@ void protected_set_expr_location (tree t, location_t loc) { if (CAN_HAVE_LOCATION_P (t)) - SET_EXPR_LOCATION (t, loc); + { + SET_EXPR_LOCATION (t, loc); + /* Avoid locus differences for C++ cdtor calls depending on whether + cdtor_returns_this: a conversion to void is added to discard the return + value, and this conversion ends up carrying the location, and when it + gets discarded, the location is lost. So hold it in the call as + well. */ + if (TREE_CODE (t) == NOP_EXPR + && TREE_TYPE (t) == void_type_node + && TREE_CODE (TREE_OPERAND (t, 0)) == CALL_EXPR) + SET_EXPR_LOCATION (TREE_OPERAND (t, 0), loc); + } else if (t && TREE_CODE (t) == STATEMENT_LIST) { t = expr_single (t);