Patchwork [gcov] a few improvements

login
register
mail settings
Submitter Nathan Sidwell
Date Dec. 30, 2011, 6:25 p.m.
Message ID <4EFE021C.3040406@acm.org>
Download mbox | patch
Permalink /patch/133692/
State New
Headers show

Comments

Nathan Sidwell - Dec. 30, 2011, 6:25 p.m.
I've committed this patch to fix and improve coverage reporting:

1) the time stamp local_tick will be -1 if the user overrides the random seed. 
In such cases the gcov data file should be deleted, just as it would if the time 
cannot be determined.

2) there was a typo in gcov_dump when dumping function data

3) when processing coverage data files, it's informative for gcov to emit the 
overall line coverage -- in addition to the individual line coverage.

built & tested on i686-pc-linux-gnu

nathan
2011-12-30  Nathan Sidwell  <nathan@acm.org>

	* gcov.c (total_lines, total_executed): New global vars.
	(generate_results): Call executed_summary.
	(executed_summary): New function, broken out of ...
	(function_summary): ... here.  Call it.
	* coverage.c (coverage_finish): Also check for local_tick == -1.
	* gcov-dump (tag_function): Correct labelling typo.
Xinliang David Li - June 3, 2012, 4:51 a.m.
On Fri, Dec 30, 2011 at 10:25 AM, Nathan Sidwell <nathan@acm.org> wrote:
> I've committed this patch to fix and improve coverage reporting:
>
> 1) the time stamp local_tick will be -1 if the user overrides the random
> seed. In such cases the gcov data file should be deleted, just as it would
> if the time cannot be determined.

The end result is that after a the profile data is used in the
compilation (with option -fprofile-use -frandom-seed=), it is then
deleted. Is this intended? This basically makes FDO very hard to use
when -frandom-seed is used.

David

>
> 2) there was a typo in gcov_dump when dumping function data
>
> 3) when processing coverage data files, it's informative for gcov to emit
> the overall line coverage -- in addition to the individual line coverage.
>
> built & tested on i686-pc-linux-gnu
>
> nathan
Nathan Sidwell - June 3, 2012, 1:10 p.m.
On 06/03/12 05:51, Xinliang David Li wrote:
> On Fri, Dec 30, 2011 at 10:25 AM, Nathan Sidwell<nathan@acm.org>  wrote:
>> I've committed this patch to fix and improve coverage reporting:
>>
>> 1) the time stamp local_tick will be -1 if the user overrides the random
>> seed. In such cases the gcov data file should be deleted, just as it would
>> if the time cannot be determined.
>
> The end result is that after a the profile data is used in the
> compilation (with option -fprofile-use -frandom-seed=), it is then
> deleted. Is this intended? This basically makes FDO very hard to use
> when -frandom-seed is used.

IIUC the file is deleted after being used.  What is the problem you are 
encountering? (if you are having a problem, that suggests others on systems 
without local_tick are also having problems)

nathan
Xinliang David Li - June 3, 2012, 4:16 p.m.
On Sun, Jun 3, 2012 at 6:10 AM, Nathan Sidwell <nathan@acm.org> wrote:
> On 06/03/12 05:51, Xinliang David Li wrote:
>>
>> On Fri, Dec 30, 2011 at 10:25 AM, Nathan Sidwell<nathan@acm.org>  wrote:
>>>
>>> I've committed this patch to fix and improve coverage reporting:
>>>
>>> 1) the time stamp local_tick will be -1 if the user overrides the random
>>> seed. In such cases the gcov data file should be deleted, just as it
>>> would
>>> if the time cannot be determined.
>>
>>
>> The end result is that after a the profile data is used in the
>> compilation (with option -fprofile-use -frandom-seed=), it is then
>> deleted. Is this intended? This basically makes FDO very hard to use
>> when -frandom-seed is used.
>
>
> IIUC the file is deleted after being used.  What is the problem you are
> encountering? (if you are having a problem, that suggests others on systems
> without local_tick are also having problems)

Basically it makes it very difficult to rebuild the file with the
profile data --- which makes problem triaging impossible. What is
worse is that when profile data is missing, gcc silently takes it
which gives you no clue you chasing in a completely different path ..

David


>
> nathan
Nathan Sidwell - June 3, 2012, 5:24 p.m.
On 06/03/12 17:16, Xinliang David Li wrote:

> Basically it makes it very difficult to rebuild the file with the
> profile data --- which makes problem triaging impossible. What is

Can you explain this more -- what exactly are trying to do?  Are you trying to 
rebuild multiple times with the same coverage data, or something different?
Xinliang David Li - June 3, 2012, 8:40 p.m.
On Sun, Jun 3, 2012 at 10:24 AM, Nathan Sidwell <nathan@acm.org> wrote:
> On 06/03/12 17:16, Xinliang David Li wrote:
>
>> Basically it makes it very difficult to rebuild the file with the
>> profile data --- which makes problem triaging impossible. What is
>
>
> Can you explain this more -- what exactly are trying to do?  Are you trying
> to rebuild multiple times with the same coverage data,

yes -- for instance, in the context of debugging a compiler problem,
you will need to compile the same file multiple times with the same
coverage data.

David


> or something
> different?
Nathan Sidwell - June 4, 2012, 7:49 a.m.
On 06/03/12 21:40, Xinliang David Li wrote:

>> Can you explain this more -- what exactly are trying to do?  Are you trying
>> to rebuild multiple times with the same coverage data,
>
> yes -- for instance, in the context of debugging a compiler problem,
> you will need to compile the same file multiple times with the same
> coverage data.

ok, debugging the compiler is not the typical user mode of use.

Do any of the following work for you?
*) don't use -frandom-seed (you don't say why you're using this flag)
*) copy and restore the coverage data file
*) stub out the unlink code at the end of coverage.c. (I presume that you're 
rebuilding cc1 with optimization off anyway, and as you're debugging a compiler 
problem with FDO you're somewhat familiar with coverage.c).

nathan
Xinliang David Li - June 4, 2012, 4:19 p.m.
On Mon, Jun 4, 2012 at 12:49 AM, Nathan Sidwell <nathan@acm.org> wrote:
> On 06/03/12 21:40, Xinliang David Li wrote:
>
>>> Can you explain this more -- what exactly are trying to do?  Are you
>>> trying
>>> to rebuild multiple times with the same coverage data,
>>
>>
>> yes -- for instance, in the context of debugging a compiler problem,
>> you will need to compile the same file multiple times with the same
>> coverage data.
>
>
> ok, debugging the compiler is not the typical user mode of use.

Another usage is FDO performance tuning trying build with different
options/parameters, so it is common.


>
> Do any of the following work for you?
> *) don't use -frandom-seed (you don't say why you're using this flag)

This is in our build system for build reproducibility.

> *) copy and restore the coverage data file
> *) stub out the unlink code at the end of coverage.c. (I presume that you're
> rebuilding cc1 with optimization off anyway, and as you're debugging a
> compiler problem with FDO you're somewhat familiar with coverage.c).

I know how to workaround/fix the problem --- I am asking the
motivation for deleting the coverage file? What does it buy us? This
looks like a regression in gcc4.7 to me.

thanks,

David


>
> nathan
Nathan Sidwell - June 10, 2012, 6:04 p.m.
On 06/04/12 17:19, Xinliang David Li wrote:

> Another usage is FDO performance tuning trying build with different
> options/parameters, so it is common.

> This is in our build system for build reproducibility.

If you're trying different options, your builds are not repeatable, so I'm not 
totally sure why you're using random seed.

The problem is that, when generating a new set of object files from the coverage 
files of the previous compilation, the old coverage files no longer match the 
new object files.  Without some kind of way of distinguishing this, the coverage 
machinery gets confused.  The random seed is the way of making this distinction, 
and allows the coverage machinery to know that when dumping data from executing 
the new object file, that the coverage file contains information about an old 
object file -- and so can truncate the file.  If the random seed matches, it 
can't do that and then (if you're lucky) notices the function checksums don't match.

I wonder if there's some way of propagating some kind of 'generation' count in 
the face of using the same random-seed.  I'll have a think.

nathan

Patch

Index: gcov.c
===================================================================
--- gcov.c	(revision 182730)
+++ gcov.c	(working copy)
@@ -278,6 +278,9 @@  static unsigned a_names;    /* Allocated
 static unsigned object_runs;
 static unsigned program_count;
 
+static unsigned total_lines;
+static unsigned total_executed;
+
 /* Modification time of graph file.  */
 
 static time_t bbg_file_time;
@@ -380,6 +383,7 @@  static void solve_flow_graph (function_t
 static void find_exception_blocks (function_t *);
 static void add_branch_counts (coverage_t *, const arc_t *);
 static void add_line_counts (coverage_t *, function_t *);
+static void executed_summary (unsigned, unsigned);
 static void function_summary (const coverage_t *, const char *);
 static const char *format_gcov (gcov_type, gcov_type, int);
 static void accumulate_line_counts (source_t *);
@@ -702,6 +706,8 @@  generate_results (const char *file_name)
       
       accumulate_line_counts (src);
       function_summary (&src->coverage, "File");
+      total_lines += src->coverage.lines;
+      total_executed += src->coverage.lines_executed;
       if (flag_gcov_file && src->coverage.lines)
 	{
 	  char *gcov_file_name
@@ -724,6 +730,9 @@  generate_results (const char *file_name)
 	}
       fnotice (stdout, "\n");
     }
+
+  if (!file_name)
+    executed_summary (total_lines, total_executed);
 }
 
 /* Release a function structure */
@@ -1666,20 +1675,25 @@  format_gcov (gcov_type top, gcov_type bo
   return buffer;
 }
 
-
-/* Output summary info for a function.  */
+/* Summary of execution */
 
 static void
-function_summary (const coverage_t *coverage, const char *title)
+executed_summary (unsigned lines, unsigned executed)
 {
-  fnotice (stdout, "%s '%s'\n", title, coverage->name);
-
-  if (coverage->lines)
+  if (lines)
     fnotice (stdout, "Lines executed:%s of %d\n",
-	     format_gcov (coverage->lines_executed, coverage->lines, 2),
-	     coverage->lines);
+	     format_gcov (executed, lines, 2), lines);
   else
     fnotice (stdout, "No executable lines\n");
+}
+  
+/* Output summary info for a function or file.  */
+
+static void
+function_summary (const coverage_t *coverage, const char *title)
+{
+  fnotice (stdout, "%s '%s'\n", title, coverage->name);
+  executed_summary (coverage->lines, coverage->lines_executed);
 
   if (flag_branches)
     {
Index: coverage.c
===================================================================
--- coverage.c	(revision 182730)
+++ coverage.c	(working copy)
@@ -1119,7 +1119,7 @@  coverage_finish (void)
   if (bbg_file_name && gcov_close ())
     unlink (bbg_file_name);
   
-  if (!local_tick)
+  if (!local_tick || local_tick == (unsigned)-1)
     /* Only remove the da file, if we cannot stamp it.  If we can
        stamp it, libgcov will DTRT.  */
     unlink (da_file_name);
Index: gcov-dump.c
===================================================================
--- gcov-dump.c	(revision 182730)
+++ gcov-dump.c	(working copy)
@@ -286,7 +286,7 @@  tag_function (const char *filename ATTRI
     {
       printf (" ident=%u", gcov_read_unsigned ());
       printf (", lineno_checksum=0x%08x", gcov_read_unsigned ());
-      printf (", cfg_checksum_checksum=0x%08x", gcov_read_unsigned ());
+      printf (", cfg_checksum=0x%08x", gcov_read_unsigned ());
 
       if (gcov_position () - pos < length)
 	{