[Bug gcov-profile/45890] New: Coverage may be weakened by r164986

[Bug gcov-profile/45890] New: Coverage may be weakened by r164986

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890

           Summary: Coverage may be weakened by r164986
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: gcov-profile
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: rguenth@gcc.gnu.org


r164986 moves edge instrumentation for coverage after early optimizations
which may weaken location information enough to in turn weaken coverage
results.

A precise-coverage option might be useful that performs instrumentation
before any optimization or inlining is performed (unlike the state before
r164986 where inlining was performed before coverage analysis).

[Bug gcov-profile/45890] Coverage may be weakened by r164986

[singhai at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890

--- Comment #1 from Sharad Singhai <singhai at gcc dot gnu.org> 2011-10-01 19:39:32 UTC ---
Created attachment 25392
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25392
tar of source and coverage files

[Bug gcov-profile/45890] Coverage may be weakened by r164986

[singhai at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890

--- Comment #2 from Sharad Singhai <singhai at gcc dot gnu.org> 2011-10-01 19:42:48 UTC ---
I don't know if it is related. But the coverage is imprecise in case of
attached source file foo.c. Build it with O2, as 

gcc --coverage -O2 foo.c
./a.out
gcov -b foo.c

Now the coverage data for the 'if' condition in 'sum' looks like this:
(in attached file foo.c.gcov)

       8:    8:    if (v[i]) total += 1;
branch  0 never executed
branch  1 never executed
branch  2 taken 75% (fallthrough)
branch  3 taken 25%

Thus a simple two-way conditional looks like a four-way branch. It is due to
early inlining of 'sum' into 'main where a couple of bb's get eliminated/merged
so  that the branch coverage incorrectly gets attributed to the conditional.
Similarly the coverage data for the for-loop in 'sum' looks like this

       9:    7:  for (i = 0; i < N; ++i) {
branch  0 never executed
branch  1 never executed
branch  2 taken 89%
branch  3 taken 11% (fallthrough)

After disabling early inlining, the coverage data looks saner. (Attached in
foo.c.gcov.no-early-inlining.)

gcc --coverage -O2 -fno-early-inlining foo.c
./a.out
gcov -b foo.c

Adding the following patch mitigates the issue.

2011-09-30   Sharad Singhai  <singhai@google.com>

       * gcc.c (cc1_options): Added -fno-early-inlining for coverage.

Index: gcc.c
===================================================================
--- gcc.c       (revision 179402)
+++ gcc.c       (working copy)
@@ -776,7 +776,7 @@
 %{!fsyntax-only:%{S:%W{o*}%{!o*:-o %b.s}}}\
 %{fsyntax-only:-o %j} %{-param*}\
 %{fmudflap|fmudflapth:-fno-builtin -fno-merge-constants}\
- %{coverage:-fprofile-arcs -ftest-coverage}";
+ %{coverage:-fprofile-arcs -ftest-coverage -fno-early-inlining}";

 /* If an assembler wrapper is used to invoke post-assembly tools
   like MAO, --save-temps need to be passed to save the output of

[Bug gcov-profile/45890] Coverage may be weakened by r164986

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890

--- Comment #3 from rguenther at suse dot de <rguenther at suse dot de> 2011-10-04 09:14:42 UTC ---
On Sat, 1 Oct 2011, singhai at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890
> 
> --- Comment #2 from Sharad Singhai <singhai at gcc dot gnu.org> 2011-10-01 19:42:48 UTC ---
> I don't know if it is related. But the coverage is imprecise in case of
> attached source file foo.c. Build it with O2, as 
> 
> gcc --coverage -O2 foo.c
> ./a.out
> gcov -b foo.c
> 
> Now the coverage data for the 'if' condition in 'sum' looks like this:
> (in attached file foo.c.gcov)
> 
>        8:    8:    if (v[i]) total += 1;
> branch  0 never executed
> branch  1 never executed
> branch  2 taken 75% (fallthrough)
> branch  3 taken 25%
> 
> Thus a simple two-way conditional looks like a four-way branch. It is due to
> early inlining of 'sum' into 'main where a couple of bb's get eliminated/merged
> so  that the branch coverage incorrectly gets attributed to the conditional.
> Similarly the coverage data for the for-loop in 'sum' looks like this
> 
>        9:    7:  for (i = 0; i < N; ++i) {
> branch  0 never executed
> branch  1 never executed
> branch  2 taken 89%
> branch  3 taken 11% (fallthrough)
> 
> After disabling early inlining, the coverage data looks saner. (Attached in
> foo.c.gcov.no-early-inlining.)
> 
> gcc --coverage -O2 -fno-early-inlining foo.c
> ./a.out
> gcov -b foo.c
> 
> Adding the following patch mitigates the issue.

That's surely not the way to go.  Why do you want precise coverage
with -O2?

> 2011-09-30   Sharad Singhai  <singhai@google.com>
> 
>        * gcc.c (cc1_options): Added -fno-early-inlining for coverage.
> 
> Index: gcc.c
> ===================================================================
> --- gcc.c       (revision 179402)
> +++ gcc.c       (working copy)
> @@ -776,7 +776,7 @@
>  %{!fsyntax-only:%{S:%W{o*}%{!o*:-o %b.s}}}\
>  %{fsyntax-only:-o %j} %{-param*}\
>  %{fmudflap|fmudflapth:-fno-builtin -fno-merge-constants}\
> - %{coverage:-fprofile-arcs -ftest-coverage}";
> + %{coverage:-fprofile-arcs -ftest-coverage -fno-early-inlining}";
> 
>  /* If an assembler wrapper is used to invoke post-assembly tools
>    like MAO, --save-temps need to be passed to save the output of

[Bug gcov-profile/45890] Coverage may be weakened by r164986

[hubicka at ucw dot cz]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890

--- Comment #4 from Jan Hubicka <hubicka at ucw dot cz> 2011-10-04 09:18:56 UTC ---
> > gcc --coverage -O2 -fno-early-inlining foo.c
> > ./a.out
> > gcov -b foo.c
> > 
> > Adding the following patch mitigates the issue.
> 
> That's surely not the way to go.  Why do you want precise coverage
> with -O2?
I guess for performance reasons.
I would vote for adding coverage pass just after into-SSA.

Honza

[Bug gcov-profile/45890] Coverage may be weakened by r164986

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> 2011-10-04 09:50:55 UTC ---
On Tue, 4 Oct 2011, hubicka at ucw dot cz wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890
> 
> --- Comment #4 from Jan Hubicka <hubicka at ucw dot cz> 2011-10-04 09:18:56 UTC ---
> > > gcc --coverage -O2 -fno-early-inlining foo.c
> > > ./a.out
> > > gcov -b foo.c
> > > 
> > > Adding the following patch mitigates the issue.
> > 
> > That's surely not the way to go.  Why do you want precise coverage
> > with -O2?
> I guess for performance reasons.
> I would vote for adding coverage pass just after into-SSA.

Where do we do it right now?  At profile instrumentation stage, right?
I'd say if we don't need SSA form we should do coverage instrumentation
right after CFG construction, no?  Before into-SSA we already have
stuff like OMP lowering.  But yes, just after into-SSA would be
an improvement.

Richard.

[Bug gcov-profile/45890] Coverage may be weakened by r164986

[hubicka at ucw dot cz]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890

--- Comment #6 from Jan Hubicka <hubicka at ucw dot cz> 2011-10-04 10:04:37 UTC ---
> Where do we do it right now?  At profile instrumentation stage, right?
> I'd say if we don't need SSA form we should do coverage instrumentation
> right after CFG construction, no?  Before into-SSA we already have
> stuff like OMP lowering.  But yes, just after into-SSA would be
> an improvement.

We could also re-instantiate the former profiling pass before SSA for coverage,
but it seems to me that there is no real reason to destroy line info before
we get into-SSA and things will be a bit smoother if we do instrumentation
at SSA form only.  But I don't really have any strong opinions here.

[Bug gcov-profile/45890] Coverage may be weakened by r164986

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890

--- Comment #7 from rguenther at suse dot de <rguenther at suse dot de> 2011-10-04 10:18:18 UTC ---
On Tue, 4 Oct 2011, hubicka at ucw dot cz wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45890
> 
> --- Comment #6 from Jan Hubicka <hubicka at ucw dot cz> 2011-10-04 10:04:37 UTC ---
> > Where do we do it right now?  At profile instrumentation stage, right?
> > I'd say if we don't need SSA form we should do coverage instrumentation
> > right after CFG construction, no?  Before into-SSA we already have
> > stuff like OMP lowering.  But yes, just after into-SSA would be
> > an improvement.
> 
> We could also re-instantiate the former profiling pass before SSA for coverage,
> but it seems to me that there is no real reason to destroy line info before
> we get into-SSA and things will be a bit smoother if we do instrumentation
> at SSA form only.  But I don't really have any strong opinions here.

Yeah, I suppose you are right.