On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches
<[email protected]> 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 <[email protected]>
> ---
> 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);
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 didn't look at the gcov changes, leaving those to the gcov maintainer(s).
Richard.
>
> /* Set the source location of the && on the second 'if'. */
> @@ -3938,7 +3939,8 @@ shortcut_cond_r (tree pred, tree *true_label_p, tree
> *false_label_p,
> true_label_p = &local_label;
>
> /* Keep the original source location on the first 'if'. */
> - t = shortcut_cond_r (TREE_OPERAND (pred, 0), true_label_p, NULL,
> locus);
> + tree op0 = TREE_OPERAND (pred, 0);
> + t = shortcut_cond_r (op0, true_label_p, NULL, EXPR_LOCATION (op0));
> append_to_statement_list (t, &expr);
>
> /* Set the source location of the || on the second 'if'. */
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr97923.c
> b/gcc/testsuite/gcc.misc-tests/gcov-pr97923.c
> new file mode 100644
> index 00000000000..ad4f7d40817
> --- /dev/null
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr97923.c
> @@ -0,0 +1,13 @@
> +/* { dg-options "-fprofile-arcs -ftest-coverage" } */
> +/* { dg-do run { target native } } */
> +
> +int foo(int c)
> +{
> + return ((c >= 'A' && c <= 'Z') /* count(1*) */
> + || (c >= 'a' && c <= 'z') /* count(1*) */
> + || (c >= '0' && c <= '0')); /* count(1*) */
> +}
> +
> +int main() { foo(0); }
> +
> +/* { dg-final { run-gcov gcov-pr97923-1.c } } */
> --
> 2.27.0
>
