* [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
@ 2023-03-02 2:29 Xionghu Luo
2023-03-02 2:29 ` [PATCH 2/2] gcov: Fix incorrect gimple line LOCATION [PR97923] Xionghu Luo
2023-03-02 8:41 ` [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] Richard Biener
0 siblings, 2 replies; 19+ messages in thread
From: Xionghu Luo @ 2023-03-02 2:29 UTC (permalink / raw)
To: gcc-patches; +Cc: luoxhu, rguenther, hubicka, Xionghu Luo
When spliting edge with self loop, the split edge should be placed just next to
the edge_in->src, otherwise it may generate different position latch bbs for
two consecutive self loops. For details, please refer to:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93680#c4
Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
master?
gcc/ChangeLog:
PR gcov/93680
* tree-cfg.cc (split_edge_bb_loc): Return edge_in->src for self loop.
gcc/testsuite/ChangeLog:
PR gcov/93680
* gcc.misc-tests/gcov-pr93680.c: New test.
Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
---
gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 +++++++++++++++++++++
gcc/tree-cfg.cc | 2 +-
2 files changed, 25 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
new file mode 100644
index 00000000000..b2bf9e626fc
--- /dev/null
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
@@ -0,0 +1,24 @@
+/* { dg-options "-fprofile-arcs -ftest-coverage" } */
+/* { dg-do run { target native } } */
+
+int f(int s, int n)
+{
+ int p = 0;
+
+ switch (s)
+ {
+ case 0: /* count(5) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+
+ case 1: /* count(5) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+ }
+
+ return 0;
+}
+
+int main() { f(0, 5); f(1, 5); return 0; }
+
+/* { dg-final { run-gcov gcov-pr93680.c } } */
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index a9fcc7fd050..6fa1d83d366 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -3009,7 +3009,7 @@ split_edge_bb_loc (edge edge_in)
if (dest_prev)
{
edge e = find_edge (dest_prev, dest);
- if (e && !(e->flags & EDGE_COMPLEX))
+ if ((e && !(e->flags & EDGE_COMPLEX)) || edge_in->src == edge_in->dest)
return edge_in->src;
}
return dest_prev;
--
2.27.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/2] gcov: Fix incorrect gimple line LOCATION [PR97923]
2023-03-02 2:29 [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] Xionghu Luo
@ 2023-03-02 2:29 ` Xionghu Luo
2023-03-02 8:16 ` Richard Biener
2023-03-02 8:41 ` [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] Richard Biener
1 sibling, 1 reply; 19+ messages in thread
From: Xionghu Luo @ 2023-03-02 2:29 UTC (permalink / raw)
To: gcc-patches; +Cc: luoxhu, rguenther, hubicka, Xionghu Luo
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 <xionghuluo@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);
/* 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] gcov: Fix incorrect gimple line LOCATION [PR97923]
2023-03-02 2:29 ` [PATCH 2/2] gcov: Fix incorrect gimple line LOCATION [PR97923] Xionghu Luo
@ 2023-03-02 8:16 ` Richard Biener
2023-03-02 9:43 ` Xionghu Luo
0 siblings, 1 reply; 19+ messages in thread
From: Richard Biener @ 2023-03-02 8:16 UTC (permalink / raw)
To: Xionghu Luo; +Cc: gcc-patches, luoxhu, rguenther, hubicka
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 <xionghuluo@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);
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-02 2:29 [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] Xionghu Luo
2023-03-02 2:29 ` [PATCH 2/2] gcov: Fix incorrect gimple line LOCATION [PR97923] Xionghu Luo
@ 2023-03-02 8:41 ` Richard Biener
2023-03-02 10:22 ` Xionghu Luo
1 sibling, 1 reply; 19+ messages in thread
From: Richard Biener @ 2023-03-02 8:41 UTC (permalink / raw)
To: Xionghu Luo; +Cc: gcc-patches, luoxhu, rguenther, hubicka
On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> When spliting edge with self loop, the split edge should be placed just next to
> the edge_in->src, otherwise it may generate different position latch bbs for
> two consecutive self loops. For details, please refer to:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93680#c4
>
> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
> master?
>
> gcc/ChangeLog:
>
> PR gcov/93680
> * tree-cfg.cc (split_edge_bb_loc): Return edge_in->src for self loop.
>
> gcc/testsuite/ChangeLog:
>
> PR gcov/93680
> * gcc.misc-tests/gcov-pr93680.c: New test.
>
> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
> ---
> gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 +++++++++++++++++++++
> gcc/tree-cfg.cc | 2 +-
> 2 files changed, 25 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
>
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> new file mode 100644
> index 00000000000..b2bf9e626fc
> --- /dev/null
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> @@ -0,0 +1,24 @@
> +/* { dg-options "-fprofile-arcs -ftest-coverage" } */
> +/* { dg-do run { target native } } */
> +
> +int f(int s, int n)
> +{
> + int p = 0;
> +
> + switch (s)
> + {
> + case 0: /* count(5) */
> + do { p++; } while (--n); /* count(5) */
> + return p; /* count(1) */
> +
> + case 1: /* count(5) */
> + do { p++; } while (--n); /* count(5) */
> + return p; /* count(1) */
> + }
> +
> + return 0;
> +}
> +
> +int main() { f(0, 5); f(1, 5); return 0; }
> +
> +/* { dg-final { run-gcov gcov-pr93680.c } } */
> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> index a9fcc7fd050..6fa1d83d366 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -3009,7 +3009,7 @@ split_edge_bb_loc (edge edge_in)
> if (dest_prev)
> {
> edge e = find_edge (dest_prev, dest);
> - if (e && !(e->flags & EDGE_COMPLEX))
> + if ((e && !(e->flags & EDGE_COMPLEX)) || edge_in->src == edge_in->dest)
I think this should eventually apply to all backedge edge_in, correct?
But of course
we cannot easily test for this here.
Still since this affects ordering in the {next,prev}_bb chain only but not CFG
semantics I wonder how it can affect coverage? Isn't it only by chance that
this block order survives?
For the case when both edge_in->src has more than one successor and
edge_in->dest has more than one predecessor there isn't any good heuristic
to make printing the blocks in chain order "nice" (well, the backedge
one maybe).
But as said - this order shouldn't have any effect on semantics ...
> return edge_in->src;
> }
> return dest_prev;
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] gcov: Fix incorrect gimple line LOCATION [PR97923]
2023-03-02 8:16 ` Richard Biener
@ 2023-03-02 9:43 ` Xionghu Luo
2023-03-02 10:02 ` Richard Biener
0 siblings, 1 reply; 19+ messages in thread
From: Xionghu Luo @ 2023-03-02 9:43 UTC (permalink / raw)
To: Richard Biener, Xionghu Luo; +Cc: gcc-patches, luoxhu, rguenther, hubicka
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 <xionghuluo@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);
}
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...
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] gcov: Fix incorrect gimple line LOCATION [PR97923]
2023-03-02 9:43 ` Xionghu Luo
@ 2023-03-02 10:02 ` Richard Biener
0 siblings, 0 replies; 19+ messages in thread
From: Richard Biener @ 2023-03-02 10:02 UTC (permalink / raw)
To: Xionghu Luo; +Cc: Richard Biener, Xionghu Luo, gcc-patches, luoxhu, hubicka
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 <xionghuluo@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 <rguenther@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)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-02 8:41 ` [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] Richard Biener
@ 2023-03-02 10:22 ` Xionghu Luo
2023-03-02 10:45 ` Richard Biener
0 siblings, 1 reply; 19+ messages in thread
From: Xionghu Luo @ 2023-03-02 10:22 UTC (permalink / raw)
To: Richard Biener, Xionghu Luo; +Cc: gcc-patches, luoxhu, rguenther, hubicka
On 2023/3/2 16:41, Richard Biener wrote:
> On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> When spliting edge with self loop, the split edge should be placed just next to
>> the edge_in->src, otherwise it may generate different position latch bbs for
>> two consecutive self loops. For details, please refer to:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93680#c4
>>
>> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
>> master?
>>
>> gcc/ChangeLog:
>>
>> PR gcov/93680
>> * tree-cfg.cc (split_edge_bb_loc): Return edge_in->src for self loop.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR gcov/93680
>> * gcc.misc-tests/gcov-pr93680.c: New test.
>>
>> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
>> ---
>> gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 +++++++++++++++++++++
>> gcc/tree-cfg.cc | 2 +-
>> 2 files changed, 25 insertions(+), 1 deletion(-)
>> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
>>
>> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
>> new file mode 100644
>> index 00000000000..b2bf9e626fc
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
>> @@ -0,0 +1,24 @@
>> +/* { dg-options "-fprofile-arcs -ftest-coverage" } */
>> +/* { dg-do run { target native } } */
>> +
>> +int f(int s, int n)
>> +{
>> + int p = 0;
>> +
>> + switch (s)
>> + {
>> + case 0: /* count(5) */
>> + do { p++; } while (--n); /* count(5) */
>> + return p; /* count(1) */
>> +
>> + case 1: /* count(5) */
>> + do { p++; } while (--n); /* count(5) */
>> + return p; /* count(1) */
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int main() { f(0, 5); f(1, 5); return 0; }
>> +
>> +/* { dg-final { run-gcov gcov-pr93680.c } } */
>> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
>> index a9fcc7fd050..6fa1d83d366 100644
>> --- a/gcc/tree-cfg.cc
>> +++ b/gcc/tree-cfg.cc
>> @@ -3009,7 +3009,7 @@ split_edge_bb_loc (edge edge_in)
>> if (dest_prev)
>> {
>> edge e = find_edge (dest_prev, dest);
>> - if (e && !(e->flags & EDGE_COMPLEX))
>> + if ((e && !(e->flags & EDGE_COMPLEX)) || edge_in->src == edge_in->dest)
>
> I think this should eventually apply to all backedge edge_in, correct?
> But of course
> we cannot easily test for this here.
>
> Still since this affects ordering in the {next,prev}_bb chain only but not CFG
> semantics I wonder how it can affect coverage? Isn't it only by chance that
> this block order survives?
For case:
1 int f(int s, int n)
2 {
3 int p = 0;
4 int q = 0;
5
6 switch (s)
7 {
8 case 0:
9 do { p++; } while (--n);
10 return p;
11
12 case 1:
13 do { p++; } while (--n);
14 return p;
15 }
16
17 return 0;
18 }
19
20 int main() { f(0, 5); f(1, 5);}
current GCC generates:
<bb 2> :
...
<bb 3> : <= first loop
...
goto <bb 4>; [INV]
else
goto <bb 5>; [INV]
<bb 4> : <= first latch bb
goto <bb 3>; [100.00%]
<bb 5> :
...
goto <bb 10>; [INV]
<bb 6> : <= second latch bb
<bb 7> : <= second loop
...
goto <bb 6>; [INV]
else
goto <bb 8>; [INV]
<bb 4> and <bb 6> are created by split_edge->split_edge_bb_loc, <bb 4>
is located after the loop, but <bb 6> is located before the loop.
First call of split_edge_bb_loc, the dest_prev is <bb 2>, and find_edge
did find a edge from <bb 2> to <bb 3>, the returned afte_bb is <bb 3>, so
latch <bb 4> is put after the loop
but second call of split_edge_bb_loc, the dest_prev is <bb 5>, so find_edge
return 0, and the returned after_bb is <bb 5>, then the created latch <bb 6>
is put before the loop...
Different latch bb position caused different gcno, while gcov has poor
information and not that smart to recognize it:(, is it reasonable to keep
this kind of loops same order?
small.gcno: 648: block 2:`small.c':1, 3, 4, 6
small.gcno: 688: 01450000: 36:LINES
small.gcno: 700: block 3:`small.c':8, 9
small.gcno: 732: 01450000: 32:LINES
small.gcno: 744: block 5:`small.c':10
-small.gcno: 772: 01450000: 32:LINES
-small.gcno: 784: block 6:`small.c':12
-small.gcno: 812: 01450000: 36:LINES
-small.gcno: 824: block 7:`small.c':12, 13
+small.gcno: 772: 01450000: 36:LINES
+small.gcno: 784: block 6:`small.c':12, 13
+small.gcno: 816: 01450000: 32:LINES
+small.gcno: 828: block 8:`small.c':14
small.gcno: 856: 01450000: 32:LINES
-small.gcno: 868: block 8:`small.c':14
-small.gcno: 896: 01450000: 32:LINES
-small.gcno: 908: block 9:`small.c':17
+small.gcno: 868: block 9:`small.c':17
>
> For the case when both edge_in->src has more than one successor and
> edge_in->dest has more than one predecessor there isn't any good heuristic
> to make printing the blocks in chain order "nice" (well, the backedge
> one maybe).
>
> But as said - this order shouldn't have any effect on semantics ...
>
>> return edge_in->src;
>> }
>> return dest_prev;
>> --
>> 2.27.0
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-02 10:22 ` Xionghu Luo
@ 2023-03-02 10:45 ` Richard Biener
2023-03-06 7:22 ` Xionghu Luo
0 siblings, 1 reply; 19+ messages in thread
From: Richard Biener @ 2023-03-02 10:45 UTC (permalink / raw)
To: Xionghu Luo; +Cc: Xionghu Luo, gcc-patches, luoxhu, rguenther, hubicka
On Thu, Mar 2, 2023 at 11:22 AM Xionghu Luo <yinyuefengyi@gmail.com> wrote:
>
>
>
> On 2023/3/2 16:41, Richard Biener wrote:
> > On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> When spliting edge with self loop, the split edge should be placed just next to
> >> the edge_in->src, otherwise it may generate different position latch bbs for
> >> two consecutive self loops. For details, please refer to:
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93680#c4
> >>
> >> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
> >> master?
> >>
> >> gcc/ChangeLog:
> >>
> >> PR gcov/93680
> >> * tree-cfg.cc (split_edge_bb_loc): Return edge_in->src for self loop.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> PR gcov/93680
> >> * gcc.misc-tests/gcov-pr93680.c: New test.
> >>
> >> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
> >> ---
> >> gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 +++++++++++++++++++++
> >> gcc/tree-cfg.cc | 2 +-
> >> 2 files changed, 25 insertions(+), 1 deletion(-)
> >> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> >>
> >> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> >> new file mode 100644
> >> index 00000000000..b2bf9e626fc
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> >> @@ -0,0 +1,24 @@
> >> +/* { dg-options "-fprofile-arcs -ftest-coverage" } */
> >> +/* { dg-do run { target native } } */
> >> +
> >> +int f(int s, int n)
> >> +{
> >> + int p = 0;
> >> +
> >> + switch (s)
> >> + {
> >> + case 0: /* count(5) */
> >> + do { p++; } while (--n); /* count(5) */
> >> + return p; /* count(1) */
> >> +
> >> + case 1: /* count(5) */
> >> + do { p++; } while (--n); /* count(5) */
> >> + return p; /* count(1) */
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +int main() { f(0, 5); f(1, 5); return 0; }
> >> +
> >> +/* { dg-final { run-gcov gcov-pr93680.c } } */
> >> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> >> index a9fcc7fd050..6fa1d83d366 100644
> >> --- a/gcc/tree-cfg.cc
> >> +++ b/gcc/tree-cfg.cc
> >> @@ -3009,7 +3009,7 @@ split_edge_bb_loc (edge edge_in)
> >> if (dest_prev)
> >> {
> >> edge e = find_edge (dest_prev, dest);
> >> - if (e && !(e->flags & EDGE_COMPLEX))
> >> + if ((e && !(e->flags & EDGE_COMPLEX)) || edge_in->src == edge_in->dest)
> >
> > I think this should eventually apply to all backedge edge_in, correct?
> > But of course
> > we cannot easily test for this here.
> >
> > Still since this affects ordering in the {next,prev}_bb chain only but not CFG
> > semantics I wonder how it can affect coverage? Isn't it only by chance that
> > this block order survives?
>
> For case:
>
> 1 int f(int s, int n)
> 2 {
> 3 int p = 0;
> 4 int q = 0;
> 5
> 6 switch (s)
> 7 {
> 8 case 0:
> 9 do { p++; } while (--n);
> 10 return p;
> 11
> 12 case 1:
> 13 do { p++; } while (--n);
> 14 return p;
> 15 }
> 16
> 17 return 0;
> 18 }
> 19
> 20 int main() { f(0, 5); f(1, 5);}
>
>
> current GCC generates:
>
> <bb 2> :
> ...
>
> <bb 3> : <= first loop
> ...
> goto <bb 4>; [INV]
> else
> goto <bb 5>; [INV]
>
> <bb 4> : <= first latch bb
> goto <bb 3>; [100.00%]
>
> <bb 5> :
> ...
> goto <bb 10>; [INV]
>
> <bb 6> : <= second latch bb
>
> <bb 7> : <= second loop
> ...
> goto <bb 6>; [INV]
> else
> goto <bb 8>; [INV]
>
>
> <bb 4> and <bb 6> are created by split_edge->split_edge_bb_loc, <bb 4>
> is located after the loop, but <bb 6> is located before the loop.
>
> First call of split_edge_bb_loc, the dest_prev is <bb 2>, and find_edge
> did find a edge from <bb 2> to <bb 3>, the returned afte_bb is <bb 3>, so
> latch <bb 4> is put after the loop
>
> but second call of split_edge_bb_loc, the dest_prev is <bb 5>, so find_edge
> return 0, and the returned after_bb is <bb 5>, then the created latch <bb 6>
> is put before the loop...
>
> Different latch bb position caused different gcno, while gcov has poor
> information and not that smart to recognize it:(, is it reasonable to keep
> this kind of loops same order?
>
>
> small.gcno: 648: block 2:`small.c':1, 3, 4, 6
> small.gcno: 688: 01450000: 36:LINES
> small.gcno: 700: block 3:`small.c':8, 9
> small.gcno: 732: 01450000: 32:LINES
> small.gcno: 744: block 5:`small.c':10
> -small.gcno: 772: 01450000: 32:LINES
> -small.gcno: 784: block 6:`small.c':12
> -small.gcno: 812: 01450000: 36:LINES
> -small.gcno: 824: block 7:`small.c':12, 13
> +small.gcno: 772: 01450000: 36:LINES
> +small.gcno: 784: block 6:`small.c':12, 13
> +small.gcno: 816: 01450000: 32:LINES
> +small.gcno: 828: block 8:`small.c':14
> small.gcno: 856: 01450000: 32:LINES
> -small.gcno: 868: block 8:`small.c':14
> -small.gcno: 896: 01450000: 32:LINES
> -small.gcno: 908: block 9:`small.c':17
> +small.gcno: 868: block 9:`small.c':17
Looking at the CFG and the instrumentation shows
<bb 2> :
PROF_edge_counter_17 = __gcov0.f[0];
PROF_edge_counter_18 = PROF_edge_counter_17 + 1;
__gcov0.f[0] = PROF_edge_counter_18;
[t.c:3:7] p_6 = 0;
[t.c:5:3] switch (s_7(D)) <default: <L6> [INV], [t.c:7:5] case 0:
<L0> [INV], [t.c:11:5] case 1: <L3> [INV]>
<bb 3> :
# n_1 = PHI <n_8(D)(2), [t.c:8:28] n_13(4)>
# p_3 = PHI <[t.c:3:7] p_6(2), [t.c:8:15] p_12(4)>
[t.c:7:5] <L0>:
[t.c:8:15] p_12 = p_3 + 1;
[t.c:8:28] n_13 = n_1 + -1;
[t.c:8:28] if (n_13 != 0)
goto <bb 4>; [INV]
else
goto <bb 5>; [INV]
<bb 4> :
PROF_edge_counter_21 = __gcov0.f[2];
PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
__gcov0.f[2] = PROF_edge_counter_22;
[t.c:7:5] goto <bb 3>; [100.00%]
<bb 5> :
PROF_edge_counter_23 = __gcov0.f[3];
PROF_edge_counter_24 = PROF_edge_counter_23 + 1;
__gcov0.f[3] = PROF_edge_counter_24;
[t.c:9:16] _14 = p_12;
[t.c:9:16] goto <bb 10>; [INV]
so the reason this goes wrong is that gcov associates the "wrong"
counter with the block containing
the 'case' label(s), for the case 0 it should have chosen the counter
from bb5 but it likely
computed the count of bb3?
It might be that ordering blocks differently puts the instrumentation
to different blocks or it
makes gcovs association chose different blocks but that means it's
just luck and not fixing
the actual issue?
To me it looks like the correct thing to investigate is switch
statement and/or case label
handling. One can also see that <L0> having line number 7 is wrong to
the extent that
the position of the label doesn't match the number of times it
executes in the source. So
placement of the label is wrong here, possibly caused by CFG cleanup
after CFG build
(but generally labels are not used for anything once the CFG is built
and coverage
instrumentation is late so it might fail due to us moving labels). It
might be OK to
avoid moving labels for --coverage but then coverage should possibly
look at edges
rather than labels?
Richard.
>
>
> >
> > For the case when both edge_in->src has more than one successor and
> > edge_in->dest has more than one predecessor there isn't any good heuristic
> > to make printing the blocks in chain order "nice" (well, the backedge
> > one maybe).
> >
> > But as said - this order shouldn't have any effect on semantics ...
> >
> >> return edge_in->src;
> >> }
> >> return dest_prev;
> >> --
> >> 2.27.0
> >>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-02 10:45 ` Richard Biener
@ 2023-03-06 7:22 ` Xionghu Luo
2023-03-06 8:11 ` Richard Biener
0 siblings, 1 reply; 19+ messages in thread
From: Xionghu Luo @ 2023-03-06 7:22 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, luoxhu, rguenther, hubicka
On 2023/3/2 18:45, Richard Biener wrote:
>>
>>
>> small.gcno: 648: block 2:`small.c':1, 3, 4, 6
>> small.gcno: 688: 01450000: 36:LINES
>> small.gcno: 700: block 3:`small.c':8, 9
>> small.gcno: 732: 01450000: 32:LINES
>> small.gcno: 744: block 5:`small.c':10
>> -small.gcno: 772: 01450000: 32:LINES
>> -small.gcno: 784: block 6:`small.c':12
>> -small.gcno: 812: 01450000: 36:LINES
>> -small.gcno: 824: block 7:`small.c':12, 13
>> +small.gcno: 772: 01450000: 36:LINES
>> +small.gcno: 784: block 6:`small.c':12, 13
>> +small.gcno: 816: 01450000: 32:LINES
>> +small.gcno: 828: block 8:`small.c':14
>> small.gcno: 856: 01450000: 32:LINES
>> -small.gcno: 868: block 8:`small.c':14
>> -small.gcno: 896: 01450000: 32:LINES
>> -small.gcno: 908: block 9:`small.c':17
>> +small.gcno: 868: block 9:`small.c':17
>
> Looking at the CFG and the instrumentation shows
>
> <bb 2> :
> PROF_edge_counter_17 = __gcov0.f[0];
> PROF_edge_counter_18 = PROF_edge_counter_17 + 1;
> __gcov0.f[0] = PROF_edge_counter_18;
> [t.c:3:7] p_6 = 0;
> [t.c:5:3] switch (s_7(D)) <default: <L6> [INV], [t.c:7:5] case 0:
> <L0> [INV], [t.c:11:5] case 1: <L3> [INV]>
>
> <bb 3> :
> # n_1 = PHI <n_8(D)(2), [t.c:8:28] n_13(4)>
> # p_3 = PHI <[t.c:3:7] p_6(2), [t.c:8:15] p_12(4)>
> [t.c:7:5] <L0>:
> [t.c:8:15] p_12 = p_3 + 1;
> [t.c:8:28] n_13 = n_1 + -1;
> [t.c:8:28] if (n_13 != 0)
> goto <bb 4>; [INV]
> else
> goto <bb 5>; [INV]
>
> <bb 4> :
> PROF_edge_counter_21 = __gcov0.f[2];
> PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
> __gcov0.f[2] = PROF_edge_counter_22;
> [t.c:7:5] goto <bb 3>; [100.00%]
>
> <bb 5> :
> PROF_edge_counter_23 = __gcov0.f[3];
> PROF_edge_counter_24 = PROF_edge_counter_23 + 1;
> __gcov0.f[3] = PROF_edge_counter_24;
> [t.c:9:16] _14 = p_12;
> [t.c:9:16] goto <bb 10>; [INV]
>
> so the reason this goes wrong is that gcov associates the "wrong"
> counter with the block containing
> the 'case' label(s), for the case 0 it should have chosen the counter
> from bb5 but it likely
> computed the count of bb3?
>
> It might be that ordering blocks differently puts the instrumentation
> to different blocks or it
> makes gcovs association chose different blocks but that means it's
> just luck and not fixing
> the actual issue?
>
> To me it looks like the correct thing to investigate is switch
> statement and/or case label
> handling. One can also see that <L0> having line number 7 is wrong to
> the extent that
> the position of the label doesn't match the number of times it
> executes in the source. So
> placement of the label is wrong here, possibly caused by CFG cleanup
> after CFG build
> (but generally labels are not used for anything once the CFG is built
> and coverage
> instrumentation is late so it might fail due to us moving labels). It
> might be OK to
> avoid moving labels for --coverage but then coverage should possibly
> look at edges
> rather than labels?
>
Thanks, I investigated the Labels, it seems wrong at the beginning from
.gimple to .cfg very early quite like PR90574:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90574
.gimple:
int f (int s, int n)
[small.c:2:1] {
int D.2755;
int p;
[small.c:3:7] p = 0;
[small.c:5:3] switch (s) <default: <D.2756>, [small.c:7:5] case 0: <D.2743>, [small.c:11:5] case 1: <D.2744>>
[small.c:7:5] <D.2743>: <= case label
<D.2748>: <= loop label
[small.c:8:13] p = p + 1;
[small.c:8:26] n = n + -1;
[small.c:8:26] if (n != 0) goto <D.2748>; else goto <D.2746>;
<D.2746>:
[small.c:9:14] D.2755 = p;
[small.c:9:14] return D.2755;
[small.c:11:5] <D.2744>:
<D.2751>:
[small.c:12:13] p = p + 1;
[small.c:12:26] n = n + -1;
[small.c:12:26] if (n != 0) goto <D.2751>; else goto <D.2749>;
<D.2749>:
[small.c:13:14] D.2755 = p;
[small.c:13:14] return D.2755;
<D.2756>:
[small.c:16:10] D.2755 = 0;
[small.c:16:10] return D.2755;
}
.cfg:
int f (int s, int n)
{
int p;
int D.2755;
<bb 2> :
[small.c:3:7] p = 0;
[small.c:5:3] switch (s) <default: <L6> [INV], [small.c:7:5] case 0: <L0> [INV], [small.c:11:5] case 1: <L3> [INV]>
<bb 3> :
[small.c:7:5] <L0>: <= case 0
[small.c:8:13 discrim 1] p = p + 1;
[small.c:8:26 discrim 1] n = n + -1;
[small.c:8:26 discrim 1] if (n != 0)
goto <bb 3>; [INV]
else
goto <bb 4>; [INV]
<bb 4> :
[small.c:9:14] D.2755 = p;
[small.c:9:14] goto <bb 8>; [INV]
<bb 5> :
[small.c:11:5] <L3>: <= case 1
[small.c:12:13 discrim 1] p = p + 1;
[small.c:12:26 discrim 1] n = n + -1;
[small.c:12:26 discrim 1] if (n != 0)
goto <bb 5>; [INV]
else
goto <bb 6>; [INV]
The labels are merged into the loop unexpected, so I tried below fix
for --coverage if two labels are not on same line to start new basic block:
index 10ca86714f4..b788198ac31 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -2860,6 +2860,13 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
|| !DECL_ARTIFICIAL (gimple_label_label (plabel)))
return true;
+ location_t loc_prev = gimple_location (plabel);
+ location_t locus = gimple_location (label_stmt);
+ expanded_location locus_e = expand_location (locus);
+
+ if (flag_test_coverage && !same_line_p (locus, &locus_e, loc_prev))
+ return true;
+
cfg_stats.num_merged_labels++;
return false;
}
.profile:
<bb 2> :
PROF_edge_counter_17 = __gcov0.f[0];
PROF_edge_counter_18 = PROF_edge_counter_17 + 1;
__gcov0.f[0] = PROF_edge_counter_18;
[small.c:3:7] p_6 = 0;
[small.c:5:3] switch (s_7(D)) <default: <L6> [INV], [small.c:7:5] case 0: <L0> [INV], [small.c:11:5] case 1: <L3> [INV]>
<bb 3> :
[small.c:7:5] <L0>: <= case 0
PROF_edge_counter_19 = __gcov0.f[1];
PROF_edge_counter_20 = PROF_edge_counter_19 + 1;
__gcov0.f[1] = PROF_edge_counter_20;
<bb 4> :
# n_1 = PHI <n_8(D)(3), [small.c:8:26 discrim 1] n_13(5)>
# p_3 = PHI <[small.c:3:7] p_6(3), [small.c:8:13 discrim 1] p_12(5)>
[small.c:8:13 discrim 1] p_12 = p_3 + 1;
[small.c:8:26 discrim 1] n_13 = n_1 + -1;
[small.c:8:26 discrim 1] if (n_13 != 0)
goto <bb 5>; [INV]
else
goto <bb 6>; [INV]
<bb 5> :
PROF_edge_counter_23 = __gcov0.f[3];
PROF_edge_counter_24 = PROF_edge_counter_23 + 1;
__gcov0.f[3] = PROF_edge_counter_24;
goto <bb 4>; [100.00%]
<bb 6> :
[small.c:9:14] _14 = p_12;
[small.c:9:14] goto <bb 12>; [INV]
<bb 7> :
[small.c:11:5] <L3>: <= case 1
PROF_edge_counter_21 = __gcov0.f[2];
PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
__gcov0.f[2] = PROF_edge_counter_22;
<bb 8> :
# n_2 = PHI <n_8(D)(7), [small.c:12:26 discrim 1] n_10(9)>
# p_4 = PHI <[small.c:3:7] p_6(7), [small.c:12:13 discrim 1] p_9(9)>
[small.c:12:13 discrim 1] p_9 = p_4 + 1;
[small.c:12:26 discrim 1] n_10 = n_2 + -1;
[small.c:12:26 discrim 1] if (n_10 != 0)
goto <bb 9>; [INV]
else
goto <bb 10>; [INV]
<bb 9> :
PROF_edge_counter_25 = __gcov0.f[4];
PROF_edge_counter_26 = PROF_edge_counter_25 + 1;
__gcov0.f[4] = PROF_edge_counter_26;
goto <bb 8>; [100.00%]
then label lines are also correct now:
.c.gcov:
Lines executed:90.91% of 11
-: 0:Source:small.c
-: 0:Graph:small.gcno
-: 0:Data:small.gcda
-: 0:Runs:1
2: 1:int f(int s, int n)
-: 2:{
2: 3: int p = 0;
-: 4:
2: 5: switch (s)
-: 6: {
1: 7: case 0:
5: 8: do { p++; } while (--n);
1: 9: return p;
-: 10:
1: 11: case 1:
5: 12: do { p++; } while (--n);
1: 13: return p;
-: 14: }
-: 15:
#####: 16: return 0;
-: 17:}
-: 18:
1: 19:int main() { f(0, 5); f(1, 5);}
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-06 7:22 ` Xionghu Luo
@ 2023-03-06 8:11 ` Richard Biener
2023-03-07 7:41 ` Xionghu Luo
0 siblings, 1 reply; 19+ messages in thread
From: Richard Biener @ 2023-03-06 8:11 UTC (permalink / raw)
To: Xionghu Luo; +Cc: gcc-patches, luoxhu, rguenther, hubicka
On Mon, Mar 6, 2023 at 8:22 AM Xionghu Luo <yinyuefengyi@gmail.com> wrote:
>
>
>
> On 2023/3/2 18:45, Richard Biener wrote:
> >>
> >>
> >> small.gcno: 648: block 2:`small.c':1, 3, 4, 6
> >> small.gcno: 688: 01450000: 36:LINES
> >> small.gcno: 700: block 3:`small.c':8, 9
> >> small.gcno: 732: 01450000: 32:LINES
> >> small.gcno: 744: block 5:`small.c':10
> >> -small.gcno: 772: 01450000: 32:LINES
> >> -small.gcno: 784: block 6:`small.c':12
> >> -small.gcno: 812: 01450000: 36:LINES
> >> -small.gcno: 824: block 7:`small.c':12, 13
> >> +small.gcno: 772: 01450000: 36:LINES
> >> +small.gcno: 784: block 6:`small.c':12, 13
> >> +small.gcno: 816: 01450000: 32:LINES
> >> +small.gcno: 828: block 8:`small.c':14
> >> small.gcno: 856: 01450000: 32:LINES
> >> -small.gcno: 868: block 8:`small.c':14
> >> -small.gcno: 896: 01450000: 32:LINES
> >> -small.gcno: 908: block 9:`small.c':17
> >> +small.gcno: 868: block 9:`small.c':17
> >
> > Looking at the CFG and the instrumentation shows
> >
> > <bb 2> :
> > PROF_edge_counter_17 = __gcov0.f[0];
> > PROF_edge_counter_18 = PROF_edge_counter_17 + 1;
> > __gcov0.f[0] = PROF_edge_counter_18;
> > [t.c:3:7] p_6 = 0;
> > [t.c:5:3] switch (s_7(D)) <default: <L6> [INV], [t.c:7:5] case 0:
> > <L0> [INV], [t.c:11:5] case 1: <L3> [INV]>
> >
> > <bb 3> :
> > # n_1 = PHI <n_8(D)(2), [t.c:8:28] n_13(4)>
> > # p_3 = PHI <[t.c:3:7] p_6(2), [t.c:8:15] p_12(4)>
> > [t.c:7:5] <L0>:
> > [t.c:8:15] p_12 = p_3 + 1;
> > [t.c:8:28] n_13 = n_1 + -1;
> > [t.c:8:28] if (n_13 != 0)
> > goto <bb 4>; [INV]
> > else
> > goto <bb 5>; [INV]
> >
> > <bb 4> :
> > PROF_edge_counter_21 = __gcov0.f[2];
> > PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
> > __gcov0.f[2] = PROF_edge_counter_22;
> > [t.c:7:5] goto <bb 3>; [100.00%]
> >
> > <bb 5> :
> > PROF_edge_counter_23 = __gcov0.f[3];
> > PROF_edge_counter_24 = PROF_edge_counter_23 + 1;
> > __gcov0.f[3] = PROF_edge_counter_24;
> > [t.c:9:16] _14 = p_12;
> > [t.c:9:16] goto <bb 10>; [INV]
> >
> > so the reason this goes wrong is that gcov associates the "wrong"
> > counter with the block containing
> > the 'case' label(s), for the case 0 it should have chosen the counter
> > from bb5 but it likely
> > computed the count of bb3?
> >
> > It might be that ordering blocks differently puts the instrumentation
> > to different blocks or it
> > makes gcovs association chose different blocks but that means it's
> > just luck and not fixing
> > the actual issue?
> >
> > To me it looks like the correct thing to investigate is switch
> > statement and/or case label
> > handling. One can also see that <L0> having line number 7 is wrong to
> > the extent that
> > the position of the label doesn't match the number of times it
> > executes in the source. So
> > placement of the label is wrong here, possibly caused by CFG cleanup
> > after CFG build
> > (but generally labels are not used for anything once the CFG is built
> > and coverage
> > instrumentation is late so it might fail due to us moving labels). It
> > might be OK to
> > avoid moving labels for --coverage but then coverage should possibly
> > look at edges
> > rather than labels?
> >
>
> Thanks, I investigated the Labels, it seems wrong at the beginning from
> .gimple to .cfg very early quite like PR90574:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90574
>
> .gimple:
>
> int f (int s, int n)
> [small.c:2:1] {
> int D.2755;
> int p;
>
> [small.c:3:7] p = 0;
> [small.c:5:3] switch (s) <default: <D.2756>, [small.c:7:5] case 0: <D.2743>, [small.c:11:5] case 1: <D.2744>>
> [small.c:7:5] <D.2743>: <= case label
> <D.2748>: <= loop label
> [small.c:8:13] p = p + 1;
> [small.c:8:26] n = n + -1;
> [small.c:8:26] if (n != 0) goto <D.2748>; else goto <D.2746>;
> <D.2746>:
> [small.c:9:14] D.2755 = p;
> [small.c:9:14] return D.2755;
> [small.c:11:5] <D.2744>:
> <D.2751>:
> [small.c:12:13] p = p + 1;
> [small.c:12:26] n = n + -1;
> [small.c:12:26] if (n != 0) goto <D.2751>; else goto <D.2749>;
> <D.2749>:
> [small.c:13:14] D.2755 = p;
> [small.c:13:14] return D.2755;
> <D.2756>:
> [small.c:16:10] D.2755 = 0;
> [small.c:16:10] return D.2755;
> }
>
> .cfg:
>
> int f (int s, int n)
> {
> int p;
> int D.2755;
>
> <bb 2> :
> [small.c:3:7] p = 0;
> [small.c:5:3] switch (s) <default: <L6> [INV], [small.c:7:5] case 0: <L0> [INV], [small.c:11:5] case 1: <L3> [INV]>
>
> <bb 3> :
> [small.c:7:5] <L0>: <= case 0
> [small.c:8:13 discrim 1] p = p + 1;
> [small.c:8:26 discrim 1] n = n + -1;
> [small.c:8:26 discrim 1] if (n != 0)
> goto <bb 3>; [INV]
> else
> goto <bb 4>; [INV]
>
> <bb 4> :
> [small.c:9:14] D.2755 = p;
> [small.c:9:14] goto <bb 8>; [INV]
>
> <bb 5> :
> [small.c:11:5] <L3>: <= case 1
> [small.c:12:13 discrim 1] p = p + 1;
> [small.c:12:26 discrim 1] n = n + -1;
> [small.c:12:26 discrim 1] if (n != 0)
> goto <bb 5>; [INV]
> else
> goto <bb 6>; [INV]
>
>
> The labels are merged into the loop unexpected, so I tried below fix
> for --coverage if two labels are not on same line to start new basic block:
>
>
> index 10ca86714f4..b788198ac31 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -2860,6 +2860,13 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
> return true;
>
> + location_t loc_prev = gimple_location (plabel);
> + location_t locus = gimple_location (label_stmt);
> + expanded_location locus_e = expand_location (locus);
> +
> + if (flag_test_coverage && !same_line_p (locus, &locus_e, loc_prev))
> + return true;
> +
> cfg_stats.num_merged_labels++;
> return false;
> }
Interesting. Note that in CFG cleanup we have the following condition
when deciding
whether to merge a forwarder block with the destination:
locus = single_succ_edge (bb)->goto_locus;
...
/* Now walk through the statements backward. We can ignore labels,
anything else means this is not a forwarder block. */
for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
{
gimple *stmt = gsi_stmt (gsi);
switch (gimple_code (stmt))
{
case GIMPLE_LABEL:
if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *> (stmt))))
return false;
if (!optimize
&& (gimple_has_location (stmt)
|| LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
&& gimple_location (stmt) != locus)
return false;
it would be nice to sync the behavior between CFG creation and this.
In particular
a missing piece of the puzzle is how CFG creation sets ->goto_locus of the edge
after your change and whether that goto_locus and the label locus
compare matches
your condition (the CFG cleanup one is even stricter but special cases
UNKNOWN_LOCATION).
I also notice the !optimize vs. flag_test_coverage condition mismatch.
That said - I think your change to stmt_starts_bb_p is definitely the
correct place to fix,
I'm wondering how to match up with CFG cleanup - possibly using
!optimize instead
of flag_test_coverage would even make sense for debugging as well - we should be
able to put a breakpoint on the label hitting once rather than once
each loop iteration.
Thanks,
Richard.
> .profile:
>
> <bb 2> :
> PROF_edge_counter_17 = __gcov0.f[0];
> PROF_edge_counter_18 = PROF_edge_counter_17 + 1;
> __gcov0.f[0] = PROF_edge_counter_18;
> [small.c:3:7] p_6 = 0;
> [small.c:5:3] switch (s_7(D)) <default: <L6> [INV], [small.c:7:5] case 0: <L0> [INV], [small.c:11:5] case 1: <L3> [INV]>
>
> <bb 3> :
> [small.c:7:5] <L0>: <= case 0
> PROF_edge_counter_19 = __gcov0.f[1];
> PROF_edge_counter_20 = PROF_edge_counter_19 + 1;
> __gcov0.f[1] = PROF_edge_counter_20;
>
> <bb 4> :
> # n_1 = PHI <n_8(D)(3), [small.c:8:26 discrim 1] n_13(5)>
> # p_3 = PHI <[small.c:3:7] p_6(3), [small.c:8:13 discrim 1] p_12(5)>
> [small.c:8:13 discrim 1] p_12 = p_3 + 1;
> [small.c:8:26 discrim 1] n_13 = n_1 + -1;
> [small.c:8:26 discrim 1] if (n_13 != 0)
> goto <bb 5>; [INV]
> else
> goto <bb 6>; [INV]
>
> <bb 5> :
> PROF_edge_counter_23 = __gcov0.f[3];
> PROF_edge_counter_24 = PROF_edge_counter_23 + 1;
> __gcov0.f[3] = PROF_edge_counter_24;
> goto <bb 4>; [100.00%]
>
> <bb 6> :
> [small.c:9:14] _14 = p_12;
> [small.c:9:14] goto <bb 12>; [INV]
>
> <bb 7> :
> [small.c:11:5] <L3>: <= case 1
> PROF_edge_counter_21 = __gcov0.f[2];
> PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
> __gcov0.f[2] = PROF_edge_counter_22;
>
>
> <bb 8> :
> # n_2 = PHI <n_8(D)(7), [small.c:12:26 discrim 1] n_10(9)>
> # p_4 = PHI <[small.c:3:7] p_6(7), [small.c:12:13 discrim 1] p_9(9)>
> [small.c:12:13 discrim 1] p_9 = p_4 + 1;
> [small.c:12:26 discrim 1] n_10 = n_2 + -1;
> [small.c:12:26 discrim 1] if (n_10 != 0)
> goto <bb 9>; [INV]
> else
> goto <bb 10>; [INV]
>
> <bb 9> :
> PROF_edge_counter_25 = __gcov0.f[4];
> PROF_edge_counter_26 = PROF_edge_counter_25 + 1;
> __gcov0.f[4] = PROF_edge_counter_26;
> goto <bb 8>; [100.00%]
>
>
> then label lines are also correct now:
>
> .c.gcov:
>
> Lines executed:90.91% of 11
> -: 0:Source:small.c
> -: 0:Graph:small.gcno
> -: 0:Data:small.gcda
> -: 0:Runs:1
> 2: 1:int f(int s, int n)
> -: 2:{
> 2: 3: int p = 0;
> -: 4:
> 2: 5: switch (s)
> -: 6: {
> 1: 7: case 0:
> 5: 8: do { p++; } while (--n);
> 1: 9: return p;
> -: 10:
> 1: 11: case 1:
> 5: 12: do { p++; } while (--n);
> 1: 13: return p;
> -: 14: }
> -: 15:
> #####: 16: return 0;
> -: 17:}
> -: 18:
> 1: 19:int main() { f(0, 5); f(1, 5);}
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-06 8:11 ` Richard Biener
@ 2023-03-07 7:41 ` Xionghu Luo
2023-03-07 8:53 ` Richard Biener
0 siblings, 1 reply; 19+ messages in thread
From: Xionghu Luo @ 2023-03-07 7:41 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, luoxhu, rguenther, hubicka
[-- Attachment #1: Type: text/plain, Size: 10338 bytes --]
On 2023/3/6 16:11, Richard Biener wrote:
> On Mon, Mar 6, 2023 at 8:22 AM Xionghu Luo <yinyuefengyi@gmail.com> wrote:
>>
>>
>>
>> On 2023/3/2 18:45, Richard Biener wrote:
>>>>
>>>>
>>>> small.gcno: 648: block 2:`small.c':1, 3, 4, 6
>>>> small.gcno: 688: 01450000: 36:LINES
>>>> small.gcno: 700: block 3:`small.c':8, 9
>>>> small.gcno: 732: 01450000: 32:LINES
>>>> small.gcno: 744: block 5:`small.c':10
>>>> -small.gcno: 772: 01450000: 32:LINES
>>>> -small.gcno: 784: block 6:`small.c':12
>>>> -small.gcno: 812: 01450000: 36:LINES
>>>> -small.gcno: 824: block 7:`small.c':12, 13
>>>> +small.gcno: 772: 01450000: 36:LINES
>>>> +small.gcno: 784: block 6:`small.c':12, 13
>>>> +small.gcno: 816: 01450000: 32:LINES
>>>> +small.gcno: 828: block 8:`small.c':14
>>>> small.gcno: 856: 01450000: 32:LINES
>>>> -small.gcno: 868: block 8:`small.c':14
>>>> -small.gcno: 896: 01450000: 32:LINES
>>>> -small.gcno: 908: block 9:`small.c':17
>>>> +small.gcno: 868: block 9:`small.c':17
>>>
>>> Looking at the CFG and the instrumentation shows
>>>
>>> <bb 2> :
>>> PROF_edge_counter_17 = __gcov0.f[0];
>>> PROF_edge_counter_18 = PROF_edge_counter_17 + 1;
>>> __gcov0.f[0] = PROF_edge_counter_18;
>>> [t.c:3:7] p_6 = 0;
>>> [t.c:5:3] switch (s_7(D)) <default: <L6> [INV], [t.c:7:5] case 0:
>>> <L0> [INV], [t.c:11:5] case 1: <L3> [INV]>
>>>
>>> <bb 3> :
>>> # n_1 = PHI <n_8(D)(2), [t.c:8:28] n_13(4)>
>>> # p_3 = PHI <[t.c:3:7] p_6(2), [t.c:8:15] p_12(4)>
>>> [t.c:7:5] <L0>:
>>> [t.c:8:15] p_12 = p_3 + 1;
>>> [t.c:8:28] n_13 = n_1 + -1;
>>> [t.c:8:28] if (n_13 != 0)
>>> goto <bb 4>; [INV]
>>> else
>>> goto <bb 5>; [INV]
>>>
>>> <bb 4> :
>>> PROF_edge_counter_21 = __gcov0.f[2];
>>> PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
>>> __gcov0.f[2] = PROF_edge_counter_22;
>>> [t.c:7:5] goto <bb 3>; [100.00%]
>>>
>>> <bb 5> :
>>> PROF_edge_counter_23 = __gcov0.f[3];
>>> PROF_edge_counter_24 = PROF_edge_counter_23 + 1;
>>> __gcov0.f[3] = PROF_edge_counter_24;
>>> [t.c:9:16] _14 = p_12;
>>> [t.c:9:16] goto <bb 10>; [INV]
>>>
>>> so the reason this goes wrong is that gcov associates the "wrong"
>>> counter with the block containing
>>> the 'case' label(s), for the case 0 it should have chosen the counter
>>> from bb5 but it likely
>>> computed the count of bb3?
>>>
>>> It might be that ordering blocks differently puts the instrumentation
>>> to different blocks or it
>>> makes gcovs association chose different blocks but that means it's
>>> just luck and not fixing
>>> the actual issue?
>>>
>>> To me it looks like the correct thing to investigate is switch
>>> statement and/or case label
>>> handling. One can also see that <L0> having line number 7 is wrong to
>>> the extent that
>>> the position of the label doesn't match the number of times it
>>> executes in the source. So
>>> placement of the label is wrong here, possibly caused by CFG cleanup
>>> after CFG build
>>> (but generally labels are not used for anything once the CFG is built
>>> and coverage
>>> instrumentation is late so it might fail due to us moving labels). It
>>> might be OK to
>>> avoid moving labels for --coverage but then coverage should possibly
>>> look at edges
>>> rather than labels?
>>>
>>
>> Thanks, I investigated the Labels, it seems wrong at the beginning from
>> .gimple to .cfg very early quite like PR90574:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90574
>>
>> .gimple:
>>
>> int f (int s, int n)
>> [small.c:2:1] {
>> int D.2755;
>> int p;
>>
>> [small.c:3:7] p = 0;
>> [small.c:5:3] switch (s) <default: <D.2756>, [small.c:7:5] case 0: <D.2743>, [small.c:11:5] case 1: <D.2744>>
>> [small.c:7:5] <D.2743>: <= case label
>> <D.2748>: <= loop label
>> [small.c:8:13] p = p + 1;
>> [small.c:8:26] n = n + -1;
>> [small.c:8:26] if (n != 0) goto <D.2748>; else goto <D.2746>;
>> <D.2746>:
>> [small.c:9:14] D.2755 = p;
>> [small.c:9:14] return D.2755;
>> [small.c:11:5] <D.2744>:
>> <D.2751>:
>> [small.c:12:13] p = p + 1;
>> [small.c:12:26] n = n + -1;
>> [small.c:12:26] if (n != 0) goto <D.2751>; else goto <D.2749>;
>> <D.2749>:
>> [small.c:13:14] D.2755 = p;
>> [small.c:13:14] return D.2755;
>> <D.2756>:
>> [small.c:16:10] D.2755 = 0;
>> [small.c:16:10] return D.2755;
>> }
>>
>> .cfg:
>>
>> int f (int s, int n)
>> {
>> int p;
>> int D.2755;
>>
>> <bb 2> :
>> [small.c:3:7] p = 0;
>> [small.c:5:3] switch (s) <default: <L6> [INV], [small.c:7:5] case 0: <L0> [INV], [small.c:11:5] case 1: <L3> [INV]>
>>
>> <bb 3> :
>> [small.c:7:5] <L0>: <= case 0
>> [small.c:8:13 discrim 1] p = p + 1;
>> [small.c:8:26 discrim 1] n = n + -1;
>> [small.c:8:26 discrim 1] if (n != 0)
>> goto <bb 3>; [INV]
>> else
>> goto <bb 4>; [INV]
>>
>> <bb 4> :
>> [small.c:9:14] D.2755 = p;
>> [small.c:9:14] goto <bb 8>; [INV]
>>
>> <bb 5> :
>> [small.c:11:5] <L3>: <= case 1
>> [small.c:12:13 discrim 1] p = p + 1;
>> [small.c:12:26 discrim 1] n = n + -1;
>> [small.c:12:26 discrim 1] if (n != 0)
>> goto <bb 5>; [INV]
>> else
>> goto <bb 6>; [INV]
>>
>>
>> The labels are merged into the loop unexpected, so I tried below fix
>> for --coverage if two labels are not on same line to start new basic block:
>>
>>
>> index 10ca86714f4..b788198ac31 100644
>> --- a/gcc/tree-cfg.cc
>> +++ b/gcc/tree-cfg.cc
>> @@ -2860,6 +2860,13 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
>> || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
>> return true;
>>
>> + location_t loc_prev = gimple_location (plabel);
>> + location_t locus = gimple_location (label_stmt);
>> + expanded_location locus_e = expand_location (locus);
>> +
>> + if (flag_test_coverage && !same_line_p (locus, &locus_e, loc_prev))
>> + return true;
>> +
>> cfg_stats.num_merged_labels++;
>> return false;
>> }
>
> Interesting. Note that in CFG cleanup we have the following condition
> when deciding
> whether to merge a forwarder block with the destination:
>
> locus = single_succ_edge (bb)->goto_locus;
> ...
> /* Now walk through the statements backward. We can ignore labels,
> anything else means this is not a forwarder block. */
> for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
> {
> gimple *stmt = gsi_stmt (gsi);
>
> switch (gimple_code (stmt))
> {
> case GIMPLE_LABEL:
> if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *> (stmt))))
> return false;
> if (!optimize
> && (gimple_has_location (stmt)
> || LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
> && gimple_location (stmt) != locus)
> return false;
>
> it would be nice to sync the behavior between CFG creation and this.
> In particular
> a missing piece of the puzzle is how CFG creation sets ->goto_locus of the edge
> after your change and whether that goto_locus and the label locus
> compare matches
> your condition (the CFG cleanup one is even stricter but special cases
> UNKNOWN_LOCATION).
>
> I also notice the !optimize vs. flag_test_coverage condition mismatch.
>
> That said - I think your change to stmt_starts_bb_p is definitely the
> correct place to fix,
> I'm wondering how to match up with CFG cleanup - possibly using
> !optimize instead
> of flag_test_coverage would even make sense for debugging as well - we should be
> able to put a breakpoint on the label hitting once rather than once
> each loop iteration.
>
Unfortunately this change (flag_test_coverage -> !optimize ) caused hundred
of gfortran cases execution failure with O0. Take gfortran.dg/index.f90 for
example:
.gimple:
__attribute__((fn spec (". ")))
void p ()
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:6:9] {
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:13:28] L.1:
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:14:28] L.2:
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:15:28] L.3:
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:16:28] L.4:
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:17:28] L.5:
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:18:72] L.6:
}
.cfg:
...
Removing basic block 7
;; basic block 7, loop depth 0
;; pred:
return;
;; succ: EXIT
;; 1 loops found
;;
;; Loop 0
;; header 0, latch 1
;; depth 0, outer -1
;; nodes: 0 1 2
;; 2 succs { }
__attribute__((fn spec (". ")))
void p ()
{
<bb 2> :
}
Due to the "return;" is removed in bb 7.
actually in build_gimple_cfg, cleanup_dead_labels will remove all labels L.1 to L.6
first, then make_edges fail to create edges for <bb 2> to <bb 7> due to they are all
EMPTY bb in make_edges_bb...
240│ /* To speed up statement iterator walks, we first purge dead labels. */
241│ cleanup_dead_labels ();
242│
243│ /* Group case nodes to reduce the number of edges.
244│ We do this after cleaning up dead labels because otherwise we miss
245│ a lot of obvious case merging opportunities. */
246│ group_case_labels ();
247│
248│ /* Create the edges of the flowgraph. */
249│ discriminator_per_locus = new hash_table<locus_discrim_hasher> (13);
250├> make_edges ();
<bb 0> :
<bb 2> :
<bb 3> :
<bb 4> :
<bb 5> :
<bb 6> :
<bb 7> :
return;
<bb 1> :
Seems deadlock here as you said to set goto_locus as labels are removed before
edges are created, the case could pass if I comment out the function cleanup_dead_labels(),
so also not call it when !optimize?
if (!!optimize)
cleanup_dead_labels ();
Attached v2 patch could pass regression test on x86_64-linux-gnu/aarch64-linux-gnu.
[-- Attachment #2: v2-0001-gcov-Fix-do-while-structure-in-case-statement-lea.patch --]
[-- Type: text/plain, Size: 4927 bytes --]
From 575f845cbfc15b250f3debf2e2c99f95584e7afa Mon Sep 17 00:00:00 2001
From: Xionghu Luo <xionghuluo@tencent.com>
Date: Tue, 28 Feb 2023 17:46:18 +0800
Subject: [PATCH v2] gcov: Fix "do-while" structure in case statement leads to
incorrect code coverage [PR93680]
Start a new basic block if two labels have different location when
test-coverage.
Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
master?
gcc/ChangeLog:
PR gcov/93680
* tree-cfg.cc (build_gimple_cfg): Don't delete labels if
!optimize.
(stmt_starts_bb_p): Check whether two labels are on same line.
gcc/testsuite/ChangeLog:
PR gcov/93680
* g++.dg/gcov/gcov-1.C: Correct counts.
* gcc.misc-tests/gcov-4.c: Likewise.
* gcc.misc-tests/gcov-pr85332.c: Likewise.
* lib/gcov.exp: Also clean gcda if fail.
* gcc.misc-tests/gcov-pr93680.c: New test.
Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
---
gcc/testsuite/g++.dg/gcov/gcov-1.C | 2 +-
gcc/testsuite/gcc.misc-tests/gcov-4.c | 2 +-
gcc/testsuite/gcc.misc-tests/gcov-pr85332.c | 2 +-
gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 +++++++++++++++++++++
gcc/testsuite/lib/gcov.exp | 4 +---
gcc/tree-cfg.cc | 10 ++++++++-
6 files changed, 37 insertions(+), 7 deletions(-)
create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
index ee383b480a8..01e7084fb03 100644
--- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
+++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
@@ -263,7 +263,7 @@ test_switch (int i, int j)
case 2:
result = do_something (1024);
break;
- case 3: /* count(3) */
+ case 3: /* count(2) */
case 4:
/* branch(67) */
if (j == 2) /* count(3) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
index da7929ef7fc..792cda8cfce 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
@@ -122,7 +122,7 @@ top:
}
else
{
-else_: /* count(1) */
+else_: /* count(2) */
j = do_something (j); /* count(2) */
if (j) /* count(2) */
{
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
index 73e50b19fc7..b37e760910c 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
@@ -7,7 +7,7 @@ int doit(int sel, int n, void *p)
switch (sel)
{
- case 0: /* count(3) */
+ case 0: /* count(1) */
do {*p0 += *p0;} while (--n); /* count(3) */
return *p0 == 0; /* count(1) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
new file mode 100644
index 00000000000..2fe340c4011
--- /dev/null
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
@@ -0,0 +1,24 @@
+/* { dg-options "-fprofile-arcs -ftest-coverage" } */
+/* { dg-do run { target native } } */
+
+int f(int s, int n)
+{
+ int p = 0;
+
+ switch (s)
+ {
+ case 0: /* count(1) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+
+ case 1: /* count(1) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+ }
+
+ return 0;
+}
+
+int main() { f(0, 5); f(1, 5); return 0; }
+
+/* { dg-final { run-gcov gcov-pr93680.c } } */
diff --git a/gcc/testsuite/lib/gcov.exp b/gcc/testsuite/lib/gcov.exp
index 80e74aeb220..07e1978d25d 100644
--- a/gcc/testsuite/lib/gcov.exp
+++ b/gcc/testsuite/lib/gcov.exp
@@ -424,9 +424,7 @@ proc run-gcov { args } {
}
if { $tfailed > 0 } {
fail "$testname gcov: $lfailed failures in line counts, $bfailed in branch percentages, $cfailed in return percentages, $ifailed in intermediate format"
- if { $xfailed } {
- clean-gcov $testcase
- }
+ clean-gcov $testcase
} else {
pass "$testname gcov"
clean-gcov $testcase
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index a9fcc7fd050..41a269b5fe2 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -238,7 +238,8 @@ build_gimple_cfg (gimple_seq seq)
n_basic_blocks_for_fn (cfun));
/* To speed up statement iterator walks, we first purge dead labels. */
- cleanup_dead_labels ();
+ if (optimize)
+ cleanup_dead_labels ();
/* Group case nodes to reduce the number of edges.
We do this after cleaning up dead labels because otherwise we miss
@@ -2860,6 +2861,13 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
|| !DECL_ARTIFICIAL (gimple_label_label (plabel)))
return true;
+ location_t loc_prev = gimple_location (plabel);
+ location_t locus = gimple_location (label_stmt);
+ expanded_location locus_e = expand_location (locus);
+
+ if (!optimize && !same_line_p (locus, &locus_e, loc_prev))
+ return true;
+
cfg_stats.num_merged_labels++;
return false;
}
--
2.27.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-07 7:41 ` Xionghu Luo
@ 2023-03-07 8:53 ` Richard Biener
2023-03-07 10:26 ` Xionghu Luo
0 siblings, 1 reply; 19+ messages in thread
From: Richard Biener @ 2023-03-07 8:53 UTC (permalink / raw)
To: Xionghu Luo; +Cc: Richard Biener, gcc-patches, luoxhu, hubicka
[-- Attachment #1: Type: text/plain, Size: 12929 bytes --]
On Tue, 7 Mar 2023, Xionghu Luo wrote:
>
>
> On 2023/3/6 16:11, Richard Biener wrote:
> > On Mon, Mar 6, 2023 at 8:22 AM Xionghu Luo <yinyuefengyi@gmail.com> wrote:
> >>
> >>
> >>
> >> On 2023/3/2 18:45, Richard Biener wrote:
> >>>>
> >>>>
> >>>> small.gcno: 648: block 2:`small.c':1, 3, 4, 6
> >>>> small.gcno: 688: 01450000: 36:LINES
> >>>> small.gcno: 700: block 3:`small.c':8, 9
> >>>> small.gcno: 732: 01450000: 32:LINES
> >>>> small.gcno: 744: block 5:`small.c':10
> >>>> -small.gcno: 772: 01450000: 32:LINES
> >>>> -small.gcno: 784: block 6:`small.c':12
> >>>> -small.gcno: 812: 01450000: 36:LINES
> >>>> -small.gcno: 824: block 7:`small.c':12, 13
> >>>> +small.gcno: 772: 01450000: 36:LINES
> >>>> +small.gcno: 784: block 6:`small.c':12, 13
> >>>> +small.gcno: 816: 01450000: 32:LINES
> >>>> +small.gcno: 828: block 8:`small.c':14
> >>>> small.gcno: 856: 01450000: 32:LINES
> >>>> -small.gcno: 868: block 8:`small.c':14
> >>>> -small.gcno: 896: 01450000: 32:LINES
> >>>> -small.gcno: 908: block 9:`small.c':17
> >>>> +small.gcno: 868: block 9:`small.c':17
> >>>
> >>> Looking at the CFG and the instrumentation shows
> >>>
> >>> <bb 2> :
> >>> PROF_edge_counter_17 = __gcov0.f[0];
> >>> PROF_edge_counter_18 = PROF_edge_counter_17 + 1;
> >>> __gcov0.f[0] = PROF_edge_counter_18;
> >>> [t.c:3:7] p_6 = 0;
> >>> [t.c:5:3] switch (s_7(D)) <default: <L6> [INV], [t.c:7:5] case 0:
> >>> <L0> [INV], [t.c:11:5] case 1: <L3> [INV]>
> >>>
> >>> <bb 3> :
> >>> # n_1 = PHI <n_8(D)(2), [t.c:8:28] n_13(4)>
> >>> # p_3 = PHI <[t.c:3:7] p_6(2), [t.c:8:15] p_12(4)>
> >>> [t.c:7:5] <L0>:
> >>> [t.c:8:15] p_12 = p_3 + 1;
> >>> [t.c:8:28] n_13 = n_1 + -1;
> >>> [t.c:8:28] if (n_13 != 0)
> >>> goto <bb 4>; [INV]
> >>> else
> >>> goto <bb 5>; [INV]
> >>>
> >>> <bb 4> :
> >>> PROF_edge_counter_21 = __gcov0.f[2];
> >>> PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
> >>> __gcov0.f[2] = PROF_edge_counter_22;
> >>> [t.c:7:5] goto <bb 3>; [100.00%]
> >>>
> >>> <bb 5> :
> >>> PROF_edge_counter_23 = __gcov0.f[3];
> >>> PROF_edge_counter_24 = PROF_edge_counter_23 + 1;
> >>> __gcov0.f[3] = PROF_edge_counter_24;
> >>> [t.c:9:16] _14 = p_12;
> >>> [t.c:9:16] goto <bb 10>; [INV]
> >>>
> >>> so the reason this goes wrong is that gcov associates the "wrong"
> >>> counter with the block containing
> >>> the 'case' label(s), for the case 0 it should have chosen the counter
> >>> from bb5 but it likely
> >>> computed the count of bb3?
> >>>
> >>> It might be that ordering blocks differently puts the instrumentation
> >>> to different blocks or it
> >>> makes gcovs association chose different blocks but that means it's
> >>> just luck and not fixing
> >>> the actual issue?
> >>>
> >>> To me it looks like the correct thing to investigate is switch
> >>> statement and/or case label
> >>> handling. One can also see that <L0> having line number 7 is wrong to
> >>> the extent that
> >>> the position of the label doesn't match the number of times it
> >>> executes in the source. So
> >>> placement of the label is wrong here, possibly caused by CFG cleanup
> >>> after CFG build
> >>> (but generally labels are not used for anything once the CFG is built
> >>> and coverage
> >>> instrumentation is late so it might fail due to us moving labels). It
> >>> might be OK to
> >>> avoid moving labels for --coverage but then coverage should possibly
> >>> look at edges
> >>> rather than labels?
> >>>
> >>
> >> Thanks, I investigated the Labels, it seems wrong at the beginning from
> >> .gimple to .cfg very early quite like PR90574:
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90574
> >>
> >> .gimple:
> >>
> >> int f (int s, int n)
> >> [small.c:2:1] {
> >> int D.2755;
> >> int p;
> >>
> >> [small.c:3:7] p = 0;
> >> [small.c:5:3] switch (s) <default: <D.2756>, [small.c:7:5] case 0:
> >> <D.2743>, [small.c:11:5] case 1: <D.2744>>
> >> [small.c:7:5] <D.2743>: <= case label
> >> <D.2748>: <= loop label
> >> [small.c:8:13] p = p + 1;
> >> [small.c:8:26] n = n + -1;
> >> [small.c:8:26] if (n != 0) goto <D.2748>; else goto <D.2746>;
> >> <D.2746>:
> >> [small.c:9:14] D.2755 = p;
> >> [small.c:9:14] return D.2755;
> >> [small.c:11:5] <D.2744>:
> >> <D.2751>:
> >> [small.c:12:13] p = p + 1;
> >> [small.c:12:26] n = n + -1;
> >> [small.c:12:26] if (n != 0) goto <D.2751>; else goto <D.2749>;
> >> <D.2749>:
> >> [small.c:13:14] D.2755 = p;
> >> [small.c:13:14] return D.2755;
> >> <D.2756>:
> >> [small.c:16:10] D.2755 = 0;
> >> [small.c:16:10] return D.2755;
> >> }
> >>
> >> .cfg:
> >>
> >> int f (int s, int n)
> >> {
> >> int p;
> >> int D.2755;
> >>
> >> <bb 2> :
> >> [small.c:3:7] p = 0;
> >> [small.c:5:3] switch (s) <default: <L6> [INV], [small.c:7:5] case 0:
> >> <L0> [INV], [small.c:11:5] case 1: <L3> [INV]>
> >>
> >> <bb 3> :
> >> [small.c:7:5] <L0>: <= case 0
> >> [small.c:8:13 discrim 1] p = p + 1;
> >> [small.c:8:26 discrim 1] n = n + -1;
> >> [small.c:8:26 discrim 1] if (n != 0)
> >> goto <bb 3>; [INV]
> >> else
> >> goto <bb 4>; [INV]
> >>
> >> <bb 4> :
> >> [small.c:9:14] D.2755 = p;
> >> [small.c:9:14] goto <bb 8>; [INV]
> >>
> >> <bb 5> :
> >> [small.c:11:5] <L3>: <= case 1
> >> [small.c:12:13 discrim 1] p = p + 1;
> >> [small.c:12:26 discrim 1] n = n + -1;
> >> [small.c:12:26 discrim 1] if (n != 0)
> >> goto <bb 5>; [INV]
> >> else
> >> goto <bb 6>; [INV]
> >>
> >>
> >> The labels are merged into the loop unexpected, so I tried below fix
> >> for --coverage if two labels are not on same line to start new basic block:
> >>
> >>
> >> index 10ca86714f4..b788198ac31 100644
> >> --- a/gcc/tree-cfg.cc
> >> +++ b/gcc/tree-cfg.cc
> >> @@ -2860,6 +2860,13 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> >> || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
> >> return true;
> >>
> >> + location_t loc_prev = gimple_location (plabel);
> >> + location_t locus = gimple_location (label_stmt);
> >> + expanded_location locus_e = expand_location (locus);
> >> +
> >> + if (flag_test_coverage && !same_line_p (locus, &locus_e,
> >> loc_prev))
> >> + return true;
> >> +
> >> cfg_stats.num_merged_labels++;
> >> return false;
> >> }
> >
> > Interesting. Note that in CFG cleanup we have the following condition
> > when deciding
> > whether to merge a forwarder block with the destination:
> >
> > locus = single_succ_edge (bb)->goto_locus;
> > ...
> > /* Now walk through the statements backward. We can ignore labels,
> > anything else means this is not a forwarder block. */
> > for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
> > {
> > gimple *stmt = gsi_stmt (gsi);
> >
> > switch (gimple_code (stmt))
> > {
> > case GIMPLE_LABEL:
> > if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *> (stmt))))
> > return false;
> > if (!optimize
> > && (gimple_has_location (stmt)
> > || LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
> > && gimple_location (stmt) != locus)
> > return false;
> >
> > it would be nice to sync the behavior between CFG creation and this.
> > In particular
> > a missing piece of the puzzle is how CFG creation sets ->goto_locus of the
> > edge
> > after your change and whether that goto_locus and the label locus
> > compare matches
> > your condition (the CFG cleanup one is even stricter but special cases
> > UNKNOWN_LOCATION).
> >
> > I also notice the !optimize vs. flag_test_coverage condition mismatch.
> >
> > That said - I think your change to stmt_starts_bb_p is definitely the
> > correct place to fix,
> > I'm wondering how to match up with CFG cleanup - possibly using
> > !optimize instead
> > of flag_test_coverage would even make sense for debugging as well - we
> > should be
> > able to put a breakpoint on the label hitting once rather than once
> > each loop iteration.
> >
>
> Unfortunately this change (flag_test_coverage -> !optimize ) caused hundred
> of gfortran cases execution failure with O0. Take gfortran.dg/index.f90 for
> example:
>
> .gimple:
>
> __attribute__((fn spec (". ")))
> void p ()
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:6:9] {
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:13:28]
> L.1:
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:14:28]
> L.2:
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:15:28]
> L.3:
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:16:28]
> L.4:
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:17:28]
> L.5:
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:18:72]
> L.6:
> }
>
> .cfg:
>
> ...
> Removing basic block 7
> ;; basic block 7, loop depth 0
> ;; pred:
> return;
> ;; succ: EXIT
>
>
> ;; 1 loops found
> ;;
> ;; Loop 0
> ;; header 0, latch 1
> ;; depth 0, outer -1
> ;; nodes: 0 1 2
> ;;2 succs { }
> __attribute__((fn spec (". ")))
> void p ()
> {
> <bb 2> :
>
> }
>
> Due to the "return;" is removed in bb 7.
OK, the issue is that make_edges_bb does nothing for an empty block
but it should at least create a fallthru edge here. Thus,
if (!last)
fallthru = true;
else
switch (gimple_code (last))
{
instead of simply returning if (!last). The alternative would be
to make sure that cleanup_dead_labels preserves at least one
statement in a block.
Looking at the testcases I wonder if preserving all the fallthru labels
is really necessary - for coverage we should have a counter ready. For
the testcase we arrive with
L.1:
L.2:
L.3:
L.4:
i = 1;
where the frontend simplified things but put labels at each line.
I suppose we could optimize this by re-computing TREE_USED and only
splitting before labels reached by a control statement? That would
cover the backedge case in the original testcase. cleanup_dead_labels
does something like that already.
> actually in build_gimple_cfg, cleanup_dead_labels will remove all labels L.1
> to L.6
> first, then make_edges fail to create edges for <bb 2> to <bb 7> due to they
> are all
> EMPTY bb in make_edges_bb...
>
>
> 240│ /* To speed up statement iterator walks, we first purge dead labels.
> */
> 241│ cleanup_dead_labels ();
> 242│
> 243│ /* Group case nodes to reduce the number of edges.
> 244│ We do this after cleaning up dead labels because otherwise we
> miss
> 245│ a lot of obvious case merging opportunities. */
> 246│ group_case_labels ();
> 247│
> 248│ /* Create the edges of the flowgraph. */
> 249│ discriminator_per_locus = new hash_table<locus_discrim_hasher> (13);
> 250├> make_edges ();
>
>
> <bb 0> :
>
> <bb 2> :
>
> <bb 3> :
>
> <bb 4> :
>
> <bb 5> :
>
> <bb 6> :
>
> <bb 7> :
> return;
>
> <bb 1> :
>
>
> Seems deadlock here as you said to set goto_locus as labels are removed before
> edges are created, the case could pass if I comment out the function
> cleanup_dead_labels(),
> so also not call it when !optimize?
>
> if (!!optimize)
> cleanup_dead_labels ();
That probably makes sense. Looking at group_case_labels () that also
seems to do unwanted things (to debugging and coverage), its comment
says that for
switch (i)
{
case 1:
/* fallthru */
case 2:
/* fallthru */
case 3:
k = 0;
it would replace that with
case 1..3:
k = 0;
but that also fails to produce correct coverage, right? Likewise
setting breakpoints.
Does preserving the labels help setting a goto_locus for the
fallthru edges? I don't see any code doing that, so
CFG cleanup will remove the forwarders we created again.
It would be nice to avoid creating blocks / preserving labels we'll
immediately remove again. For that we do need some analysis
before creating basic-blocks that determines whether a label is
possibly reached by a non-falltru edge.
Richard.
>
> Attached v2 patch could pass regression test on
> x86_64-linux-gnu/aarch64-linux-gnu.
>
--
Richard Biener <rguenther@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)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-07 8:53 ` Richard Biener
@ 2023-03-07 10:26 ` Xionghu Luo
2023-03-07 11:25 ` Richard Biener
0 siblings, 1 reply; 19+ messages in thread
From: Xionghu Luo @ 2023-03-07 10:26 UTC (permalink / raw)
To: Richard Biener; +Cc: Richard Biener, gcc-patches, luoxhu, hubicka
On 2023/3/7 16:53, Richard Biener wrote:
> On Tue, 7 Mar 2023, Xionghu Luo wrote:
>> Unfortunately this change (flag_test_coverage -> !optimize ) caused hundred
>> of gfortran cases execution failure with O0. Take gfortran.dg/index.f90 for
>> example:
>>
>> .gimple:
>>
>> __attribute__((fn spec (". ")))
>> void p ()
>> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:6:9] {
>> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:13:28]
>> L.1:
>> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:14:28]
>> L.2:
>> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:15:28]
>> L.3:
>> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:16:28]
>> L.4:
>> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:17:28]
>> L.5:
>> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:18:72]
>> L.6:
>> }
>>
>> .cfg:
>>
>> ...
>> Removing basic block 7
>> ;; basic block 7, loop depth 0
>> ;; pred:
>> return;
>> ;; succ: EXIT
>>
>>
>> ;; 1 loops found
>> ;;
>> ;; Loop 0
>> ;; header 0, latch 1
>> ;; depth 0, outer -1
>> ;; nodes: 0 1 2
>> ;;2 succs { }
>> __attribute__((fn spec (". ")))
>> void p ()
>> {
>> <bb 2> :
>>
>> }
>>
>> Due to the "return;" is removed in bb 7.
>
> OK, the issue is that make_edges_bb does nothing for an empty block
> but it should at least create a fallthru edge here. Thus,
>
> if (!last)
> fallthru = true;
>
> else
> switch (gimple_code (last))
> {
>
> instead of simply returning if (!last). The alternative would be
> to make sure that cleanup_dead_labels preserves at least one
> statement in a block.
>
> Looking at the testcases I wonder if preserving all the fallthru labels
> is really necessary - for coverage we should have a counter ready. For
> the testcase we arrive with
>
> L.1:
> L.2:
> L.3:
> L.4:
> i = 1;
It was:
<bb 0> :
<bb 2> :
L.1:
<bb 3> :
L.2:
<bb 4> :
L.3:
<bb 5> :
L.4:
<bb 6> :
L.5:
<bb 7> :
L.6:
return;
<bb 1> :
before the second call of cleanup_dead_labels, after it, all labels are
removed, then tree_forwarder_block_p remove all forworders. Yes, it
creates blocks and remove blocks immediately...
>
> where the frontend simplified things but put labels at each line.
> I suppose we could optimize this by re-computing TREE_USED and only
> splitting before labels reached by a control statement? That would
> cover the backedge case in the original testcase. cleanup_dead_labels
> does something like that already.
>
>> actually in build_gimple_cfg, cleanup_dead_labels will remove all labels L.1
>> to L.6
>> first, then make_edges fail to create edges for <bb 2> to <bb 7> due to they
>> are all
>> EMPTY bb in make_edges_bb...
>>
>>
>> 240│ /* To speed up statement iterator walks, we first purge dead labels.
>> */
>> 241│ cleanup_dead_labels ();
>> 242│
>> 243│ /* Group case nodes to reduce the number of edges.
>> 244│ We do this after cleaning up dead labels because otherwise we
>> miss
>> 245│ a lot of obvious case merging opportunities. */
>> 246│ group_case_labels ();
>> 247│
>> 248│ /* Create the edges of the flowgraph. */
>> 249│ discriminator_per_locus = new hash_table<locus_discrim_hasher> (13);
>> 250├> make_edges ();
>>
>>
>> <bb 0> :
>>
>> <bb 2> :
>>
>> <bb 3> :
>>
>> <bb 4> :
>>
>> <bb 5> :
>>
>> <bb 6> :
>>
>> <bb 7> :
>> return;
>>
>> <bb 1> :
>>
>>
>> Seems deadlock here as you said to set goto_locus as labels are removed before
>> edges are created, the case could pass if I comment out the function
>> cleanup_dead_labels(),
>> so also not call it when !optimize?
>>
>> if (!!optimize)
>> cleanup_dead_labels ();
>
> That probably makes sense. Looking at group_case_labels () that also
> seems to do unwanted things (to debugging and coverage), its comment
> says that for
>
> switch (i)
> {
> case 1:
> /* fallthru */
> case 2:
> /* fallthru */
> case 3:
> k = 0;
>
> it would replace that with
>
> case 1..3:
> k = 0;
>
> but that also fails to produce correct coverage, right? Likewise
> setting breakpoints.
Yes. Should also exclude this.
>
> Does preserving the labels help setting a goto_locus for the
> fallthru edges? I don't see any code doing that, so
> CFG cleanup will remove the forwarders we created again.
For the backedge case with switch-case-do-while, tree_forwarder_block_p
returns false when iterating the statement check.
The new created <bb 3> with only one case label instruction still owns
location information in it, so CFG cleanup won't remove the forwarders.
390│ for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
391│ {
392│ gimple *stmt = gsi_stmt (gsi);
393│
394│ switch (gimple_code (stmt))
395│ {
396│ case GIMPLE_LABEL:
397│ if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *>(stmt))))
398│ return false;
399│ if (!optimize
400│ && (gimple_has_location (stmt)
401│ || LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
402│ && gimple_location (stmt) != locus)
403├> return false;
404│ break;
(gdb) ps stmt
<L0>:
(gdb) p gimple_location (stmt)
$154 = 2147483656
(gdb) pel $154
{file = 0x3e41af0 "small.c", line = 7, column = 5, data = 0x7ffff6f80420, sysp = false}
(gdb)
(gdb) pbb bb
;; basic block 3, loop depth 0
;; pred: 2
<L0>:
;; succ: 4
>
> It would be nice to avoid creating blocks / preserving labels we'll
> immediately remove again. For that we do need some analysis
> before creating basic-blocks that determines whether a label is
> possibly reached by a non-falltru edge.
>
<bb 2> :
p = 0;
switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
<bb 3> :
<L0>: <= prev_stmt
<D.2748>: <= stmt
p = p + 1;
n = n + -1;
if (n != 0) goto <D.2748>; else goto <D.2746>;
Check if <L0> is a case label and <D.2748> is a goto target then return true
in stmt_starts_bb_p to start a new basic block? This would avoid creating and
removing blocks, but cleanup_dead_labels has all bbs setup while stmt_starts_bb_p
does't yet to iterate bbs/labels to establish label_for_bb[] map?
Thanks,
Xionghu
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-07 10:26 ` Xionghu Luo
@ 2023-03-07 11:25 ` Richard Biener
2023-03-08 13:07 ` [PATCH v3] " Xionghu Luo
0 siblings, 1 reply; 19+ messages in thread
From: Richard Biener @ 2023-03-07 11:25 UTC (permalink / raw)
To: Xionghu Luo; +Cc: Richard Biener, gcc-patches, luoxhu, hubicka
[-- Attachment #1: Type: text/plain, Size: 7467 bytes --]
On Tue, 7 Mar 2023, Xionghu Luo wrote:
>
>
> On 2023/3/7 16:53, Richard Biener wrote:
> > On Tue, 7 Mar 2023, Xionghu Luo wrote:
>
> >> Unfortunately this change (flag_test_coverage -> !optimize ) caused hundred
> >> of gfortran cases execution failure with O0. Take gfortran.dg/index.f90
> >> for
> >> example:
> >>
> >> .gimple:
> >>
> >> __attribute__((fn spec (". ")))
> >> void p ()
> >> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:6:9]
> >> {
> >> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:13:28]
> >> L.1:
> >> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:14:28]
> >> L.2:
> >> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:15:28]
> >> L.3:
> >> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:16:28]
> >> L.4:
> >> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:17:28]
> >> L.5:
> >> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:18:72]
> >> L.6:
> >> }
> >>
> >> .cfg:
> >>
> >> ...
> >> Removing basic block 7
> >> ;; basic block 7, loop depth 0
> >> ;; pred:
> >> return;
> >> ;; succ: EXIT
> >>
> >>
> >> ;; 1 loops found
> >> ;;
> >> ;; Loop 0
> >> ;; header 0, latch 1
> >> ;; depth 0, outer -1
> >> ;; nodes: 0 1 2
> >> ;;2 succs { }
> >> __attribute__((fn spec (". ")))
> >> void p ()
> >> {
> >> <bb 2> :
> >>
> >> }
> >>
> >> Due to the "return;" is removed in bb 7.
> >
> > OK, the issue is that make_edges_bb does nothing for an empty block
> > but it should at least create a fallthru edge here. Thus,
> >
> > if (!last)
> > fallthru = true;
> >
> > else
> > switch (gimple_code (last))
> > {
> >
> > instead of simply returning if (!last). The alternative would be
> > to make sure that cleanup_dead_labels preserves at least one
> > statement in a block.
> >
> > Looking at the testcases I wonder if preserving all the fallthru labels
> > is really necessary - for coverage we should have a counter ready. For
> > the testcase we arrive with
> >
> > L.1:
> > L.2:
> > L.3:
> > L.4:
> > i = 1;
>
> It was:
>
> <bb 0> :
>
> <bb 2> :
> L.1:
>
> <bb 3> :
> L.2:
>
> <bb 4> :
> L.3:
>
> <bb 5> :
> L.4:
>
> <bb 6> :
> L.5:
>
> <bb 7> :
> L.6:
> return;
>
> <bb 1> :
>
> before the second call of cleanup_dead_labels, after it, all labels are
> removed, then tree_forwarder_block_p remove all forworders. Yes, it
> creates blocks and remove blocks immediately...
>
> >
> > where the frontend simplified things but put labels at each line.
> > I suppose we could optimize this by re-computing TREE_USED and only
> > splitting before labels reached by a control statement? That would
> > cover the backedge case in the original testcase. cleanup_dead_labels
> > does something like that already.
> >
> >> actually in build_gimple_cfg, cleanup_dead_labels will remove all labels
> >> L.1
> >> to L.6
> >> first, then make_edges fail to create edges for <bb 2> to <bb 7> due to
> >> they
> >> are all
> >> EMPTY bb in make_edges_bb...
> >>
> >>
> >> 240│ /* To speed up statement iterator walks, we first purge dead
> >> labels.
> >> */
> >> 241│ cleanup_dead_labels ();
> >> 242│
> >> 243│ /* Group case nodes to reduce the number of edges.
> >> 244│ We do this after cleaning up dead labels because otherwise we
> >> miss
> >> 245│ a lot of obvious case merging opportunities. */
> >> 246│ group_case_labels ();
> >> 247│
> >> 248│ /* Create the edges of the flowgraph. */
> >> 249│ discriminator_per_locus = new hash_table<locus_discrim_hasher>
> >> (13);
> >> 250├> make_edges ();
> >>
> >>
> >> <bb 0> :
> >>
> >> <bb 2> :
> >>
> >> <bb 3> :
> >>
> >> <bb 4> :
> >>
> >> <bb 5> :
> >>
> >> <bb 6> :
> >>
> >> <bb 7> :
> >> return;
> >>
> >> <bb 1> :
> >>
> >>
> >> Seems deadlock here as you said to set goto_locus as labels are removed
> >> before
> >> edges are created, the case could pass if I comment out the function
> >> cleanup_dead_labels(),
> >> so also not call it when !optimize?
> >>
> >> if (!!optimize)
> >> cleanup_dead_labels ();
> >
> > That probably makes sense. Looking at group_case_labels () that also
> > seems to do unwanted things (to debugging and coverage), its comment
> > says that for
> >
> > switch (i)
> > {
> > case 1:
> > /* fallthru */
> > case 2:
> > /* fallthru */
> > case 3:
> > k = 0;
> >
> > it would replace that with
> >
> > case 1..3:
> > k = 0;
> >
> > but that also fails to produce correct coverage, right? Likewise
> > setting breakpoints.
>
> Yes. Should also exclude this.
>
> >
> > Does preserving the labels help setting a goto_locus for the
> > fallthru edges? I don't see any code doing that, so
> > CFG cleanup will remove the forwarders we created again.
>
> For the backedge case with switch-case-do-while, tree_forwarder_block_p
> returns false when iterating the statement check.
> The new created <bb 3> with only one case label instruction still owns
> location information in it, so CFG cleanup won't remove the forwarders.
>
> 390│ for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
> 391│ {
> 392│ gimple *stmt = gsi_stmt (gsi);
> 393│
> 394│ switch (gimple_code (stmt))
> 395│ {
> 396│ case GIMPLE_LABEL:
> 397│ if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *>(stmt))))
> 398│ return false;
> 399│ if (!optimize
> 400│ && (gimple_has_location (stmt)
> 401│ || LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
> 402│ && gimple_location (stmt) != locus)
> 403├> return false;
> 404│ break;
>
>
> (gdb) ps stmt
> <L0>:
> (gdb) p gimple_location (stmt)
> $154 = 2147483656
> (gdb) pel $154
> {file = 0x3e41af0 "small.c", line = 7, column = 5, data = 0x7ffff6f80420, sysp
> = false}
> (gdb)
> (gdb) pbb bb
> ;; basic block 3, loop depth 0
> ;; pred: 2
> <L0>:
> ;; succ: 4
>
> >
> > It would be nice to avoid creating blocks / preserving labels we'll
> > immediately remove again. For that we do need some analysis
> > before creating basic-blocks that determines whether a label is
> > possibly reached by a non-falltru edge.
> >
>
>
> <bb 2> :
> p = 0;
> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
>
> <bb 3> :
> <L0>: <= prev_stmt
> <D.2748>: <= stmt
> p = p + 1;
> n = n + -1;
> if (n != 0) goto <D.2748>; else goto <D.2746>;
>
> Check if <L0> is a case label and <D.2748> is a goto target then return true
> in stmt_starts_bb_p to start a new basic block? This would avoid creating and
> removing blocks, but cleanup_dead_labels has all bbs setup while
> stmt_starts_bb_p
> does't yet to iterate bbs/labels to establish label_for_bb[] map?
Yes. I think we'd need something more pragmatic before make_blocks (),
like re-computing TREE_USED of the label decls or computing a bitmap
of targeted labels (targeted by goto, switch or any other means).
I'll note that doing a cleanup_dead_labels () like optimization before
we create blocks will help keeping LABEL_DECL_UID and thus
label_to_block_map dense. But it does look like a bit of
an chicken-and-egg problem and the question is how effective the
dead label removal is in practice.
Richard.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-07 11:25 ` Richard Biener
@ 2023-03-08 13:07 ` Xionghu Luo
2023-03-09 12:02 ` Richard Biener
0 siblings, 1 reply; 19+ messages in thread
From: Xionghu Luo @ 2023-03-08 13:07 UTC (permalink / raw)
To: Richard Biener; +Cc: Richard Biener, gcc-patches, luoxhu, hubicka
On 2023/3/7 19:25, Richard Biener wrote:
>>> It would be nice to avoid creating blocks / preserving labels we'll
>>> immediately remove again. For that we do need some analysis
>>> before creating basic-blocks that determines whether a label is
>>> possibly reached by a non-falltru edge.
>>>
>>
>> <bb 2> :
>> p = 0;
>> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
>>
>> <bb 3> :
>> <L0>: <= prev_stmt
>> <D.2748>: <= stmt
>> p = p + 1;
>> n = n + -1;
>> if (n != 0) goto <D.2748>; else goto <D.2746>;
>>
>> Check if <L0> is a case label and <D.2748> is a goto target then return true
>> in stmt_starts_bb_p to start a new basic block? This would avoid creating and
>> removing blocks, but cleanup_dead_labels has all bbs setup while
>> stmt_starts_bb_p
>> does't yet to iterate bbs/labels to establish label_for_bb[] map?
> Yes. I think we'd need something more pragmatic before make_blocks (),
> like re-computing TREE_USED of the label decls or computing a bitmap
> of targeted labels (targeted by goto, switch or any other means).
>
> I'll note that doing a cleanup_dead_labels () like optimization before
> we create blocks will help keeping LABEL_DECL_UID and thus
> label_to_block_map dense. But it does look like a bit of
> an chicken-and-egg problem and the question is how effective the
> dead label removal is in practice.
Tried to add function compute_target_labels(not sure whether the function
name is suitable) in the front of make_blocks_1, now the fortran case doesn't
create/removing blocks now, but I still have several questions:
1. I used hash_set<tree> to save the target labels instead of bitmap, as labels
are tree type value instead of block index so bitmap is not good for it since
we don't have LABEL_DECL_UID now?
2. Is the compute_target_labels still only for !optimize? And if we compute
the target labels before create bbs, it is unnessary to guard the first
cleanup_dead_labels under !optimize now, because the switch-case-do-while
case already create new block for CASE_LABEL already.
3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
labels_eh?
PS1: The v3 patch will cause one test case fail:
Number of regressions in total: 1
> FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess errors)
due to this exausting case has labels from L0 to L100001, they won't be optimized
to a simple if-else expression like before...
PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it so
just skip these label...
+ case GIMPLE_GOTO:
+#if 0
+ if (!computed_goto_p (stmt))
+ {
+ tree dest = gimple_goto_dest (stmt);
+ target_labels->add (dest);
+ }
+#endif
+ break;
Change the #if 0 to #if 1 result in:
Number of regressions in total: 8
> FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess errors)
> FAIL: gcc.dg/analyzer/explode-2a.c (test for excess errors)
> FAIL: gcc.dg/analyzer/pragma-2.c (test for excess errors)
> FAIL: gfortran.dg/bound_2.f90 -O0 execution test
> FAIL: gfortran.dg/bound_7.f90 -O0 execution test
> FAIL: gfortran.dg/char_result_14.f90 -O0 execution test
> FAIL: gfortran.dg/pointer_array_1.f90 -O0 execution test
> FAIL: gfortran.dg/select_type_15.f03 -O0 execution test
Paste the updated patch v3:
v3: Add compute_target_labels and call it in the front of make_blocks_1.
Start a new basic block if two labels have different location when
test-coverage.
Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
master?
gcc/ChangeLog:
PR gcov/93680
* tree-cfg.cc (stmt_starts_bb_p): Check whether the label is in
target_labels.
(compute_target_labels): New function.
(make_blocks_1): Call compute_target_labels.
gcc/testsuite/ChangeLog:
PR gcov/93680
* g++.dg/gcov/gcov-1.C: Correct counts.
* gcc.misc-tests/gcov-4.c: Likewise.
* gcc.misc-tests/gcov-pr85332.c: Likewise.
* lib/gcov.exp: Also clean gcda if fail.
* gcc.misc-tests/gcov-pr93680.c: New test.
Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
---
gcc/tree-cfg.cc | 68 ++++++++++++++++++++-
gcc/testsuite/g++.dg/gcov/gcov-1.C | 2 +-
gcc/testsuite/gcc.dg/analyzer/paths-4.c | 8 +--
gcc/testsuite/gcc.misc-tests/gcov-pr85332.c | 2 +-
gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 ++++++++
gcc/testsuite/lib/gcov.exp | 4 +-
6 files changed, 96 insertions(+), 12 deletions(-)
create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index a9fcc7fd050..0f8efcf4aa3 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -164,7 +164,7 @@ static edge gimple_redirect_edge_and_branch (edge, basic_block);
static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
/* Various helpers. */
-static inline bool stmt_starts_bb_p (gimple *, gimple *);
+static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set<tree> *);
static int gimple_verify_flow_info (void);
static void gimple_make_forwarder_block (edge);
static gimple *first_non_label_stmt (basic_block);
@@ -521,6 +521,59 @@ gimple_call_initialize_ctrl_altering (gimple *stmt)
gimple_call_set_ctrl_altering (stmt, false);
}
+/* Compute target labels to save useful labels. */
+static void
+compute_target_labels (gimple_seq seq, hash_set<tree> *target_labels)
+{
+ gimple *stmt = NULL;
+ gimple_stmt_iterator j = gsi_start (seq);
+
+ while (!gsi_end_p (j))
+ {
+ stmt = gsi_stmt (j);
+
+ switch (gimple_code (stmt))
+ {
+ case GIMPLE_COND:
+ {
+ gcond *cstmt = as_a <gcond *> (stmt);
+ tree true_label = gimple_cond_true_label (cstmt);
+ tree false_label = gimple_cond_false_label (cstmt);
+ target_labels->add (true_label);
+ target_labels->add (false_label);
+ }
+ break;
+ case GIMPLE_SWITCH:
+ {
+ gswitch *gstmt = as_a <gswitch *> (stmt);
+ size_t i, n = gimple_switch_num_labels (gstmt);
+ tree elt, label;
+ for (i = 0; i < n; i++)
+ {
+ elt = gimple_switch_label (gstmt, i);
+ label = CASE_LABEL (elt);
+ target_labels->add (label);
+ }
+ }
+ break;
+ case GIMPLE_GOTO:
+#if 0
+ if (!computed_goto_p (stmt))
+ {
+ tree dest = gimple_goto_dest (stmt);
+ target_labels->add (dest);
+ }
+#endif
+ break;
+
+ default:
+ break;
+ }
+
+ gsi_next (&j);
+ }
+}
+
/* Insert SEQ after BB and build a flowgraph. */
@@ -532,6 +585,10 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
gimple *prev_stmt = NULL;
bool start_new_block = true;
bool first_stmt_of_seq = true;
+ hash_set<tree> target_labels;
+
+ if (!optimize)
+ compute_target_labels (seq, &target_labels);
while (!gsi_end_p (i))
{
@@ -553,7 +610,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
/* If the statement starts a new basic block or if we have determined
in a previous pass that we need to create a new block for STMT, do
so now. */
- if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt))
+ if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt, &target_labels))
{
if (!first_stmt_of_seq)
gsi_split_seq_before (&i, &seq);
@@ -2832,7 +2889,8 @@ simple_goto_p (gimple *t)
label. */
static inline bool
-stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
+stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt,
+ hash_set<tree> *target_labels)
{
if (stmt == NULL)
return false;
@@ -2860,6 +2918,10 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
|| !DECL_ARTIFICIAL (gimple_label_label (plabel)))
return true;
+ if (!optimize
+ && target_labels->contains (gimple_label_label (label_stmt)))
+ return true;
+
cfg_stats.num_merged_labels++;
return false;
}
diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
index ee383b480a8..01e7084fb03 100644
--- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
+++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
@@ -263,7 +263,7 @@ test_switch (int i, int j)
case 2:
result = do_something (1024);
break;
- case 3: /* count(3) */
+ case 3: /* count(2) */
case 4:
/* branch(67) */
if (j == 2) /* count(3) */
diff --git a/gcc/testsuite/gcc.dg/analyzer/paths-4.c b/gcc/testsuite/gcc.dg/analyzer/paths-4.c
index b72e658739e..fdf33e68d0c 100644
--- a/gcc/testsuite/gcc.dg/analyzer/paths-4.c
+++ b/gcc/testsuite/gcc.dg/analyzer/paths-4.c
@@ -35,18 +35,18 @@ int test_2 (struct state *s)
do_stuff (s, 0);
break;
case 1:
- __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */
+ __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enode" } */
do_stuff (s, 17);
break;
case 2:
- __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */
+ __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enode" } */
do_stuff (s, 5);
break;
case 3:
- __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */
+ __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enode" } */
return 42;
case 4:
- __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */
+ __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enode" } */
return -3;
}
}
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
index 73e50b19fc7..b37e760910c 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
@@ -7,7 +7,7 @@ int doit(int sel, int n, void *p)
switch (sel)
{
- case 0: /* count(3) */
+ case 0: /* count(1) */
do {*p0 += *p0;} while (--n); /* count(3) */
return *p0 == 0; /* count(1) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
new file mode 100644
index 00000000000..2fe340c4011
--- /dev/null
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
@@ -0,0 +1,24 @@
+/* { dg-options "-fprofile-arcs -ftest-coverage" } */
+/* { dg-do run { target native } } */
+
+int f(int s, int n)
+{
+ int p = 0;
+
+ switch (s)
+ {
+ case 0: /* count(1) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+
+ case 1: /* count(1) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+ }
+
+ return 0;
+}
+
+int main() { f(0, 5); f(1, 5); return 0; }
+
+/* { dg-final { run-gcov gcov-pr93680.c } } */
diff --git a/gcc/testsuite/lib/gcov.exp b/gcc/testsuite/lib/gcov.exp
index 80e74aeb220..07e1978d25d 100644
--- a/gcc/testsuite/lib/gcov.exp
+++ b/gcc/testsuite/lib/gcov.exp
@@ -424,9 +424,7 @@ proc run-gcov { args } {
}
if { $tfailed > 0 } {
fail "$testname gcov: $lfailed failures in line counts, $bfailed in branch percentages, $cfailed in return percentages, $ifailed in intermediate format"
- if { $xfailed } {
- clean-gcov $testcase
- }
+ clean-gcov $testcase
} else {
pass "$testname gcov"
clean-gcov $testcase
--
2.27.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-08 13:07 ` [PATCH v3] " Xionghu Luo
@ 2023-03-09 12:02 ` Richard Biener
2023-03-14 2:06 ` [PATCH v4] " Xionghu Luo
2023-03-15 10:07 ` Xionghu Luo
0 siblings, 2 replies; 19+ messages in thread
From: Richard Biener @ 2023-03-09 12:02 UTC (permalink / raw)
To: Xionghu Luo; +Cc: Richard Biener, gcc-patches, luoxhu, hubicka
On Wed, 8 Mar 2023, Xionghu Luo wrote:
>
>
> On 2023/3/7 19:25, Richard Biener wrote:
> >>> It would be nice to avoid creating blocks / preserving labels we'll
> >>> immediately remove again. For that we do need some analysis
> >>> before creating basic-blocks that determines whether a label is
> >>> possibly reached by a non-falltru edge.
> >>>
> >>
> >> <bb 2> :
> >> p = 0;
> >> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
> >>
> >> <bb 3> :
> >> <L0>: <= prev_stmt
> >> <D.2748>: <= stmt
> >> p = p + 1;
> >> n = n + -1;
> >> if (n != 0) goto <D.2748>; else goto <D.2746>;
> >>
> >> Check if <L0> is a case label and <D.2748> is a goto target then return
> >> true
> >> in stmt_starts_bb_p to start a new basic block? This would avoid creating
> >> and
> >> removing blocks, but cleanup_dead_labels has all bbs setup while
> >> stmt_starts_bb_p
> >> does't yet to iterate bbs/labels to establish label_for_bb[] map?
>
> > Yes. I think we'd need something more pragmatic before make_blocks (),
> > like re-computing TREE_USED of the label decls or computing a bitmap
> > of targeted labels (targeted by goto, switch or any other means).
> >
> > I'll note that doing a cleanup_dead_labels () like optimization before
> > we create blocks will help keeping LABEL_DECL_UID and thus
> > label_to_block_map dense. But it does look like a bit of
> > an chicken-and-egg problem and the question is how effective the
> > dead label removal is in practice.
>
> Tried to add function compute_target_labels(not sure whether the function
> name is suitable) in the front of make_blocks_1, now the fortran case doesn't
> create/removing blocks now, but I still have several questions:
>
> 1. I used hash_set<tree> to save the target labels instead of bitmap, as
> labels
> are tree type value instead of block index so bitmap is not good for it since
> we don't have LABEL_DECL_UID now?
We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
hash_set<tree> vs. bitmap is somewhat arbitrary here. The real cost is
the extra walk over all stmts.
> 2. Is the compute_target_labels still only for !optimize? And if we compute
> the target labels before create bbs, it is unnessary to guard the first
> cleanup_dead_labels under !optimize now, because the switch-case-do-while
> case already create new block for CASE_LABEL already.
OK.
> 3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
> labels_eh?
I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
coverage already?
> PS1: The v3 patch will cause one test case fail:
>
> Number of regressions in total: 1
> > FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
> > errors)
>
> due to this exausting case has labels from L0 to L100001, they won't be
> optimized
> to a simple if-else expression like before...
Hmm, that's somewhat unexpected.
>
> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it
> so
> just skip these label...
Please investigate, we might be missing a corner case here.
>
> + case GIMPLE_GOTO:
> +#if 0
> + if (!computed_goto_p (stmt))
> + {
> + tree dest = gimple_goto_dest (stmt);
> + target_labels->add (dest);
> + }
> +#endif
> + break;
>
> Change the #if 0 to #if 1 result in:
>
> Number of regressions in total: 8
> > FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
> > FAIL: errors)
> > FAIL: gcc.dg/analyzer/explode-2a.c (test for excess errors)
> > FAIL: gcc.dg/analyzer/pragma-2.c (test for excess errors)
> > FAIL: gfortran.dg/bound_2.f90 -O0 execution test
> > FAIL: gfortran.dg/bound_7.f90 -O0 execution test
> > FAIL: gfortran.dg/char_result_14.f90 -O0 execution test
> > FAIL: gfortran.dg/pointer_array_1.f90 -O0 execution test
> > FAIL: gfortran.dg/select_type_15.f03 -O0 execution test
>
>
>
> Paste the updated patch v3:
The gcov testcase adjustments look good, does the analyzer testcase
(missing in the changelog) get different CFG input?
Thanks,
Richard.
>
> v3: Add compute_target_labels and call it in the front of make_blocks_1.
>
> Start a new basic block if two labels have different location when
> test-coverage.
>
> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
> master?
>
> gcc/ChangeLog:
>
> PR gcov/93680
> * tree-cfg.cc (stmt_starts_bb_p): Check whether the label is in
> target_labels.
> (compute_target_labels): New function.
> (make_blocks_1): Call compute_target_labels.
>
> gcc/testsuite/ChangeLog:
>
> PR gcov/93680
> * g++.dg/gcov/gcov-1.C: Correct counts.
> * gcc.misc-tests/gcov-4.c: Likewise.
> * gcc.misc-tests/gcov-pr85332.c: Likewise.
> * lib/gcov.exp: Also clean gcda if fail.
> * gcc.misc-tests/gcov-pr93680.c: New test.
>
> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
> ---
> gcc/tree-cfg.cc | 68 ++++++++++++++++++++-
> gcc/testsuite/g++.dg/gcov/gcov-1.C | 2 +-
> gcc/testsuite/gcc.dg/analyzer/paths-4.c | 8 +--
> gcc/testsuite/gcc.misc-tests/gcov-pr85332.c | 2 +-
> gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 ++++++++
> gcc/testsuite/lib/gcov.exp | 4 +-
> 6 files changed, 96 insertions(+), 12 deletions(-)
> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
>
> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> index a9fcc7fd050..0f8efcf4aa3 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -164,7 +164,7 @@ static edge gimple_redirect_edge_and_branch (edge,
> basic_block);
> static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
>
> /* Various helpers. */
> -static inline bool stmt_starts_bb_p (gimple *, gimple *);
> +static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set<tree> *);
> static int gimple_verify_flow_info (void);
> static void gimple_make_forwarder_block (edge);
> static gimple *first_non_label_stmt (basic_block);
> @@ -521,6 +521,59 @@ gimple_call_initialize_ctrl_altering (gimple *stmt)
> gimple_call_set_ctrl_altering (stmt, false);
> }
>
> +/* Compute target labels to save useful labels. */
> +static void
> +compute_target_labels (gimple_seq seq, hash_set<tree> *target_labels)
> +{
> + gimple *stmt = NULL;
> + gimple_stmt_iterator j = gsi_start (seq);
> +
> + while (!gsi_end_p (j))
> + {
> + stmt = gsi_stmt (j);
> +
> + switch (gimple_code (stmt))
> + {
> + case GIMPLE_COND:
> + {
> + gcond *cstmt = as_a <gcond *> (stmt);
> + tree true_label = gimple_cond_true_label (cstmt);
> + tree false_label = gimple_cond_false_label (cstmt);
> + target_labels->add (true_label);
> + target_labels->add (false_label);
> + }
> + break;
> + case GIMPLE_SWITCH:
> + {
> + gswitch *gstmt = as_a <gswitch *> (stmt);
> + size_t i, n = gimple_switch_num_labels (gstmt);
> + tree elt, label;
> + for (i = 0; i < n; i++)
> + {
> + elt = gimple_switch_label (gstmt, i);
> + label = CASE_LABEL (elt);
> + target_labels->add (label);
> + }
> + }
> + break;
> + case GIMPLE_GOTO:
> +#if 0
> + if (!computed_goto_p (stmt))
> + {
> + tree dest = gimple_goto_dest (stmt);
> + target_labels->add (dest);
> + }
> +#endif
> + break;
> +
> + default:
> + break;
> + }
> +
> + gsi_next (&j);
> + }
> +}
> +
>
> /* Insert SEQ after BB and build a flowgraph. */
>
> @@ -532,6 +585,10 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
> gimple *prev_stmt = NULL;
> bool start_new_block = true;
> bool first_stmt_of_seq = true;
> + hash_set<tree> target_labels;
> +
> + if (!optimize)
> + compute_target_labels (seq, &target_labels);
>
> while (!gsi_end_p (i))
> {
> @@ -553,7 +610,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
> /* If the statement starts a new basic block or if we have determined
> in a previous pass that we need to create a new block for STMT, do
> so now. */
> - if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt))
> + if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt,
> &target_labels))
> {
> if (!first_stmt_of_seq)
> gsi_split_seq_before (&i, &seq);
> @@ -2832,7 +2889,8 @@ simple_goto_p (gimple *t)
> label. */
>
> static inline bool
> -stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> +stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt,
> + hash_set<tree> *target_labels)
> {
> if (stmt == NULL)
> return false;
> @@ -2860,6 +2918,10 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
> return true;
> + if (!optimize
> + && target_labels->contains (gimple_label_label (label_stmt)))
> + return true;
> +
> cfg_stats.num_merged_labels++;
> return false;
> }
> diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C
> b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> index ee383b480a8..01e7084fb03 100644
> --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
> +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> @@ -263,7 +263,7 @@ test_switch (int i, int j)
> case 2:
> result = do_something (1024);
> break;
> - case 3: /* count(3) */
> + case 3: /* count(2) */
> case 4:
> /* branch(67) */
> if (j == 2) /* count(3) */
> diff --git a/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> b/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> index b72e658739e..fdf33e68d0c 100644
> --- a/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> +++ b/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> @@ -35,18 +35,18 @@ int test_2 (struct state *s)
> do_stuff (s, 0);
> break;
> case 1:
> - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
> do_stuff (s, 17);
> break;
> case 2:
> - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
> do_stuff (s, 5);
> break;
> case 3:
> - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
> return 42;
> case 4:
> - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
> return -3;
> }
> }
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> index 73e50b19fc7..b37e760910c 100644
> --- a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> @@ -7,7 +7,7 @@ int doit(int sel, int n, void *p)
>
> switch (sel)
> {
> - case 0: /* count(3) */
> + case 0: /* count(1) */
> do {*p0 += *p0;} while (--n); /* count(3) */
> return *p0 == 0; /* count(1) */
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> new file mode 100644
> index 00000000000..2fe340c4011
> --- /dev/null
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> @@ -0,0 +1,24 @@
> +/* { dg-options "-fprofile-arcs -ftest-coverage" } */
> +/* { dg-do run { target native } } */
> +
> +int f(int s, int n)
> +{
> + int p = 0;
> +
> + switch (s)
> + {
> + case 0: /* count(1) */
> + do { p++; } while (--n); /* count(5) */
> + return p; /* count(1) */
> +
> + case 1: /* count(1) */
> + do { p++; } while (--n); /* count(5) */
> + return p; /* count(1) */
> + }
> +
> + return 0;
> +}
> +
> +int main() { f(0, 5); f(1, 5); return 0; }
> +
> +/* { dg-final { run-gcov gcov-pr93680.c } } */
> diff --git a/gcc/testsuite/lib/gcov.exp b/gcc/testsuite/lib/gcov.exp
> index 80e74aeb220..07e1978d25d 100644
> --- a/gcc/testsuite/lib/gcov.exp
> +++ b/gcc/testsuite/lib/gcov.exp
> @@ -424,9 +424,7 @@ proc run-gcov { args } {
> }
> if { $tfailed > 0 } {
> fail "$testname gcov: $lfailed failures in line counts, $bfailed in
> branch percentages, $cfailed in return percentages, $ifailed in intermediate
> format"
> - if { $xfailed } {
> - clean-gcov $testcase
> - }
> + clean-gcov $testcase
> } else {
> pass "$testname gcov"
> clean-gcov $testcase
>
--
Richard Biener <rguenther@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)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-09 12:02 ` Richard Biener
@ 2023-03-14 2:06 ` Xionghu Luo
2023-03-21 11:18 ` Richard Biener
2023-03-15 10:07 ` Xionghu Luo
1 sibling, 1 reply; 19+ messages in thread
From: Xionghu Luo @ 2023-03-14 2:06 UTC (permalink / raw)
To: Richard Biener; +Cc: Richard Biener, gcc-patches, luoxhu, hubicka
On 2023/3/9 20:02, Richard Biener wrote:
> On Wed, 8 Mar 2023, Xionghu Luo wrote:
>
>>
>>
>> On 2023/3/7 19:25, Richard Biener wrote:
>>>>> It would be nice to avoid creating blocks / preserving labels we'll
>>>>> immediately remove again. For that we do need some analysis
>>>>> before creating basic-blocks that determines whether a label is
>>>>> possibly reached by a non-falltru edge.
>>>>>
>>>>
>>>> <bb 2> :
>>>> p = 0;
>>>> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
>>>>
>>>> <bb 3> :
>>>> <L0>: <= prev_stmt
>>>> <D.2748>: <= stmt
>>>> p = p + 1;
>>>> n = n + -1;
>>>> if (n != 0) goto <D.2748>; else goto <D.2746>;
>>>>
>>>> Check if <L0> is a case label and <D.2748> is a goto target then return
>>>> true
>>>> in stmt_starts_bb_p to start a new basic block? This would avoid creating
>>>> and
>>>> removing blocks, but cleanup_dead_labels has all bbs setup while
>>>> stmt_starts_bb_p
>>>> does't yet to iterate bbs/labels to establish label_for_bb[] map?
>>
>>> Yes. I think we'd need something more pragmatic before make_blocks (),
>>> like re-computing TREE_USED of the label decls or computing a bitmap
>>> of targeted labels (targeted by goto, switch or any other means).
>>>
>>> I'll note that doing a cleanup_dead_labels () like optimization before
>>> we create blocks will help keeping LABEL_DECL_UID and thus
>>> label_to_block_map dense. But it does look like a bit of
>>> an chicken-and-egg problem and the question is how effective the
>>> dead label removal is in practice.
>>
>> Tried to add function compute_target_labels(not sure whether the function
>> name is suitable) in the front of make_blocks_1, now the fortran case doesn't
>> create/removing blocks now, but I still have several questions:
>>
>> 1. I used hash_set<tree> to save the target labels instead of bitmap, as
>> labels
>> are tree type value instead of block index so bitmap is not good for it since
>> we don't have LABEL_DECL_UID now?
>
> We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
> hash_set<tree> vs. bitmap is somewhat arbitrary here. The real cost is
> the extra walk over all stmts.
>
>> 2. Is the compute_target_labels still only for !optimize? And if we compute
>> the target labels before create bbs, it is unnessary to guard the first
>> cleanup_dead_labels under !optimize now, because the switch-case-do-while
>> case already create new block for CASE_LABEL already.
>
> OK.
>
>> 3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
>> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
>> labels_eh?
>
> I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
> coverage already?
Added in patch v4.
>
>> PS1: The v3 patch will cause one test case fail:
>>
>> Number of regressions in total: 1
>>> FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
>>> errors)
>>
>> due to this exausting case has labels from L0 to L100001, they won't be
>> optimized
>> to a simple if-else expression like before...
>
> Hmm, that's somewhat unexpected.
It could be fixed by not start a new block if two locus are on same line as the
labels are expanded by MACRO with same location info. BTW, I found that two
UNKOWN_LOCATION variable may have different value but return true in same_line_p?
2: locus1 = 2147483670
3: locus2 = 2147483652
(gdb) pel locus1
{file = 0x0, line = 0, column = 0, data = 0x7ffff6bdc300, sysp = false}
(gdb) pel locus2
{file = 0x0, line = 0, column = 0, data = 0x7ffff6bdc4e0, sysp = false}
(gdb) p LOCATION_LOCUS (locus1)
$16 = 0
(gdb) p LOCATION_LOCUS (locus2)
$17 = 0
So fix the function like this?
@@ -1152,6 +1218,10 @@ same_line_p (location_t locus1, expanded_location *from, location_t locus2)
{
expanded_location to;
+ if (LOCATION_LOCUS (locus1) == UNKNOWN_LOCATION
+ && LOCATION_LOCUS (locus2) == UNKNOWN_LOCATION)
+ return false;
+
if (locus1 == locus2)
return true;
>
>>
>> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
>> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it
>> so
>> just skip these label...
>
> Please investigate, we might be missing a corner case here.
Yes. Take the case pointer_array_1.f90 as example, it has an UNUSED label "L.7"
with locus info in it, not sure why it exists even since .original.
[pointer_array_1.f90:39:10] if (test.14 != 0) goto <D.4599>; els
e goto <D.4600>;
<D.4599>:
[pointer_array_1.f90:39:52] _gfortran_stop_numeric (3, 0);
<D.4600>:
parm.16 = {CLOBBER(eol)};
[pointer_array_1.f90:39:52] L.7: <= UNUSED label
<D.4594>:
[pointer_array_1.f90:39:52] L.3:
atmp.0 = {CLOBBER(eol)};
A.1 = {CLOBBER(eol)};
atmp.5 = {CLOBBER(eol)};
A.6 = {CLOBBER(eol)};
d = {CLOBBER(eol)};
[pointer_array_1.f90:41:14] return;
stmt_starts_bb_p will return true for L.7 as the prev_stmt "parm.16 = {CLOBBER(eol)};"
is not a label statement, then <D.4594> will also return true in stmt_starts_bb_p as
the label_stmt and prev_stmt are NOT on same line.
<bb 38> :
L.9:
L.8:
if (test.14 != 0) goto <L39>; else goto <L40>;
<bb 39> :
<L39>:
_gfortran_stop_numeric (3, 0);
<bb 40> :
<L40>:
parm.16 = {CLOBBER(eol)};
<bb 41> : <= empty block
L.7:
<bb 42> :
<L42>:
L.3:
atmp.0 = {CLOBBER(eol)};
A.1 = {CLOBBER(eol)};
atmp.5 = {CLOBBER(eol)};
A.6 = {CLOBBER(eol)};
d = {CLOBBER(eol)};
return;
So I have to fix it with this: Return false to not start a new block when prev_stmt
is not a label_statement and target_labels not containing the current label:
static inline bool stmt_starts_bb_p(gimple *stmt, gimple *prev_stmt, hash_set<tree> *target_labels)
{
...
if (glabel *plabel = safe_dyn_cast <glabel *> (prev_stmt))
{
...
cfg_stats.num_merged_labels++;
return false;
}
+ else if (!optimize
+ && !target_labels->contains (gimple_label_label (label_stmt))) <= Fix gfortran.dg failure
+ && stmt->next && gimple_code (stmt->next) == GIMPLE_LABEL) <= Fix bootstrap failure of openacc.f90
+ return false;
else
return true;
...
}
Though this fix would caused stage3 bootstrap failure quite like the fortran falure::
/data/src/gcc/libgomp/openacc.f90:1058:36:
1058 | subroutine acc_get_property_string_h (devicenum, devicetype, property, string)
| ^
Error: label ‘L.3’ in the middle of basic block 13
<bb 12> :
D.4377 = i > D.4374;
if (D.4377 != 0)
goto <bb 14>; [INV]
else
goto <bb 13>; [INV]
<bb 13> :
_10 = sptr.data;
_11 = sptr.offset;
_12 = i + _11;
_13 = sptr.span;
_14 = _12 * _13;
_15 = (sizetype) _14;
_16 = _10 + _15;
_17 = MEM[(character(kind=1) *)_16];
(*string)[i]{lb: 1 sz: 1} = _17;
L.3: <= UNUSED label.
i = i + 1;
goto <bb 12>; [INV]
<bb 14> :
sptr = {CLOBBER(eol)};
<bb 15> :
<L18>:
return;
<bb 1> :
>
>>
>> + case GIMPLE_GOTO:
>> +#if 0
>> + if (!computed_goto_p (stmt))
>> + {
>> + tree dest = gimple_goto_dest (stmt);
>> + target_labels->add (dest);
>> + }
>> +#endif
>> + break;
>>
>> Change the #if 0 to #if 1 result in:
>>
>> Number of regressions in total: 8
>>> FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
>>> FAIL: errors)
>>> FAIL: gcc.dg/analyzer/explode-2a.c (test for excess errors)
>>> FAIL: gcc.dg/analyzer/pragma-2.c (test for excess errors)
>>> FAIL: gfortran.dg/bound_2.f90 -O0 execution test
>>> FAIL: gfortran.dg/bound_7.f90 -O0 execution test
>>> FAIL: gfortran.dg/char_result_14.f90 -O0 execution test
>>> FAIL: gfortran.dg/pointer_array_1.f90 -O0 execution test
>>> FAIL: gfortran.dg/select_type_15.f03 -O0 execution test
>>
>>
>>
>> Paste the updated patch v3:
>
> The gcov testcase adjustments look good, does the analyzer testcase
> (missing in the changelog) get different CFG input?
>
Yes, it was different. But the analyzer case recovered with the same_line_p check.
One thing to note is that gimple_set_bb would refresh Label uid in it,
so target_labels need also update after it to follow that change?
@@ -566,6 +632,9 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
codes. */
gimple_set_bb (stmt, bb);
+ if (!optimize && gimple_code (stmt) == GIMPLE_LABEL)
+ target_labels.add (gimple_label_label (as_a<glabel *> (stmt)));
Another thing is the v4 patch will caused strange failures on libgomp.fortran testsuites.
It only happens when running "make check" but doesn't fail on single run, and more strangely,
the RUNTESTFLAGS doesn't work when running, it runs all cases under libgomp.fortran instead
of vla1.f90, which makes it difficult to analyze the failures...
make check-fortran RUNTESTFLAGS="fortran.exp=vla1.f90 -v -v" -j8
single run success:
/data/./gcc/xgcc -B/data/./gcc/ -B/data/install/x86_64-linux-gnu/bin/ -B/data/install/x86_64-linux-gnu/lib/ -isystem /data/install/x86_64-linux-gnu/include -isystem /data/install/x86_64-linux-gnu/sys-include /data/RocksDB_Docker/tgcc-master/libgomp/testsuite/libgomp.fortran/vla1.f90 -B/data/RocksDB_Docker/debug_master/x86_64-linux-gnu/./libgomp/ -B/data/x86_64-linux-gnu/./libgomp/.libs -I/data/x86_64-linux-gnu/./libgomp -I/data/RocksDB_Docker/tgcc-master/libgomp/testsuite/../../include -I/data/RocksDB_Docker/tgcc-master/libgomp/testsuite/.. -fmessage-length=0 -fno-diagnostics-show-caret -fdiagnostics-color=never -fopenmp -B/data/x86_64-linux-gnu/./libgomp/../libquadmath/.libs/ -O0 -B/data/x86_64-linux-gnu/./libgomp/../libgfortran/.libs -fintrinsic-modules-path=/data/x86_64-linux-gnu/./libgomp -L/data/x86_64-linux-gnu/./libgomp/.libs -L/data/x86_64-linux-gnu/./libgomp/../libquadmath/.libs/ -L/data/x86_64-linux-gnu/./libgomp/../libgfortran/.libs -lgfortran -foffload=-lgfortran -lquadmath -lm -o ./vla1.exe
FAIL: libgomp.fortran/appendix-a/a.4.1.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/collapse2.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/task2.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/udr13.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/udr3.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/udr4.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/vla1.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/vla2.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/vla3.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/vla4.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/vla5.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/vla6.f90 -O0 (test for excess errors)
FAIL: libgomp.fortran/vla8.f90 -O0 (test for excess errors)
FAIL: libgomp.oacc-fortran/collapse-2.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable -O0 (test for excess errors)
FAIL: libgomp.oacc-fortran/nested-function-1.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable -O0 (test for excess errors)
UNRESOLVED: libgomp.fortran/appendix-a/a.4.1.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/collapse2.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/task2.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/udr13.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/udr3.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/udr4.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla1.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla2.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla3.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla4.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla5.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla6.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla8.f90 -O0 compilation failed to produce executable
UNRESOLVED: libgomp.oacc-fortran/collapse-2.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable -O0 compilation failed to produce
executable
UNRESOLVED: libgomp.oacc-fortran/nested-function-1.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable -O0 compilation failed to
produce executable
v4: Address comments.
4.1. Handle GIMPLE_GOTO and GIMPLE_ASM.
4.2. Fix failure of limit-caselabels.c (labels on same line),
pointer_array_1.f90 (unused labels) etc.
v3: Add compute_target_labels and call it in the front of make_blocks_1.
v2: Check whether two locus are on same line.
Start a new basic block if two labels have different location when
test-coverage.
Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
master?
gcc/ChangeLog:
PR gcov/93680
* tree-cfg.cc (stmt_starts_bb_p): Check whether the label is in
target_labels.
(compute_target_labels): New function.
(make_blocks_1): Call compute_target_labels.
(same_line_p): Return false if two locus are both
UNKOWN_LOCATION.
gcc/testsuite/ChangeLog:
PR gcov/93680
* g++.dg/gcov/gcov-1.C: Correct counts.
* gcc.misc-tests/gcov-4.c: Likewise.
* gcc.misc-tests/gcov-pr85332.c: Likewise.
* lib/gcov.exp: Also clean gcda if fail.
* gcc.misc-tests/gcov-pr93680.c: New test.
Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
---
gcc/tree-cfg.cc | 91 ++++++++++++++++++++-
gcc/testsuite/g++.dg/gcov/gcov-1.C | 2 +-
gcc/testsuite/gcc.misc-tests/gcov-4.c | 2 +-
gcc/testsuite/gcc.misc-tests/gcov-pr85332.c | 2 +-
gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 ++++++
gcc/testsuite/lib/gcov.exp | 4 +-
6 files changed, 116 insertions(+), 9 deletions(-)
create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index a9fcc7fd050..d7ce121713e 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -164,7 +164,7 @@ static edge gimple_redirect_edge_and_branch (edge, basic_block);
static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
/* Various helpers. */
-static inline bool stmt_starts_bb_p (gimple *, gimple *);
+static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set<tree> *);
static int gimple_verify_flow_info (void);
static void gimple_make_forwarder_block (edge);
static gimple *first_non_label_stmt (basic_block);
@@ -521,6 +521,68 @@ gimple_call_initialize_ctrl_altering (gimple *stmt)
gimple_call_set_ctrl_altering (stmt, false);
}
+/* Compute target labels to save useful labels. */
+static void
+compute_target_labels (gimple_seq seq, hash_set<tree> *target_labels)
+{
+ gimple *stmt = NULL;
+ gimple_stmt_iterator j = gsi_start (seq);
+
+ while (!gsi_end_p (j))
+ {
+ stmt = gsi_stmt (j);
+
+ switch (gimple_code (stmt))
+ {
+ case GIMPLE_COND:
+ {
+ gcond *cstmt = as_a <gcond *> (stmt);
+ tree true_label = gimple_cond_true_label (cstmt);
+ tree false_label = gimple_cond_false_label (cstmt);
+ target_labels->add (true_label);
+ target_labels->add (false_label);
+ }
+ break;
+ case GIMPLE_SWITCH:
+ {
+ gswitch *gstmt = as_a <gswitch *> (stmt);
+ size_t i, n = gimple_switch_num_labels (gstmt);
+ tree elt, label;
+ for (i = 0; i < n; i++)
+ {
+ elt = gimple_switch_label (gstmt, i);
+ label = CASE_LABEL (elt);
+ target_labels->add (label);
+ }
+ }
+ break;
+ case GIMPLE_GOTO:
+ if (!computed_goto_p (stmt))
+ {
+ tree dest = gimple_goto_dest (stmt);
+ target_labels->add (dest);
+ }
+ break;
+ case GIMPLE_ASM:
+ {
+ gasm *asm_stmt = as_a <gasm *> (stmt);
+ int i, n = gimple_asm_nlabels (asm_stmt);
+ for (i = 0; i < n; ++i)
+ {
+ tree cons = gimple_asm_label_op (asm_stmt, i);
+ target_labels->add (cons);
+ }
+ }
+ break;
+
+ default:
+ break;
+ }
+
+ gsi_next (&j);
+ }
+}
+
/* Insert SEQ after BB and build a flowgraph. */
@@ -532,6 +594,10 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
gimple *prev_stmt = NULL;
bool start_new_block = true;
bool first_stmt_of_seq = true;
+ hash_set<tree> target_labels;
+
+ if (!optimize)
+ compute_target_labels (seq, &target_labels);
while (!gsi_end_p (i))
{
@@ -553,7 +619,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
/* If the statement starts a new basic block or if we have determined
in a previous pass that we need to create a new block for STMT, do
so now. */
- if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt))
+ if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt, &target_labels))
{
if (!first_stmt_of_seq)
gsi_split_seq_before (&i, &seq);
@@ -566,6 +632,9 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
codes. */
gimple_set_bb (stmt, bb);
+ if (!optimize && gimple_code (stmt) == GIMPLE_LABEL)
+ target_labels.add (gimple_label_label (as_a<glabel *> (stmt)));
+
/* If STMT is a basic block terminator, set START_NEW_BLOCK for the
next iteration. */
if (stmt_ends_bb_p (stmt))
@@ -2832,7 +2901,8 @@ simple_goto_p (gimple *t)
label. */
static inline bool
-stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
+stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt,
+ hash_set<tree> *target_labels)
{
if (stmt == NULL)
return false;
@@ -2860,9 +2930,24 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
|| !DECL_ARTIFICIAL (gimple_label_label (plabel)))
return true;
+ location_t prev_locus = gimple_location (plabel);
+ location_t locus = gimple_location (label_stmt);
+ expanded_location locus_e = expand_location (locus);
+
+ if (!optimize
+ && target_labels->contains (gimple_label_label (label_stmt))
+ && (LOCATION_LOCUS (locus) != UNKNOWN_LOCATION
+ || LOCATION_LOCUS (prev_locus) != UNKNOWN_LOCATION)
+ && !same_line_p (locus, &locus_e, prev_locus))
+ return true;
+
cfg_stats.num_merged_labels++;
return false;
}
+ else if (!optimize
+ && !target_labels->contains (gimple_label_label (label_stmt))
+ && stmt->next && gimple_code (stmt->next) == GIMPLE_LABEL)
+ return false;
else
return true;
}
diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
index ee383b480a8..01e7084fb03 100644
--- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
+++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
@@ -263,7 +263,7 @@ test_switch (int i, int j)
case 2:
result = do_something (1024);
break;
- case 3: /* count(3) */
+ case 3: /* count(2) */
case 4:
/* branch(67) */
if (j == 2) /* count(3) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
index da7929ef7fc..792cda8cfce 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
@@ -122,7 +122,7 @@ top:
}
else
{
-else_: /* count(1) */
+else_: /* count(2) */
j = do_something (j); /* count(2) */
if (j) /* count(2) */
{
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
index 73e50b19fc7..b37e760910c 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
@@ -7,7 +7,7 @@ int doit(int sel, int n, void *p)
switch (sel)
{
- case 0: /* count(3) */
+ case 0: /* count(1) */
do {*p0 += *p0;} while (--n); /* count(3) */
return *p0 == 0; /* count(1) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
new file mode 100644
index 00000000000..2fe340c4011
--- /dev/null
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
@@ -0,0 +1,24 @@
+/* { dg-options "-fprofile-arcs -ftest-coverage" } */
+/* { dg-do run { target native } } */
+
+int f(int s, int n)
+{
+ int p = 0;
+
+ switch (s)
+ {
+ case 0: /* count(1) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+
+ case 1: /* count(1) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+ }
+
+ return 0;
+}
+
+int main() { f(0, 5); f(1, 5); return 0; }
+
+/* { dg-final { run-gcov gcov-pr93680.c } } */
diff --git a/gcc/testsuite/lib/gcov.exp b/gcc/testsuite/lib/gcov.exp
index 80e74aeb220..07e1978d25d 100644
--- a/gcc/testsuite/lib/gcov.exp
+++ b/gcc/testsuite/lib/gcov.exp
@@ -424,9 +424,7 @@ proc run-gcov { args } {
}
if { $tfailed > 0 } {
fail "$testname gcov: $lfailed failures in line counts, $bfailed in branch percentages, $cfailed in return percentages, $ifailed in intermediate format"
- if { $xfailed } {
- clean-gcov $testcase
- }
+ clean-gcov $testcase
} else {
pass "$testname gcov"
clean-gcov $testcase
--
2.27.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-09 12:02 ` Richard Biener
2023-03-14 2:06 ` [PATCH v4] " Xionghu Luo
@ 2023-03-15 10:07 ` Xionghu Luo
1 sibling, 0 replies; 19+ messages in thread
From: Xionghu Luo @ 2023-03-15 10:07 UTC (permalink / raw)
To: Richard Biener; +Cc: Richard Biener, gcc-patches, luoxhu, hubicka
[-- Attachment #1: Type: text/plain, Size: 4007 bytes --]
On 2023/3/9 20:02, Richard Biener wrote:
> On Wed, 8 Mar 2023, Xionghu Luo wrote:
>
>>
>>
>> On 2023/3/7 19:25, Richard Biener wrote:
>>>>> It would be nice to avoid creating blocks / preserving labels we'll
>>>>> immediately remove again. For that we do need some analysis
>>>>> before creating basic-blocks that determines whether a label is
>>>>> possibly reached by a non-falltru edge.
>>>>>
>>>>
>>>> <bb 2> :
>>>> p = 0;
>>>> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
>>>>
>>>> <bb 3> :
>>>> <L0>: <= prev_stmt
>>>> <D.2748>: <= stmt
>>>> p = p + 1;
>>>> n = n + -1;
>>>> if (n != 0) goto <D.2748>; else goto <D.2746>;
>>>>
>>>> Check if <L0> is a case label and <D.2748> is a goto target then return
>>>> true
>>>> in stmt_starts_bb_p to start a new basic block? This would avoid creating
>>>> and
>>>> removing blocks, but cleanup_dead_labels has all bbs setup while
>>>> stmt_starts_bb_p
>>>> does't yet to iterate bbs/labels to establish label_for_bb[] map?
>>
>>> Yes. I think we'd need something more pragmatic before make_blocks (),
>>> like re-computing TREE_USED of the label decls or computing a bitmap
>>> of targeted labels (targeted by goto, switch or any other means).
>>>
>>> I'll note that doing a cleanup_dead_labels () like optimization before
>>> we create blocks will help keeping LABEL_DECL_UID and thus
>>> label_to_block_map dense. But it does look like a bit of
>>> an chicken-and-egg problem and the question is how effective the
>>> dead label removal is in practice.
>>
>> Tried to add function compute_target_labels(not sure whether the function
>> name is suitable) in the front of make_blocks_1, now the fortran case doesn't
>> create/removing blocks now, but I still have several questions:
>>
>> 1. I used hash_set<tree> to save the target labels instead of bitmap, as
>> labels
>> are tree type value instead of block index so bitmap is not good for it since
>> we don't have LABEL_DECL_UID now?
>
> We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
> hash_set<tree> vs. bitmap is somewhat arbitrary here. The real cost is
> the extra walk over all stmts.
>
>> 2. Is the compute_target_labels still only for !optimize? And if we compute
>> the target labels before create bbs, it is unnessary to guard the first
>> cleanup_dead_labels under !optimize now, because the switch-case-do-while
>> case already create new block for CASE_LABEL already.
>
> OK.
>
>> 3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
>> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
>> labels_eh?
>
> I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
> coverage already?
>
>> PS1: The v3 patch will cause one test case fail:
>>
>> Number of regressions in total: 1
>>> FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
>>> errors)
>>
>> due to this exausting case has labels from L0 to L100001, they won't be
>> optimized
>> to a simple if-else expression like before...
>
> Hmm, that's somewhat unexpected.
>
>>
>> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
>> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it
>> so
>> just skip these label...
>
> Please investigate, we might be missing a corner case here.
>
I think the *previous fix* for labels “in the middle of block” is *incorrect*, it should
be handled in make_edges_bb when a basic block only has Label in it, just create a
fallthrough edge for it to avoid wrong cfg and unreachable trap generated?
@@ -853,6 +922,12 @@ make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index)
bool fallthru = false;
int ret = 0;
+ if (!optimize && !last)
+ {
+ make_edge (bb, bb->next_bb, EDGE_FALLTHRU);
+ return 0;
+ }
+
if (!last)
return ret;
With the fix, the attached version could pass bootstrap and regression test on x86_64-linux-gnu.
[-- Attachment #2: v4-0001-gcov-Fix-do-while-structure-in-case-statement-lea.patch --]
[-- Type: text/plain, Size: 9190 bytes --]
From ec505cc7952707db805802af83dd82776a1d949f Mon Sep 17 00:00:00 2001
From: Xionghu Luo <xionghuluo@tencent.com>
Date: Tue, 28 Feb 2023 17:46:18 +0800
Subject: [PATCH v4] gcov: Fix "do-while" structure in case statement leads to
incorrect code coverage [PR93680]
v4: Address comments.
4.1. Handle GIMPLE_GOTO and GIMPLE_ASM.
4.2. Fix failure of limit-caselabels.c (labels on same line),
pointer_array_1.f90 (unused labels) etc.
v3: Add compute_target_labels and call it in the front of make_blocks_1.
v2: Check whether two locus are on same line.
Start a new basic block if two labels have different location when
test-coverage.
Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
master?
gcc/ChangeLog:
PR gcov/93680
* tree-cfg.cc (stmt_starts_bb_p): Check whether the label is in
target_labels.
(compute_target_labels): New function.
(make_blocks_1): Call compute_target_labels.
(same_line_p): Return false if two locus are both
UNKOWN_LOCATION.
gcc/testsuite/ChangeLog:
PR gcov/93680
* g++.dg/gcov/gcov-1.C: Correct counts.
* gcc.misc-tests/gcov-4.c: Likewise.
* gcc.misc-tests/gcov-pr85332.c: Likewise.
* lib/gcov.exp: Also clean gcda if fail.
* gcc.misc-tests/gcov-pr93680.c: New test.
Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
---
gcc/tree-cfg.cc | 97 ++++++++++++++++++++-
gcc/testsuite/g++.dg/gcov/gcov-1.C | 2 +-
gcc/testsuite/gcc.misc-tests/gcov-4.c | 2 +-
gcc/testsuite/gcc.misc-tests/gcov-pr85332.c | 2 +-
gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 +++++
gcc/testsuite/lib/gcov.exp | 4 +-
6 files changed, 122 insertions(+), 9 deletions(-)
create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index a9fcc7fd050..7b185200aaf 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -164,7 +164,7 @@ static edge gimple_redirect_edge_and_branch (edge, basic_block);
static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
/* Various helpers. */
-static inline bool stmt_starts_bb_p (gimple *, gimple *);
+static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set<tree> *);
static int gimple_verify_flow_info (void);
static void gimple_make_forwarder_block (edge);
static gimple *first_non_label_stmt (basic_block);
@@ -521,6 +521,68 @@ gimple_call_initialize_ctrl_altering (gimple *stmt)
gimple_call_set_ctrl_altering (stmt, false);
}
+/* Compute target labels to save useful labels. */
+static void
+compute_target_labels (gimple_seq seq, hash_set<tree> *target_labels)
+{
+ gimple *stmt = NULL;
+ gimple_stmt_iterator j = gsi_start (seq);
+
+ while (!gsi_end_p (j))
+ {
+ stmt = gsi_stmt (j);
+
+ switch (gimple_code (stmt))
+ {
+ case GIMPLE_COND:
+ {
+ gcond *cstmt = as_a <gcond *> (stmt);
+ tree true_label = gimple_cond_true_label (cstmt);
+ tree false_label = gimple_cond_false_label (cstmt);
+ target_labels->add (true_label);
+ target_labels->add (false_label);
+ }
+ break;
+ case GIMPLE_SWITCH:
+ {
+ gswitch *gstmt = as_a <gswitch *> (stmt);
+ size_t i, n = gimple_switch_num_labels (gstmt);
+ tree elt, label;
+ for (i = 0; i < n; i++)
+ {
+ elt = gimple_switch_label (gstmt, i);
+ label = CASE_LABEL (elt);
+ target_labels->add (label);
+ }
+ }
+ break;
+ case GIMPLE_GOTO:
+ if (!computed_goto_p (stmt))
+ {
+ tree dest = gimple_goto_dest (stmt);
+ target_labels->add (dest);
+ }
+ break;
+ case GIMPLE_ASM:
+ {
+ gasm *asm_stmt = as_a <gasm *> (stmt);
+ int i, n = gimple_asm_nlabels (asm_stmt);
+ for (i = 0; i < n; ++i)
+ {
+ tree cons = gimple_asm_label_op (asm_stmt, i);
+ target_labels->add (cons);
+ }
+ }
+ break;
+
+ default:
+ break;
+ }
+
+ gsi_next (&j);
+ }
+}
+
/* Insert SEQ after BB and build a flowgraph. */
@@ -532,6 +594,10 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
gimple *prev_stmt = NULL;
bool start_new_block = true;
bool first_stmt_of_seq = true;
+ hash_set<tree> target_labels;
+
+ if (!optimize)
+ compute_target_labels (seq, &target_labels);
while (!gsi_end_p (i))
{
@@ -553,7 +619,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
/* If the statement starts a new basic block or if we have determined
in a previous pass that we need to create a new block for STMT, do
so now. */
- if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt))
+ if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt, &target_labels))
{
if (!first_stmt_of_seq)
gsi_split_seq_before (&i, &seq);
@@ -566,6 +632,9 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
codes. */
gimple_set_bb (stmt, bb);
+ if (!optimize && gimple_code (stmt) == GIMPLE_LABEL)
+ target_labels.add (gimple_label_label (as_a<glabel *> (stmt)));
+
/* If STMT is a basic block terminator, set START_NEW_BLOCK for the
next iteration. */
if (stmt_ends_bb_p (stmt))
@@ -853,6 +922,12 @@ make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index)
bool fallthru = false;
int ret = 0;
+ if (!optimize && !last)
+ {
+ make_edge (bb, bb->next_bb, EDGE_FALLTHRU);
+ return 0;
+ }
+
if (!last)
return ret;
@@ -1152,6 +1227,10 @@ same_line_p (location_t locus1, expanded_location *from, location_t locus2)
{
expanded_location to;
+ if (LOCATION_LOCUS (locus1) == UNKNOWN_LOCATION
+ && LOCATION_LOCUS (locus2) == UNKNOWN_LOCATION)
+ return false;
+
if (locus1 == locus2)
return true;
@@ -2832,7 +2911,8 @@ simple_goto_p (gimple *t)
label. */
static inline bool
-stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
+stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt,
+ hash_set<tree> *target_labels)
{
if (stmt == NULL)
return false;
@@ -2860,6 +2940,17 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
|| !DECL_ARTIFICIAL (gimple_label_label (plabel)))
return true;
+ location_t prev_locus = gimple_location (plabel);
+ location_t locus = gimple_location (label_stmt);
+ expanded_location locus_e = expand_location (locus);
+
+ if (!optimize
+ && target_labels->contains (gimple_label_label (label_stmt))
+ && (LOCATION_LOCUS (locus) != UNKNOWN_LOCATION
+ || LOCATION_LOCUS (prev_locus) != UNKNOWN_LOCATION)
+ && !same_line_p (locus, &locus_e, prev_locus))
+ return true;
+
cfg_stats.num_merged_labels++;
return false;
}
diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
index ee383b480a8..01e7084fb03 100644
--- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
+++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
@@ -263,7 +263,7 @@ test_switch (int i, int j)
case 2:
result = do_something (1024);
break;
- case 3: /* count(3) */
+ case 3: /* count(2) */
case 4:
/* branch(67) */
if (j == 2) /* count(3) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
index da7929ef7fc..792cda8cfce 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
@@ -122,7 +122,7 @@ top:
}
else
{
-else_: /* count(1) */
+else_: /* count(2) */
j = do_something (j); /* count(2) */
if (j) /* count(2) */
{
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
index 73e50b19fc7..b37e760910c 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
@@ -7,7 +7,7 @@ int doit(int sel, int n, void *p)
switch (sel)
{
- case 0: /* count(3) */
+ case 0: /* count(1) */
do {*p0 += *p0;} while (--n); /* count(3) */
return *p0 == 0; /* count(1) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
new file mode 100644
index 00000000000..2fe340c4011
--- /dev/null
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
@@ -0,0 +1,24 @@
+/* { dg-options "-fprofile-arcs -ftest-coverage" } */
+/* { dg-do run { target native } } */
+
+int f(int s, int n)
+{
+ int p = 0;
+
+ switch (s)
+ {
+ case 0: /* count(1) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+
+ case 1: /* count(1) */
+ do { p++; } while (--n); /* count(5) */
+ return p; /* count(1) */
+ }
+
+ return 0;
+}
+
+int main() { f(0, 5); f(1, 5); return 0; }
+
+/* { dg-final { run-gcov gcov-pr93680.c } } */
diff --git a/gcc/testsuite/lib/gcov.exp b/gcc/testsuite/lib/gcov.exp
index 80e74aeb220..07e1978d25d 100644
--- a/gcc/testsuite/lib/gcov.exp
+++ b/gcc/testsuite/lib/gcov.exp
@@ -424,9 +424,7 @@ proc run-gcov { args } {
}
if { $tfailed > 0 } {
fail "$testname gcov: $lfailed failures in line counts, $bfailed in branch percentages, $cfailed in return percentages, $ifailed in intermediate format"
- if { $xfailed } {
- clean-gcov $testcase
- }
+ clean-gcov $testcase
} else {
pass "$testname gcov"
clean-gcov $testcase
--
2.27.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
2023-03-14 2:06 ` [PATCH v4] " Xionghu Luo
@ 2023-03-21 11:18 ` Richard Biener
0 siblings, 0 replies; 19+ messages in thread
From: Richard Biener @ 2023-03-21 11:18 UTC (permalink / raw)
To: Xionghu Luo; +Cc: Richard Biener, gcc-patches, luoxhu, hubicka
[-- Attachment #1: Type: text/plain, Size: 24457 bytes --]
On Tue, 14 Mar 2023, Xionghu Luo wrote:
>
>
> On 2023/3/9 20:02, Richard Biener wrote:
> > On Wed, 8 Mar 2023, Xionghu Luo wrote:
> >
> >>
> >>
> >> On 2023/3/7 19:25, Richard Biener wrote:
> >>>>> It would be nice to avoid creating blocks / preserving labels we'll
> >>>>> immediately remove again. For that we do need some analysis
> >>>>> before creating basic-blocks that determines whether a label is
> >>>>> possibly reached by a non-falltru edge.
> >>>>>
> >>>>
> >>>> <bb 2> :
> >>>> p = 0;
> >>>> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
> >>>>
> >>>> <bb 3> :
> >>>> <L0>: <= prev_stmt
> >>>> <D.2748>: <= stmt
> >>>> p = p + 1;
> >>>> n = n + -1;
> >>>> if (n != 0) goto <D.2748>; else goto <D.2746>;
> >>>>
> >>>> Check if <L0> is a case label and <D.2748> is a goto target then return
> >>>> true
> >>>> in stmt_starts_bb_p to start a new basic block? This would avoid
> >>>> creating
> >>>> and
> >>>> removing blocks, but cleanup_dead_labels has all bbs setup while
> >>>> stmt_starts_bb_p
> >>>> does't yet to iterate bbs/labels to establish label_for_bb[] map?
> >>
> >>> Yes. I think we'd need something more pragmatic before make_blocks (),
> >>> like re-computing TREE_USED of the label decls or computing a bitmap
> >>> of targeted labels (targeted by goto, switch or any other means).
> >>>
> >>> I'll note that doing a cleanup_dead_labels () like optimization before
> >>> we create blocks will help keeping LABEL_DECL_UID and thus
> >>> label_to_block_map dense. But it does look like a bit of
> >>> an chicken-and-egg problem and the question is how effective the
> >>> dead label removal is in practice.
> >>
> >> Tried to add function compute_target_labels(not sure whether the function
> >> name is suitable) in the front of make_blocks_1, now the fortran case
> >> doesn't
> >> create/removing blocks now, but I still have several questions:
> >>
> >> 1. I used hash_set<tree> to save the target labels instead of bitmap, as
> >> labels
> >> are tree type value instead of block index so bitmap is not good for it
> >> since
> >> we don't have LABEL_DECL_UID now?
> >
> > We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
> > hash_set<tree> vs. bitmap is somewhat arbitrary here. The real cost is
> > the extra walk over all stmts.
> >
> >> 2. Is the compute_target_labels still only for !optimize? And if we
> >> compute
> >> the target labels before create bbs, it is unnessary to guard the first
> >> cleanup_dead_labels under !optimize now, because the switch-case-do-while
> >> case already create new block for CASE_LABEL already.
> >
> > OK.
> >
> >> 3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
> >> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
> >> labels_eh?
> >
> > I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
> > coverage already?
>
> Added in patch v4.
>
> >
> >> PS1: The v3 patch will cause one test case fail:
> >>
> >> Number of regressions in total: 1
> >>> FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
> >>> errors)
> >>
> >> due to this exausting case has labels from L0 to L100001, they won't be
> >> optimized
> >> to a simple if-else expression like before...
> >
> > Hmm, that's somewhat unexpected.
>
> It could be fixed by not start a new block if two locus are on same line as
> the
> labels are expanded by MACRO with same location info. BTW, I found that two
> UNKOWN_LOCATION variable may have different value but return true in
> same_line_p?
Yes, the raw location value also encodes other info so only
LOCATION_LOCUS (loc) will be equal to UNKNOWN_LOCATION. There's some
existing inconsistency in whether LOCATION_LOCUS or raw locus is
compared.
> 2: locus1 = 2147483670
> 3: locus2 = 2147483652
> (gdb) pel locus1
> {file = 0x0, line = 0, column = 0, data = 0x7ffff6bdc300, sysp = false}
> (gdb) pel locus2
> {file = 0x0, line = 0, column = 0, data = 0x7ffff6bdc4e0, sysp = false}
> (gdb) p LOCATION_LOCUS (locus1)
> $16 = 0
> (gdb) p LOCATION_LOCUS (locus2)
> $17 = 0
>
> So fix the function like this?
>
> @@ -1152,6 +1218,10 @@ same_line_p (location_t locus1, expanded_location
> *from, location_t locus2)
> {
> expanded_location to;
>
> + if (LOCATION_LOCUS (locus1) == UNKNOWN_LOCATION
> + && LOCATION_LOCUS (locus2) == UNKNOWN_LOCATION)
> + return false;
> +
I think we want to treat two unknown locations as the same line, but for
consistency I'd change the following test to use LOCATION_LOCUS
> if (locus1 == locus2)
> return true;
>
> >
> >>
> >> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail
> >> due
> >> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into
> >> it
> >> so
> >> just skip these label...
> >
> > Please investigate, we might be missing a corner case here.
>
> Yes. Take the case pointer_array_1.f90 as example, it has an UNUSED label
> "L.7"
> with locus info in it, not sure why it exists even since .original.
>
>
> [pointer_array_1.f90:39:10] if (test.14 != 0) goto <D.4599>; els
> e goto <D.4600>;
> <D.4599>:
> [pointer_array_1.f90:39:52] _gfortran_stop_numeric (3, 0);
> <D.4600>:
> parm.16 = {CLOBBER(eol)};
> [pointer_array_1.f90:39:52] L.7: <= UNUSED label
> <D.4594>:
> [pointer_array_1.f90:39:52] L.3:
> atmp.0 = {CLOBBER(eol)};
> A.1 = {CLOBBER(eol)};
> atmp.5 = {CLOBBER(eol)};
> A.6 = {CLOBBER(eol)};
> d = {CLOBBER(eol)};
> [pointer_array_1.f90:41:14] return;
>
> stmt_starts_bb_p will return true for L.7 as the prev_stmt "parm.16 =
> {CLOBBER(eol)};"
> is not a label statement, then <D.4594> will also return true in
> stmt_starts_bb_p as
> the label_stmt and prev_stmt are NOT on same line.
>
> <bb 38> :
> L.9:
> L.8:
> if (test.14 != 0) goto <L39>; else goto <L40>;
>
> <bb 39> :
> <L39>:
> _gfortran_stop_numeric (3, 0);
>
> <bb 40> :
> <L40>:
> parm.16 = {CLOBBER(eol)};
>
> <bb 41> : <= empty block
> L.7:
So this is another case that's fixed by the make_edges_bb fix (I
see you followed up this mail with something similar).
Please use my original suggested fix though, do
if (!last)
fallthru = true;
else
switch (gimple_code (last))
{
...
instead of the if (!last) return ret, that should be done independent
of !optimize
> <bb 42> :
> <L42>:
> L.3:
> atmp.0 = {CLOBBER(eol)};
> A.1 = {CLOBBER(eol)};
> atmp.5 = {CLOBBER(eol)};
> A.6 = {CLOBBER(eol)};
> d = {CLOBBER(eol)};
> return;
>
>
> So I have to fix it with this: Return false to not start a new block when
> prev_stmt
> is not a label_statement and target_labels not containing the current label:
>
> static inline bool stmt_starts_bb_p(gimple *stmt, gimple *prev_stmt,
> hash_set<tree> *target_labels)
> {
> ...
> if (glabel *plabel = safe_dyn_cast <glabel *> (prev_stmt))
> {
> ...
> cfg_stats.num_merged_labels++;
> return false;
> }
> + else if (!optimize
> + && !target_labels->contains (gimple_label_label (label_stmt)))
> <= Fix gfortran.dg failure
> + && stmt->next && gimple_code (stmt->next) == GIMPLE_LABEL)
> <= Fix bootstrap failure of openacc.f90
> + return false;
> else
> return true;
> ...
> }
>
> Though this fix would caused stage3 bootstrap failure quite like the fortran
> falure::
>
> /data/src/gcc/libgomp/openacc.f90:1058:36:
>
> 1058 | subroutine acc_get_property_string_h (devicenum, devicetype, property,
> string)
> | ^
> Error: label ‘L.3’ in the middle of basic block 13
>
>
> <bb 12> :
> D.4377 = i > D.4374;
> if (D.4377 != 0)
> goto <bb 14>; [INV]
> else
> goto <bb 13>; [INV]
>
> <bb 13> :
> _10 = sptr.data;
> _11 = sptr.offset;
> _12 = i + _11;
> _13 = sptr.span;
> _14 = _12 * _13;
> _15 = (sizetype) _14;
> _16 = _10 + _15;
> _17 = MEM[(character(kind=1) *)_16];
> (*string)[i]{lb: 1 sz: 1} = _17;
> L.3: <= UNUSED label.
> i = i + 1;
> goto <bb 12>; [INV]
>
> <bb 14> :
> sptr = {CLOBBER(eol)};
>
> <bb 15> :
> <L18>:
> return;
>
> <bb 1> :
>
> >
> >>
> >> + case GIMPLE_GOTO:
> >> +#if 0
> >> + if (!computed_goto_p (stmt))
> >> + {
> >> + tree dest = gimple_goto_dest (stmt);
> >> + target_labels->add (dest);
> >> + }
> >> +#endif
> >> + break;
> >>
> >> Change the #if 0 to #if 1 result in:
> >>
> >> Number of regressions in total: 8
> >>> FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
> >>> FAIL: errors)
> >>> FAIL: gcc.dg/analyzer/explode-2a.c (test for excess errors)
> >>> FAIL: gcc.dg/analyzer/pragma-2.c (test for excess errors)
> >>> FAIL: gfortran.dg/bound_2.f90 -O0 execution test
> >>> FAIL: gfortran.dg/bound_7.f90 -O0 execution test
> >>> FAIL: gfortran.dg/char_result_14.f90 -O0 execution test
> >>> FAIL: gfortran.dg/pointer_array_1.f90 -O0 execution test
> >>> FAIL: gfortran.dg/select_type_15.f03 -O0 execution test
> >>
> >>
> >>
> >> Paste the updated patch v3:
> >
> > The gcov testcase adjustments look good, does the analyzer testcase
> > (missing in the changelog) get different CFG input?
> >
>
> Yes, it was different. But the analyzer case recovered with the same_line_p
> check.
>
>
> One thing to note is that gimple_set_bb would refresh Label uid in it,
> so target_labels need also update after it to follow that change?
>
>
> @@ -566,6 +632,9 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
> codes. */
> gimple_set_bb (stmt, bb);
>
> + if (!optimize && gimple_code (stmt) == GIMPLE_LABEL)
> + target_labels.add (gimple_label_label (as_a<glabel *> (stmt)));
>
>
> Another thing is the v4 patch will caused strange failures on libgomp.fortran
> testsuites.
> It only happens when running "make check" but doesn't fail on single run, and
> more strangely,
> the RUNTESTFLAGS doesn't work when running, it runs all cases under
> libgomp.fortran instead
> of vla1.f90, which makes it difficult to analyze the failures...
>
> make check-fortran RUNTESTFLAGS="fortran.exp=vla1.f90 -v -v" -j8
>
> single run success:
> /data/./gcc/xgcc -B/data/./gcc/ -B/data/install/x86_64-linux-gnu/bin/
> -B/data/install/x86_64-linux-gnu/lib/ -isystem
> /data/install/x86_64-linux-gnu/include -isystem
> /data/install/x86_64-linux-gnu/sys-include
> /data/RocksDB_Docker/tgcc-master/libgomp/testsuite/libgomp.fortran/vla1.f90
> -B/data/RocksDB_Docker/debug_master/x86_64-linux-gnu/./libgomp/
> -B/data/x86_64-linux-gnu/./libgomp/.libs -I/data/x86_64-linux-gnu/./libgomp
> -I/data/RocksDB_Docker/tgcc-master/libgomp/testsuite/../../include
> -I/data/RocksDB_Docker/tgcc-master/libgomp/testsuite/.. -fmessage-length=0
> -fno-diagnostics-show-caret -fdiagnostics-color=never -fopenmp
> -B/data/x86_64-linux-gnu/./libgomp/../libquadmath/.libs/ -O0
> -B/data/x86_64-linux-gnu/./libgomp/../libgfortran/.libs
> -fintrinsic-modules-path=/data/x86_64-linux-gnu/./libgomp
> -L/data/x86_64-linux-gnu/./libgomp/.libs
> -L/data/x86_64-linux-gnu/./libgomp/../libquadmath/.libs/
> -L/data/x86_64-linux-gnu/./libgomp/../libgfortran/.libs -lgfortran
> -foffload=-lgfortran -lquadmath -lm -o ./vla1.exe
>
> FAIL: libgomp.fortran/appendix-a/a.4.1.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/collapse2.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/task2.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/udr13.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/udr3.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/udr4.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/vla1.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/vla2.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/vla3.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/vla4.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/vla5.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/vla6.f90 -O0 (test for excess errors)
> FAIL: libgomp.fortran/vla8.f90 -O0 (test for excess errors)
> FAIL: libgomp.oacc-fortran/collapse-2.f90 -DACC_DEVICE_TYPE_host=1
> FAIL: -DACC_MEM_SHARED=1 -foffload=disable -O0 (test for excess errors)
> FAIL: libgomp.oacc-fortran/nested-function-1.f90 -DACC_DEVICE_TYPE_host=1
> FAIL: -DACC_MEM_SHARED=1 -foffload=disable -O0 (test for excess errors)
> UNRESOLVED: libgomp.fortran/appendix-a/a.4.1.f90 -O0 compilation failed to
> UNRESOLVED: produce executable
> UNRESOLVED: libgomp.fortran/collapse2.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/task2.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/udr13.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/udr3.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/udr4.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla1.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla2.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla3.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla4.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla5.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla6.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla8.f90 -O0 compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.oacc-fortran/collapse-2.f90 -DACC_DEVICE_TYPE_host=1
> UNRESOLVED: -DACC_MEM_SHARED=1 -foffload=disable -O0 compilation failed to
> UNRESOLVED: produce
> executable
> UNRESOLVED: libgomp.oacc-fortran/nested-function-1.f90
> -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable -O0
> compilation failed to
> produce executable
>
>
> v4: Address comments.
> 4.1. Handle GIMPLE_GOTO and GIMPLE_ASM.
> 4.2. Fix failure of limit-caselabels.c (labels on same line),
> pointer_array_1.f90 (unused labels) etc.
>
> v3: Add compute_target_labels and call it in the front of make_blocks_1.
> v2: Check whether two locus are on same line.
>
> Start a new basic block if two labels have different location when
> test-coverage.
>
> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
> master?
Note this has to wait for stage1 to open I think.
Some comments on the patch below
> gcc/ChangeLog:
>
> PR gcov/93680
> * tree-cfg.cc (stmt_starts_bb_p): Check whether the label is in
> target_labels.
> (compute_target_labels): New function.
> (make_blocks_1): Call compute_target_labels.
> (same_line_p): Return false if two locus are both
> UNKOWN_LOCATION.
>
> gcc/testsuite/ChangeLog:
>
> PR gcov/93680
> * g++.dg/gcov/gcov-1.C: Correct counts.
> * gcc.misc-tests/gcov-4.c: Likewise.
> * gcc.misc-tests/gcov-pr85332.c: Likewise.
> * lib/gcov.exp: Also clean gcda if fail.
> * gcc.misc-tests/gcov-pr93680.c: New test.
>
> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
> ---
> gcc/tree-cfg.cc | 91 ++++++++++++++++++++-
> gcc/testsuite/g++.dg/gcov/gcov-1.C | 2 +-
> gcc/testsuite/gcc.misc-tests/gcov-4.c | 2 +-
> gcc/testsuite/gcc.misc-tests/gcov-pr85332.c | 2 +-
> gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 ++++++
> gcc/testsuite/lib/gcov.exp | 4 +-
> 6 files changed, 116 insertions(+), 9 deletions(-)
> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
>
> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> index a9fcc7fd050..d7ce121713e 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -164,7 +164,7 @@ static edge gimple_redirect_edge_and_branch (edge,
> basic_block);
> static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
>
> /* Various helpers. */
> -static inline bool stmt_starts_bb_p (gimple *, gimple *);
> +static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set<tree> *);
> static int gimple_verify_flow_info (void);
> static void gimple_make_forwarder_block (edge);
> static gimple *first_non_label_stmt (basic_block);
> @@ -521,6 +521,68 @@ gimple_call_initialize_ctrl_altering (gimple *stmt)
> gimple_call_set_ctrl_altering (stmt, false);
> }
>
> +/* Compute target labels to save useful labels. */
> +static void
> +compute_target_labels (gimple_seq seq, hash_set<tree> *target_labels)
> +{
> + gimple *stmt = NULL;
> + gimple_stmt_iterator j = gsi_start (seq);
> +
> + while (!gsi_end_p (j))
> + {
> + stmt = gsi_stmt (j);
> +
> + switch (gimple_code (stmt))
> + {
> + case GIMPLE_COND:
> + {
> + gcond *cstmt = as_a <gcond *> (stmt);
> + tree true_label = gimple_cond_true_label (cstmt);
> + tree false_label = gimple_cond_false_label (cstmt);
> + target_labels->add (true_label);
> + target_labels->add (false_label);
> + }
> + break;
> + case GIMPLE_SWITCH:
> + {
> + gswitch *gstmt = as_a <gswitch *> (stmt);
> + size_t i, n = gimple_switch_num_labels (gstmt);
> + tree elt, label;
> + for (i = 0; i < n; i++)
> + {
> + elt = gimple_switch_label (gstmt, i);
> + label = CASE_LABEL (elt);
> + target_labels->add (label);
> + }
> + }
> + break;
> + case GIMPLE_GOTO:
> + if (!computed_goto_p (stmt))
> + {
> + tree dest = gimple_goto_dest (stmt);
> + target_labels->add (dest);
> + }
> + break;
> + case GIMPLE_ASM:
> + {
> + gasm *asm_stmt = as_a <gasm *> (stmt);
> + int i, n = gimple_asm_nlabels (asm_stmt);
> + for (i = 0; i < n; ++i)
> + {
> + tree cons = gimple_asm_label_op (asm_stmt, i);
> + target_labels->add (cons);
> + }
> + }
> + break;
> +
> + default:
> + break;
> + }
> +
> + gsi_next (&j);
> + }
> +}
> +
>
> /* Insert SEQ after BB and build a flowgraph. */
>
> @@ -532,6 +594,10 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
> gimple *prev_stmt = NULL;
> bool start_new_block = true;
> bool first_stmt_of_seq = true;
> + hash_set<tree> target_labels;
> +
> + if (!optimize)
> + compute_target_labels (seq, &target_labels);
>
> while (!gsi_end_p (i))
> {
> @@ -553,7 +619,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
> /* If the statement starts a new basic block or if we have determined
> in a previous pass that we need to create a new block for STMT, do
> so now. */
> - if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt))
> + if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt,
> &target_labels))
quoting your stmt_starts_bb_p change:
+ location_t prev_locus = gimple_location (plabel);
+ location_t locus = gimple_location (label_stmt);
+ expanded_location locus_e = expand_location (locus);
+
+ if (!optimize
+ && target_labels->contains (gimple_label_label (label_stmt))
+ && (LOCATION_LOCUS (locus) != UNKNOWN_LOCATION
+ || LOCATION_LOCUS (prev_locus) != UNKNOWN_LOCATION)
+ && !same_line_p (locus, &locus_e, prev_locus))
+ return true;
I think the UNKNOWN_LOCATION check isn't necessary? Two unknown
location locs will compare equal (and I think they should).
> {
> if (!first_stmt_of_seq)
> gsi_split_seq_before (&i, &seq);
> @@ -566,6 +632,9 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
> codes. */
> gimple_set_bb (stmt, bb);
> + if (!optimize && gimple_code (stmt) == GIMPLE_LABEL)
> + target_labels.add (gimple_label_label (as_a<glabel *> (stmt)));
> +
Huh, why? We're never going to visit the stmt again and thus never
look up the label again?
> /* If STMT is a basic block terminator, set START_NEW_BLOCK for the
> next iteration. */
> if (stmt_ends_bb_p (stmt))
> @@ -2832,7 +2901,8 @@ simple_goto_p (gimple *t)
> label. */
>
> static inline bool
> -stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> +stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt,
> + hash_set<tree> *target_labels)
> {
> if (stmt == NULL)
> return false;
> @@ -2860,9 +2930,24 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
> return true;
> + location_t prev_locus = gimple_location (plabel);
> + location_t locus = gimple_location (label_stmt);
> + expanded_location locus_e = expand_location (locus);
> +
> + if (!optimize
> + && target_labels->contains (gimple_label_label (label_stmt))
> + && (LOCATION_LOCUS (locus) != UNKNOWN_LOCATION
> + || LOCATION_LOCUS (prev_locus) != UNKNOWN_LOCATION)
> + && !same_line_p (locus, &locus_e, prev_locus))
> + return true;
> +
> cfg_stats.num_merged_labels++;
> return false;
> }
> + else if (!optimize
> + && !target_labels->contains (gimple_label_label (label_stmt))
> + && stmt->next && gimple_code (stmt->next) == GIMPLE_LABEL)
> + return false;
Huh? That would leave a label after a non-label stmt which is invalid.
Thanks and sorry for the delay.
Richard.
> else
> return true;
> }
> diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C
> b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> index ee383b480a8..01e7084fb03 100644
> --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
> +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> @@ -263,7 +263,7 @@ test_switch (int i, int j)
> case 2:
> result = do_something (1024);
> break;
> - case 3: /* count(3) */
> + case 3: /* count(2) */
> case 4:
> /* branch(67) */
> if (j == 2) /* count(3) */
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c
> b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> index da7929ef7fc..792cda8cfce 100644
> --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> @@ -122,7 +122,7 @@ top:
> }
> else
> {
> -else_: /* count(1) */
> +else_: /* count(2) */
> j = do_something (j); /* count(2) */
> if (j) /* count(2) */
> {
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> index 73e50b19fc7..b37e760910c 100644
> --- a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> @@ -7,7 +7,7 @@ int doit(int sel, int n, void *p)
>
> switch (sel)
> {
> - case 0: /* count(3) */
> + case 0: /* count(1) */
> do {*p0 += *p0;} while (--n); /* count(3) */
> return *p0 == 0; /* count(1) */
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> new file mode 100644
> index 00000000000..2fe340c4011
> --- /dev/null
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> @@ -0,0 +1,24 @@
> +/* { dg-options "-fprofile-arcs -ftest-coverage" } */
> +/* { dg-do run { target native } } */
> +
> +int f(int s, int n)
> +{
> + int p = 0;
> +
> + switch (s)
> + {
> + case 0: /* count(1) */
> + do { p++; } while (--n); /* count(5) */
> + return p; /* count(1) */
> +
> + case 1: /* count(1) */
> + do { p++; } while (--n); /* count(5) */
> + return p; /* count(1) */
> + }
> +
> + return 0;
> +}
> +
> +int main() { f(0, 5); f(1, 5); return 0; }
> +
> +/* { dg-final { run-gcov gcov-pr93680.c } } */
> diff --git a/gcc/testsuite/lib/gcov.exp b/gcc/testsuite/lib/gcov.exp
> index 80e74aeb220..07e1978d25d 100644
> --- a/gcc/testsuite/lib/gcov.exp
> +++ b/gcc/testsuite/lib/gcov.exp
> @@ -424,9 +424,7 @@ proc run-gcov { args } {
> }
> if { $tfailed > 0 } {
> fail "$testname gcov: $lfailed failures in line counts, $bfailed in
> branch percentages, $cfailed in return percentages, $ifailed in intermediate
> format"
> - if { $xfailed } {
> - clean-gcov $testcase
> - }
> + clean-gcov $testcase
> } else {
> pass "$testname gcov"
> clean-gcov $testcase
>
--
Richard Biener <rguenther@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)
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-03-21 11:18 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-02 2:29 [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] Xionghu Luo
2023-03-02 2:29 ` [PATCH 2/2] gcov: Fix incorrect gimple line LOCATION [PR97923] Xionghu Luo
2023-03-02 8:16 ` Richard Biener
2023-03-02 9:43 ` Xionghu Luo
2023-03-02 10:02 ` Richard Biener
2023-03-02 8:41 ` [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] Richard Biener
2023-03-02 10:22 ` Xionghu Luo
2023-03-02 10:45 ` Richard Biener
2023-03-06 7:22 ` Xionghu Luo
2023-03-06 8:11 ` Richard Biener
2023-03-07 7:41 ` Xionghu Luo
2023-03-07 8:53 ` Richard Biener
2023-03-07 10:26 ` Xionghu Luo
2023-03-07 11:25 ` Richard Biener
2023-03-08 13:07 ` [PATCH v3] " Xionghu Luo
2023-03-09 12:02 ` Richard Biener
2023-03-14 2:06 ` [PATCH v4] " Xionghu Luo
2023-03-21 11:18 ` Richard Biener
2023-03-15 10:07 ` Xionghu Luo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).