[1/2] gcov: test switch/break line counts

Message ID 20221011124303.99673-1-jorgen.kvalsvik@woven-planet.global
State Accepted, archived
Headers
Series [1/2] gcov: test switch/break line counts |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Jørgen Kvalsvik Oct. 11, 2022, 12:43 p.m. UTC
  The coverage support will under some conditions decide to split edges to
accurately report coverage. By running the test suite with/without this
edge splitting a small diff shows up, addressed by this patch, which
should catch future regressions.

Removing the edge splitting:

    $ diff --git a/gcc/profile.cc b/gcc/profile.cc
    --- a/gcc/profile.cc
    +++ b/gcc/profile.cc
    @@ -1244,19 +1244,7 @@ branch_prob (bool thunk)
                    Don't do that when the locuses match, so
                    if (blah) goto something;
                    is not computed twice.  */
    -             if (last
    -                 && gimple_has_location (last)
    -                 && !RESERVED_LOCATION_P (e->goto_locus)
    -                 && !single_succ_p (bb)
    -                 && (LOCATION_FILE (e->goto_locus)
    -                     != LOCATION_FILE (gimple_location (last))
    -                     || (LOCATION_LINE (e->goto_locus)
    -                         != LOCATION_LINE (gimple_location (last)))))
    -               {
    -                 basic_block new_bb = split_edge (e);
    -                 edge ne = single_succ_edge (new_bb);
    -                 ne->goto_locus = e->goto_locus;
    -               }
    +
            if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL))
                    && e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun))
                    need_exit_edge = 1;

Assuming the .gcov files from make chec-gcc RUNTESTFLAGS=gcov.exp are
kept:

    $ diff -r no-split-edge with-split-edge | grep -C 2 -E "^[<>]\s\s"
    diff -r sans-split-edge/gcc/gcov-4.c.gcov with-split-edge/gcc/gcov-4.c.gcov
    228c228
    <         -:  224:        break;
    ---
    >         1:  224:        break;
    231c231
    <         -:  227:        break;
    ---
    >     #####:  227:        break;
    237c237
    <         -:  233:        break;
    ---
    >         2:  233:        break;

gcc/testsuite/ChangeLog:

	* g++.dg/gcov/gcov-1.C: Add line count check.
	* gcc.misc-tests/gcov-4.c: Likewise.
---
 gcc/testsuite/g++.dg/gcov/gcov-1.C    | 8 ++++----
 gcc/testsuite/gcc.misc-tests/gcov-4.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)
  

Comments

Michael Matz Oct. 11, 2022, 1:55 p.m. UTC | #1
Hello,

On Tue, 11 Oct 2022, Jørgen Kvalsvik via Gcc-patches wrote:

> The coverage support will under some conditions decide to split edges to
> accurately report coverage. By running the test suite with/without this
> edge splitting a small diff shows up, addressed by this patch, which
> should catch future regressions.
> 
> Removing the edge splitting:
> 
>     $ diff --git a/gcc/profile.cc b/gcc/profile.cc
>     --- a/gcc/profile.cc
>     +++ b/gcc/profile.cc
>     @@ -1244,19 +1244,7 @@ branch_prob (bool thunk)
>                     Don't do that when the locuses match, so
>                     if (blah) goto something;
>                     is not computed twice.  */
>     -             if (last
>     -                 && gimple_has_location (last)
>     -                 && !RESERVED_LOCATION_P (e->goto_locus)
>     -                 && !single_succ_p (bb)
>     -                 && (LOCATION_FILE (e->goto_locus)
>     -                     != LOCATION_FILE (gimple_location (last))
>     -                     || (LOCATION_LINE (e->goto_locus)
>     -                         != LOCATION_LINE (gimple_location (last)))))
>     -               {
>     -                 basic_block new_bb = split_edge (e);
>     -                 edge ne = single_succ_edge (new_bb);
>     -                 ne->goto_locus = e->goto_locus;
>     -               }
>     +

Assuming this is correct (I really can't say) then the comment needs 
adjustments.  It specifically talks about this very code you remove.


Ciao,
Michael.
  
Jørgen Kvalsvik Oct. 11, 2022, 1:57 p.m. UTC | #2
On 11/10/2022 15:55, Michael Matz wrote:
> Hello,
> 
> On Tue, 11 Oct 2022, Jørgen Kvalsvik via Gcc-patches wrote:
> 
>> The coverage support will under some conditions decide to split edges to
>> accurately report coverage. By running the test suite with/without this
>> edge splitting a small diff shows up, addressed by this patch, which
>> should catch future regressions.
>>
>> Removing the edge splitting:
>>
>>     $ diff --git a/gcc/profile.cc b/gcc/profile.cc
>>     --- a/gcc/profile.cc
>>     +++ b/gcc/profile.cc
>>     @@ -1244,19 +1244,7 @@ branch_prob (bool thunk)
>>                     Don't do that when the locuses match, so
>>                     if (blah) goto something;
>>                     is not computed twice.  */
>>     -             if (last
>>     -                 && gimple_has_location (last)
>>     -                 && !RESERVED_LOCATION_P (e->goto_locus)
>>     -                 && !single_succ_p (bb)
>>     -                 && (LOCATION_FILE (e->goto_locus)
>>     -                     != LOCATION_FILE (gimple_location (last))
>>     -                     || (LOCATION_LINE (e->goto_locus)
>>     -                         != LOCATION_LINE (gimple_location (last)))))
>>     -               {
>>     -                 basic_block new_bb = split_edge (e);
>>     -                 edge ne = single_succ_edge (new_bb);
>>     -                 ne->goto_locus = e->goto_locus;
>>     -               }
>>     +
> 
> Assuming this is correct (I really can't say) then the comment needs 
> adjustments.  It specifically talks about this very code you remove.
> 
> 
> Ciao,
> Michael.

Michael,

I apologise for the confusion. The diff there is not a part of the change itself
(note the indentation) but rather a way to reproduce, or at least understand,
the type of change that would trigger the new test error. If it is too confusing
I can re-write the commit message.

Thanks,
Jørgen
  
Michael Matz Oct. 11, 2022, 2 p.m. UTC | #3
Hello,

On Tue, 11 Oct 2022, Jørgen Kvalsvik wrote:

> I apologise for the confusion. The diff there is not a part of the 
> change itself (note the indentation) but rather a way to reproduce,

Ah!  Thanks, that explains it, sorry for adding confusion on top :-)


Ciao,
Michael.
  
Richard Biener Oct. 13, 2022, 11:39 a.m. UTC | #4
On Tue, Oct 11, 2022 at 2:43 PM Jørgen Kvalsvik
<jorgen.kvalsvik@woven-planet.global> wrote:
>
> The coverage support will under some conditions decide to split edges to
> accurately report coverage. By running the test suite with/without this
> edge splitting a small diff shows up, addressed by this patch, which
> should catch future regressions.
>
> Removing the edge splitting:
>
>     $ diff --git a/gcc/profile.cc b/gcc/profile.cc
>     --- a/gcc/profile.cc
>     +++ b/gcc/profile.cc
>     @@ -1244,19 +1244,7 @@ branch_prob (bool thunk)
>                     Don't do that when the locuses match, so
>                     if (blah) goto something;
>                     is not computed twice.  */
>     -             if (last
>     -                 && gimple_has_location (last)
>     -                 && !RESERVED_LOCATION_P (e->goto_locus)
>     -                 && !single_succ_p (bb)
>     -                 && (LOCATION_FILE (e->goto_locus)
>     -                     != LOCATION_FILE (gimple_location (last))
>     -                     || (LOCATION_LINE (e->goto_locus)
>     -                         != LOCATION_LINE (gimple_location (last)))))
>     -               {
>     -                 basic_block new_bb = split_edge (e);
>     -                 edge ne = single_succ_edge (new_bb);
>     -                 ne->goto_locus = e->goto_locus;
>     -               }
>     +
>             if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL))
>                     && e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun))
>                     need_exit_edge = 1;
>
> Assuming the .gcov files from make chec-gcc RUNTESTFLAGS=gcov.exp are
> kept:
>
>     $ diff -r no-split-edge with-split-edge | grep -C 2 -E "^[<>]\s\s"
>     diff -r sans-split-edge/gcc/gcov-4.c.gcov with-split-edge/gcc/gcov-4.c.gcov
>     228c228
>     <         -:  224:        break;
>     ---
>     >         1:  224:        break;
>     231c231
>     <         -:  227:        break;
>     ---
>     >     #####:  227:        break;
>     237c237
>     <         -:  233:        break;
>     ---
>     >         2:  233:        break;
>
> gcc/testsuite/ChangeLog:

OK.

Thanks,
Richard.

>
>         * g++.dg/gcov/gcov-1.C: Add line count check.
>         * gcc.misc-tests/gcov-4.c: Likewise.
> ---
>  gcc/testsuite/g++.dg/gcov/gcov-1.C    | 8 ++++----
>  gcc/testsuite/gcc.misc-tests/gcov-4.c | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> index 9018b9a3a73..ee383b480a8 100644
> --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
> +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> @@ -257,20 +257,20 @@ test_switch (int i, int j)
>    switch (i)                           /* count(5) */
>                                         /* branch(end) */
>      {
> -      case 1:
> +      case 1:                          /* count(1) */
>          result = do_something (2);     /* count(1) */
> -        break;
> +        break;                         /* count(1) */
>        case 2:
>          result = do_something (1024);
>          break;
> -      case 3:
> +      case 3:                          /* count(3) */
>        case 4:
>                                         /* branch(67) */
>          if (j == 2)                    /* count(3) */
>                                         /* branch(end) */
>            return do_something (4);     /* count(1) */
>          result = do_something (8);     /* count(2) */
> -        break;
> +        break;                         /* count(2) */
>        default:
>         result = do_something (32);     /* count(1) */
>         switch_m++;                     /* count(1) */
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> index 9d8ab1c1097..498d299b66b 100644
> --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> @@ -221,7 +221,7 @@ test_switch (int i, int j)
>      {
>        case 1:
>          result = do_something (2);     /* count(1) */
> -        break;
> +        break;                         /* count(1) */
>        case 2:
>          result = do_something (1024);
>          break;
> @@ -230,7 +230,7 @@ test_switch (int i, int j)
>          if (j == 2)                    /* count(3) */
>            return do_something (4);     /* count(1) */
>          result = do_something (8);     /* count(2) */
> -        break;
> +        break;                         /* count(2) */
>        default:
>         result = do_something (32);     /* count(1) */
>         switch_m++;                     /* count(1) */
> --
> 2.34.0
>
  
Jørgen Kvalsvik Oct. 14, 2022, 10:09 a.m. UTC | #5
On 13/10/2022 13:39, Richard Biener wrote:
> On Tue, Oct 11, 2022 at 2:43 PM Jørgen Kvalsvik
> <jorgen.kvalsvik@woven-planet.global> wrote:
>>
>> The coverage support will under some conditions decide to split edges to
>> accurately report coverage. By running the test suite with/without this
>> edge splitting a small diff shows up, addressed by this patch, which
>> should catch future regressions.
>>
>> Removing the edge splitting:
>>
>>     $ diff --git a/gcc/profile.cc b/gcc/profile.cc
>>     --- a/gcc/profile.cc
>>     +++ b/gcc/profile.cc
>>     @@ -1244,19 +1244,7 @@ branch_prob (bool thunk)
>>                     Don't do that when the locuses match, so
>>                     if (blah) goto something;
>>                     is not computed twice.  */
>>     -             if (last
>>     -                 && gimple_has_location (last)
>>     -                 && !RESERVED_LOCATION_P (e->goto_locus)
>>     -                 && !single_succ_p (bb)
>>     -                 && (LOCATION_FILE (e->goto_locus)
>>     -                     != LOCATION_FILE (gimple_location (last))
>>     -                     || (LOCATION_LINE (e->goto_locus)
>>     -                         != LOCATION_LINE (gimple_location (last)))))
>>     -               {
>>     -                 basic_block new_bb = split_edge (e);
>>     -                 edge ne = single_succ_edge (new_bb);
>>     -                 ne->goto_locus = e->goto_locus;
>>     -               }
>>     +
>>             if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL))
>>                     && e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun))
>>                     need_exit_edge = 1;
>>
>> Assuming the .gcov files from make chec-gcc RUNTESTFLAGS=gcov.exp are
>> kept:
>>
>>     $ diff -r no-split-edge with-split-edge | grep -C 2 -E "^[<>]\s\s"
>>     diff -r sans-split-edge/gcc/gcov-4.c.gcov with-split-edge/gcc/gcov-4.c.gcov
>>     228c228
>>     <         -:  224:        break;
>>     ---
>>     >         1:  224:        break;
>>     231c231
>>     <         -:  227:        break;
>>     ---
>>     >     #####:  227:        break;
>>     237c237
>>     <         -:  233:        break;
>>     ---
>>     >         2:  233:        break;
>>
>> gcc/testsuite/ChangeLog:
> 
> OK.
> 
> Thanks,
> Richard.
> 
>>
>>         * g++.dg/gcov/gcov-1.C: Add line count check.
>>         * gcc.misc-tests/gcov-4.c: Likewise.
>> ---
>>  gcc/testsuite/g++.dg/gcov/gcov-1.C    | 8 ++++----
>>  gcc/testsuite/gcc.misc-tests/gcov-4.c | 4 ++--
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
>> index 9018b9a3a73..ee383b480a8 100644
>> --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
>> +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
>> @@ -257,20 +257,20 @@ test_switch (int i, int j)
>>    switch (i)                           /* count(5) */
>>                                         /* branch(end) */
>>      {
>> -      case 1:
>> +      case 1:                          /* count(1) */
>>          result = do_something (2);     /* count(1) */
>> -        break;
>> +        break;                         /* count(1) */
>>        case 2:
>>          result = do_something (1024);
>>          break;
>> -      case 3:
>> +      case 3:                          /* count(3) */
>>        case 4:
>>                                         /* branch(67) */
>>          if (j == 2)                    /* count(3) */
>>                                         /* branch(end) */
>>            return do_something (4);     /* count(1) */
>>          result = do_something (8);     /* count(2) */
>> -        break;
>> +        break;                         /* count(2) */
>>        default:
>>         result = do_something (32);     /* count(1) */
>>         switch_m++;                     /* count(1) */
>> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
>> index 9d8ab1c1097..498d299b66b 100644
>> --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
>> +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
>> @@ -221,7 +221,7 @@ test_switch (int i, int j)
>>      {
>>        case 1:
>>          result = do_something (2);     /* count(1) */
>> -        break;
>> +        break;                         /* count(1) */
>>        case 2:
>>          result = do_something (1024);
>>          break;
>> @@ -230,7 +230,7 @@ test_switch (int i, int j)
>>          if (j == 2)                    /* count(3) */
>>            return do_something (4);     /* count(1) */
>>          result = do_something (8);     /* count(2) */
>> -        break;
>> +        break;                         /* count(2) */
>>        default:
>>         result = do_something (32);     /* count(1) */
>>         switch_m++;                     /* count(1) */
>> --
>> 2.34.0
>>

Thank you, I've installed the patches.

I noticed that the inclusion of diffs in the message works fine with git
generally, but could be picked up on by git format-patch && git am < patch. I
hope that does not end up causing too many problems (last time, promise!)

Thanks,
Jørgen
  

Patch

diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
index 9018b9a3a73..ee383b480a8 100644
--- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
+++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
@@ -257,20 +257,20 @@  test_switch (int i, int j)
   switch (i)				/* count(5) */
 					/* branch(end) */
     {
-      case 1:
+      case 1:				/* count(1) */
         result = do_something (2);	/* count(1) */
-        break;
+        break;				/* count(1) */
       case 2:
         result = do_something (1024);
         break;
-      case 3:
+      case 3:				/* count(3) */
       case 4:
 					/* branch(67) */
         if (j == 2)			/* count(3) */
 					/* branch(end) */
           return do_something (4);	/* count(1) */
         result = do_something (8);	/* count(2) */
-        break;
+        break;				/* count(2) */
       default:
 	result = do_something (32);	/* count(1) */
 	switch_m++;			/* count(1) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
index 9d8ab1c1097..498d299b66b 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
@@ -221,7 +221,7 @@  test_switch (int i, int j)
     {
       case 1:
         result = do_something (2);	/* count(1) */
-        break;
+        break;				/* count(1) */
       case 2:
         result = do_something (1024);
         break;
@@ -230,7 +230,7 @@  test_switch (int i, int j)
         if (j == 2)			/* count(3) */
           return do_something (4);	/* count(1) */
         result = do_something (8);	/* count(2) */
-        break;
+        break;				/* count(2) */
       default:
 	result = do_something (32);	/* count(1) */
 	switch_m++;			/* count(1) */