From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 94B3F3858C83; Thu, 2 Mar 2023 10:02:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 94B3F3858C83 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id CB5A621C1F; Thu, 2 Mar 2023 10:02:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1677751334; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=wtBNonwC3ZlT7unlyLbi/+5hN2VWOIU8MmOgAYOpTPo=; b=ACpnOgJ+5HbRC0DSErg6y0hp/hEoEt4tKElg5WsqVaS4Ue35RPA2wYrhYz8GEWeXPSsekr 1HbbRMG5dL4Ml5m8JC0ZX9S/BWF6bE9BluExNnsAZ3xBUTbT0nKOx5nZ1tdrrhDykmWhDY f5NuJCyg0KRYBpkZ35MWkSqmTCTrNJY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1677751334; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=wtBNonwC3ZlT7unlyLbi/+5hN2VWOIU8MmOgAYOpTPo=; b=5zfrU8yeIHFziYj0ccMFsUMZBjmtQdcvfT3icsFj1V/k2wbRvQs9bfRsCaUZfZ5zc4cKfj CoF+iBo6h++TMqCw== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 8E5082C143; Thu, 2 Mar 2023 10:02:14 +0000 (UTC) Date: Thu, 2 Mar 2023 10:02:14 +0000 (UTC) From: Richard Biener To: Xionghu Luo cc: Richard Biener , Xionghu Luo , gcc-patches@gcc.gnu.org, luoxhu@gcc.gnu.org, hubicka@ucw.cz Subject: Re: [PATCH 2/2] gcov: Fix incorrect gimple line LOCATION [PR97923] In-Reply-To: <2956dda9-5205-372b-8c6a-842f185ad6e6@gmail.com> Message-ID: References: <20230302022921.4055291-1-xionghuluo@tencent.com> <20230302022921.4055291-2-xionghuluo@tencent.com> <2956dda9-5205-372b-8c6a-842f185ad6e6@gmail.com> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP,WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, 2 Mar 2023, Xionghu Luo wrote: > > > On 2023/3/2 16:16, Richard Biener wrote: > > On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches > > wrote: > >> > >> For case like belowi test.c: > >> > >> 1:int foo(char c) > >> 2:{ > >> 3: return ((c >= 'A' && c <= 'Z') > >> 4: || (c >= 'a' && c <= 'z') > >> 5: || (c >= '0' && c <='0'));} > >> > >> the generated line number is incorrect for condition c>='A' of block 2: > >> Thus correct the condition op0 location. > >> > >> gcno diff before and with this patch: > >> > >> test.gcno: 575: block 11: 1:0001(tree) > >> test.gcno: 583: 01450000: 35:LINES > >> -test.gcno: 595: block 2:`test.c':1, 5 > >> +test.gcno: 595: block 2:`test.c':1, 3 > >> test.gcno: 626: 01450000: 31:LINES > >> test.gcno: 638: block 3:`test.c':3 > >> test.gcno: 665: 01450000: 31:LINES > >> test.gcno: 677: block 4:`test.c':4 > >> test.gcno: 704: 01450000: 31:LINES > >> test.gcno: 716: block 5:`test.c':4 > >> test.gcno: 743: 01450000: 31:LINES > >> test.gcno: 755: block 6:`test.c':5 > >> > >> Also save line id in line vector for gcov debug use. > >> > >> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for > >> master? > >> > >> gcc/ChangeLog: > >> > >> PR gcov/97923 > >> * gcov.cc (line_info::line_info): Init id. > >> (solve_flow_graph): Fix typo. > >> (add_line_counts): Set line->id. > >> * gimplify.cc (shortcut_cond_r): Correct cond expr op0 location. > >> > >> gcc/testsuite/ChangeLog: > >> > >> PR gcov/97923 > >> * gcc.misc-tests/gcov-pr97923.c: New test. > >> > >> Signed-off-by: Xionghu Luo > >> --- > >> gcc/gcov.cc | 9 ++++++--- > >> gcc/gimplify.cc | 6 ++++-- > >> gcc/testsuite/gcc.misc-tests/gcov-pr97923.c | 13 +++++++++++++ > >> 3 files changed, 23 insertions(+), 5 deletions(-) > >> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr97923.c > >> > >> diff --git a/gcc/gcov.cc b/gcc/gcov.cc > >> index 2ec7248cc0e..77ca94c71c4 100644 > >> --- a/gcc/gcov.cc > >> +++ b/gcc/gcov.cc > >> @@ -205,6 +205,8 @@ public: > >> /* Execution count. */ > >> gcov_type count; > >> > >> + unsigned id; > >> + > >> /* Branches from blocks that end on this line. */ > >> vector branches; > >> > >> @@ -216,8 +218,8 @@ public: > >> unsigned has_unexecuted_block : 1; > >> }; > >> > >> -line_info::line_info (): count (0), branches (), blocks (), exists > >> (false), > >> - unexceptional (0), has_unexecuted_block (0) > >> +line_info::line_info (): count (0), id (0), branches (), blocks (), > >> + exists (false), unexceptional (0), has_unexecuted_block (0) > >> { > >> } > >> > >> @@ -2370,7 +2372,7 @@ solve_flow_graph (function_info *fn) > >> > >> /* If the graph has been correctly solved, every block will have a > >> valid count. */ > >> - for (unsigned i = 0; ix < fn->blocks.size (); i++) > >> + for (unsigned i = 0; i < fn->blocks.size (); i++) > >> if (!fn->blocks[i].count_valid) > >> { > >> fnotice (stderr, "%s:graph is unsolvable for '%s'\n", > >> @@ -2730,6 +2732,7 @@ add_line_counts (coverage_info *coverage, > >> function_info *fn) > >> } > >> line->count += block->count; > >> } > >> + line->id = ln; > >> } > >> > >> has_any_line = true; > >> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc > >> index ade6e335da7..341a27b033e 100644 > >> --- a/gcc/gimplify.cc > >> +++ b/gcc/gimplify.cc > >> @@ -3915,7 +3915,8 @@ shortcut_cond_r (tree pred, tree *true_label_p, tree > >> *false_label_p, > >> false_label_p = &local_label; > >> > >> /* Keep the original source location on the first 'if'. */ > >> - t = shortcut_cond_r (TREE_OPERAND (pred, 0), NULL, false_label_p, > >> locus); > >> + tree op0 = TREE_OPERAND (pred, 0); > >> + t = shortcut_cond_r (op0, NULL, false_label_p, EXPR_LOCATION (op0)); > >> append_to_statement_list (t, &expr); > > > > The comment now no longer is true? For the else arm we use > > rexpr_location, why not > > here as well? To quote the following lines: > > > > /* Set the source location of the && on the second 'if'. */ > > new_locus = rexpr_location (pred, locus); > > t = shortcut_cond_r (TREE_OPERAND (pred, 1), true_label_p, > > false_label_p, > > new_locus); > > append_to_statement_list (t, &expr); > > Thanks, should use rexpr_location with each operand like below. > > > > > > with your change the location of the outer COND_EXPR is lost? Can we > > guarantee > > that it's used for the first operand of a if (a && b && c)? It would > > be nice to expand > > the leading comment for such a three operand case and explain how it's > > supposed > > to work. > > I tested the three operand case, it will iteratively call shortcut_cond_r and > also works as expected. Seems the outer COND_EXPR is useless if we do the > followed conversion? > > > if (TREE_CODE (pred) == TRUTH_ANDIF_EXPR) > { > location_t new_locus; > > /* Turn if (a && b) into > > if (a); else goto no; > if (b) goto yes; else goto no; > (no:) */ > > if (false_label_p == NULL) > false_label_p = &local_label; > > - /* Keep the original source location on the first 'if'. */ > - tree op0 = TREE_OPERAND (pred, 0); > - t = shortcut_cond_r (op0, NULL, false_label_p, EXPR_LOCATION (op0)); > + /* Set the original source location on the first 'if'. */ > + tree op0 = TREE_OPERAND(pred, 0); > + new_locus = rexpr_location (op0, locus); > + t = shortcut_cond_r (op0, NULL, false_label_p, new_locus); // <= > append_to_statement_list (t, &expr); > > /* Set the source location of the && on the second 'if'. */ > - new_locus = rexpr_location (pred, locus); > - t = shortcut_cond_r (TREE_OPERAND (pred, 1), true_label_p, > false_label_p, > - new_locus); > + tree op1 = TREE_OPERAND(pred, 1); > + new_locus = rexpr_location (op1, locus); > + t = shortcut_cond_r (op1, true_label_p, false_label_p, new_locus); > append_to_statement_list (t, &expr); > } OK, so I think the original code did, for if (a && b) use the location of 'if (a && b)' for the emitted if (a); else goto no; and the location of 'a && b' for the emitted if (b) goto yes; else goto no; that kind of makes sense to me to some extent - the operands themselves keep their location but using the operands location for the generated 'if's doesn't make much sense to me? So I think the original code is correct. Richard. > > Breakpoint 5, shortcut_cond_r (pred=0x7ffff6f78fa0, true_label_p=0x0, > false_label_p=0x7fffffffbef0, locus=2147483654) at ../. > ./tgcc-master/gcc/gimplify.cc:3918 > (gdb) pel locus > {file = 0x3e3e940 "test.c", line = 5, column = 19, data = 0x0, sysp = false} > (gdb) n > (gdb) > (gdb) pel new_locus > {file = 0x3e3e940 "test.c", line = 4, column = 18, data = 0x0, sysp = false} > (gdb) ptree op0 > type size > unit-size > align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type > 0x7ffff6df2b28 precision:1 min df60f0 0> max > > arg:0 > arg:0 char> > used read QI test.c:1:15 size > unit-size > align:8 warn_if_not_align:0 context test> arg-type > > arg:1 > test.c:4:11 start: test.c:4:9 finish: test.c:4:16> > arg:1 > arg:0 > arg:1 > test.c:5:12 start: test.c:5:10 finish: test.c:5:17> > test.c:4:18 start: test.c:4:9 finish: test.c:5:17> > > > 1int test(char c) > 2{ > 3 return ( > 4 c >= 'A' && > 5 c >= 'M' && > 6 c <= 'Z'); > 7} > > > bbs: > > : > if (c_2(D) > 64) > goto ; [INV] > else > goto ; [INV] > > : > if (c_2(D) > 76) > goto ; [INV] > else > goto ; [INV] > > : > if (c_2(D) <= 90) > goto ; [INV] > else > goto ; [INV] > > > gcno diff before and with this patch: > > -test.gcno: 1312: block 2:`test.c':1, 5 > +test.gcno: 1312: block 2:`test.c':1, 4 > test.gcno: 1343: 01450000: 31:LINES > -test.gcno: 1355: block 3:`test.c':4 > +test.gcno: 1355: block 3:`test.c':5 > test.gcno: 1382: 01450000: 31:LINES > -test.gcno: 1394: block 4:`test.c':5 > +test.gcno: 1394: block 4:`test.c':6 > test.gcno: 1421: 01450000: 31:LINES > test.gcno: 1433: block 5:`test.c':5 > test.gcno: 1460: 01450000: 31:LINES > test.gcno: 1472: block 6:`test.c':5 > test.gcno: 1499: 01450000: 31:LINES > test.gcno: 1511: block 7:`test.c':5 > test.gcno: 1538: 01450000: 31:LINES > test.gcno: 1550: block 8:`test.c':5 > > , and are mapped to line 4, 5, 6 correctly now... > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)