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
> > <gcc-patches@gcc.gnu.org> 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 <xionghu...@tencent.com>
> >> ---
> >> 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<arc_info *> 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
> <truth_andif_expr 0x7ffff6f78f50
> type <boolean_type 0x7ffff6df2b28 _Bool public unsigned QI
> size <integer_cst 0x7ffff6dd3e88 constant 8>
> unit-size <integer_cst 0x7ffff6dd3ea0 constant 1>
> align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> 0x7ffff6df2b28 precision:1 min <integer_cst 0x7ffff6
> df60f0 0> max <integer_cst 0x7ffff6df6120 1>>
> arg:0 <gt_expr 0x7ffff6f78eb0 type <boolean_type 0x7ffff6df2b28 _Bool>
> arg:0 <parm_decl 0x7ffff7ff7080 c type <integer_type 0x7ffff6df23f0
> char>
> used read QI test.c:1:15 size <integer_cst 0x7ffff6dd3e88 8>
> unit-size <integer_cst 0x7ffff6dd3ea0 1>
> align:8 warn_if_not_align:0 context <function_decl 0x7ffff6f7e800
> test> arg-type <integer_type 0x7ffff6df25e8 int>>
> arg:1 <integer_cst 0x7ffff6f88a08 constant 64>
> test.c:4:11 start: test.c:4:9 finish: test.c:4:16>
> arg:1 <gt_expr 0x7ffff6f78ed8 type <boolean_type 0x7ffff6df2b28 _Bool>
> arg:0 <parm_decl 0x7ffff7ff7080 c>
> arg:1 <integer_cst 0x7ffff6f88a38 constant 76>
> 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:
>
> <bb 2> :
> if (c_2(D) > 64)
> goto <bb 3>; [INV]
> else
> goto <bb 6>; [INV]
>
> <bb 3> :
> if (c_2(D) > 76)
> goto <bb 4>; [INV]
> else
> goto <bb 6>; [INV]
>
> <bb 4> :
> if (c_2(D) <= 90)
> goto <bb 5>; [INV]
> else
> goto <bb 6>; [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
>
> <bb 2>, <bb 3> and <bb 4> are mapped to line 4, 5, 6 correctly now...
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
