[RFC] libgcov.c re-factoring and offline profile-tool

Rong Xu xur@google.com
Wed Jan 8 22:10:00 GMT 2014


Here is the patch that addresses Honza's concern about bss increment.
It just makes this_prg a local variable.

Some comments are inlined.

On Fri, Dec 6, 2013 at 6:23 AM, Jan Hubicka <hubicka@ucw.cz> wrote:

> ....
> Do you know how the size of libgcov changed with your patch?
> Quick check of current mainline on compiling empty main gives:
>
> jh@gcc10:~/trunk/build/gcc$ cat t.c
> main()
> {
> }
> jh@gcc10:~/trunk/build/gcc$ ./xgcc -B ./ -O2 -fprofile-generate -o a.out-new --static t.c
> jh@gcc10:~/trunk/build/gcc$ gcc -O2 -fprofile-generate -o a.out-old --static t.c
> jh@gcc10:~/trunk/build/gcc$ size a.out-old
>    text    data     bss     dec     hex filename
>  608141    3560   16728  628429   996cd a.out-old
> jh@gcc10:~/trunk/build/gcc$ size a.out-new
>    text    data     bss     dec     hex filename
>  612621    3688   22880  639189   9c0d5 a.out-new
>
> Without profiling I get:
> jh@gcc10:~/trunk/build/gcc$ size a.out-new-no
> jh@gcc10:~/trunk/build/gcc$ size a.out-old-no
>    text    data     bss     dec     hex filename
>  599719    3448   12568  615735   96537 a.out-old-no
>    text    data     bss     dec     hex filename
>  600247    3448   12568  616263   96747 a.out-new-no
>
> Quite big for empty program, but mostly glibc fault, I suppose
> (that won't be an issue for embedded platforms). But anyway
> we increased text size overhead from 8k to 12k, BSS size
> overhead from 4k to 10k and data by another 1k.
>

I think it would more fair to compare r204729 and r204730. Your
comparison had some other changes in libgcov such as time_profiler and
indirecto_call_profiler_v2.

Using the same empty t.c, for r204729, we have
xur2%208:gcc >> ./xgcc -B ./ -O2 -fprofile-generate --static -o
a.out-r204729 t.c
xur2%209:gcc >> size a.out-r204729
   text   data    bss    dec    hex filename
 803207   6352  15448 825007  c96af a.out-r204729
xur2%210:gcc >> ./xgcc -B ./ -O2 --static -o a.out-r204729-no t.c
xur2%211:gcc >> size a.out-r204729-no
   text   data    bss    dec    hex filename
 790337   6112  11336 807785  c5369 a.out-r204729-no

For r204730, we have
xur2%216:gcc >> ./xgcc -B ./ -O2 -fprofile-generate --static -o
a.out-r204730 t.c
xur2%217:gcc >> size a.out-r204730
   text   data    bss    dec    hex filename
 802919   6384  21592 830895  cadaf a.out-r204730
xur2%218:gcc >> ./xgcc -B ./ -O2  --static -o a.out-r204730-no t.c
xur2%219:gcc >> size a.out-r204730-no
   text   data    bss    dec    hex filename
 790337   6112  11336 807785  c5369 a.out-r204730-no

r204730 actually has smaller text, data size with -fprofile-generate.
You are right about there are 6kb more bss space due to the static
variables introduced. It mostly caused by this_prg object.

With the attached trunk patch that localizes this_prg, we have
xur2%42:fdo >> size a.out-new
   text   data    bss    dec    hex filename
 803479   6456  15512 825447  c9867 a.out-new
xur2%43:fdo >> size a.out-new-no
   text   data    bss    dec    hex filename
 790545   6112  11368 808025  c5459 a.out-new-no

We are now using 64 more bytes in m64.

Objects size for r204730:
   text   data    bss    dec    hex filename
     57      0      0     57     39 _gcov_average_profiler.o
     66      0      0     66     42 _gcov_dump.o
    516      0      0    516    204 _gcov_execle.o
    476      0      0    476    1dc _gcov_execl.o
    476      0      0    476    1dc _gcov_execlp.o
    108      0      0    108     6c _gcov_execve.o
     98      0      0     98     62 _gcov_execv.o
     98      0      0     98     62 _gcov_execvp.o
    126      0     40    166     a6 _gcov_flush.o
    101      0      0    101     65 _gcov_fork.o
    122      0      0    122     7a _gcov_indirect_call_profiler.o
    178      0     16    194     c2 _gcov_indirect_call_profiler_v2.o
     89      0      0     89     59 _gcov_interval_profiler.o
     52      0      0     52     34 _gcov_ior_profiler.o
    126      0      0    126     7e _gcov_merge_add.o
    242      0      0    242     f2 _gcov_merge_delta.o
    126      0      0    126     7e _gcov_merge_ior.o
    251      0      0    251     fb _gcov_merge_single.o
    156      0      0    156     9c _gcov_merge_time_profile.o
   9252      0   6144  15396   3c24 _gcov.o
    115      0      0    115     73 _gcov_one_value_profiler.o
     69      0      0     69     45 _gcov_pow2_profiler.o
     66      0      0     66     42 _gcov_reset.o
     77      0      8     85     55 _gcov_time_profiler.o

Objects size for r204729:
   text   data    bss    dec    hex filename
     57      0      0     57     39 _gcov_average_profiler.o
     72      0      0     72     48 _gcov_dump.o
    516      0      0    516    204 _gcov_execle.o
    476      0      0    476    1dc _gcov_execl.o
    476      0      0    476    1dc _gcov_execlp.o
    108      0      0    108     6c _gcov_execve.o
     98      0      0     98     62 _gcov_execv.o
     98      0      0     98     62 _gcov_execvp.o
    101      0      0    101     65 _gcov_fork.o
    122      0      0    122     7a _gcov_indirect_call_profiler.o
    178      0     16    194     c2 _gcov_indirect_call_profiler_v2.o
     89      0      0     89     59 _gcov_interval_profiler.o
     52      0      0     52     34 _gcov_ior_profiler.o
    126      0      0    126     7e _gcov_merge_add.o
    242      0      0    242     f2 _gcov_merge_delta.o
    126      0      0    126     7e _gcov_merge_ior.o
    251      0      0    251     fb _gcov_merge_single.o
    156      0      0    156     9c _gcov_merge_time_profile.o
   9564      0     64   9628   259c _gcov.o
    115      0      0    115     73 _gcov_one_value_profiler.o
     69      0      0     69     45 _gcov_pow2_profiler.o
     72      0      0     72     48 _gcov_reset.o
     77      0      0     77     4d _gcov_time_profiler.o


>    text    data     bss     dec     hex filename
>     126       0       0     126      7e _gcov_merge_add.o (ex libgcov.a)
>     251       0       0     251      fb _gcov_merge_single.o (ex libgcov.a)
>     242       0       0     242      f2 _gcov_merge_delta.o (ex libgcov.a)
>     126       0       0     126      7e _gcov_merge_ior.o (ex libgcov.a)
>     156       0       0     156      9c _gcov_merge_time_profile.o (ex libgcov.a)
>      89       0       0      89      59 _gcov_interval_profiler.o (ex libgcov.a)
>      69       0       0      69      45 _gcov_pow2_profiler.o (ex libgcov.a)
>     115       0       0     115      73 _gcov_one_value_profiler.o (ex libgcov.a)
>     122       0       0     122      7a _gcov_indirect_call_profiler.o (ex libgcov.a)
>      57       0       0      57      39 _gcov_average_profiler.o (ex libgcov.a)
>      52       0       0      52      34 _gcov_ior_profiler.o (ex libgcov.a)
>     178       0      16     194      c2 _gcov_indirect_call_profiler_v2.o (ex libgcov.a)
>      77       0       8      85      55 _gcov_time_profiler.o (ex libgcov.a)
>     126       0      40     166      a6 _gcov_flush.o (ex libgcov.a)
>     101       0       0     101      65 _gcov_fork.o (ex libgcov.a)
>     471       0       0     471     1d7 _gcov_execl.o (ex libgcov.a)
>     471       0       0     471     1d7 _gcov_execlp.o (ex libgcov.a)
>     524       0       0     524     20c _gcov_execle.o (ex libgcov.a)
>      98       0       0      98      62 _gcov_execv.o (ex libgcov.a)
>      98       0       0      98      62 _gcov_execvp.o (ex libgcov.a)
>     108       0       0     108      6c _gcov_execve.o (ex libgcov.a)
>      66       0       0      66      42 _gcov_reset.o (ex libgcov.a)
>      66       0       0      66      42 _gcov_dump.o (ex libgcov.a)
>    9505       0    6144   15649    3d21 _gcov.o (ex libgcov.a)
>
> I think we definitely need to move those 6k of bss space out.  I think those are new
> static vars you introduced that I think are unsafe anyway because multiple streaming
> may run at once in threaded program where locking mechanizm fails.
> (it will probably do other bad things, but definitely we do not want to
> conflict on things like filename to write into).

I don't understand why static variables can cause any safety issue in
multi-thread programs.
For any process, gcov_dump will be called only once and there will be
one instance of globals.
Where is the problem?
The file locking is for multi-process program. For that case, each
process has its own instance of globals.
I don't see this causes any new problems.


>
> Compared to my system gcov:
>    text    data     bss     dec     hex filename
>    9765       0      64    9829    2665 _gcov.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     125       0       0     125      7d _gcov_merge_add.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     251       0       0     251      fb _gcov_merge_single.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     242       0       0     242      f2 _gcov_merge_delta.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     101       0       0     101      65 _gcov_fork.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     471       0       0     471     1d7 _gcov_execl.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     471       0       0     471     1d7 _gcov_execlp.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     524       0       0     524     20c _gcov_execle.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>      98       0       0      98      62 _gcov_execv.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>      98       0       0      98      62 _gcov_execvp.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     108       0       0     108      6c _gcov_execve.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>      72       0       0      72      48 _gcov_reset.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>      72       0       0      72      48 _gcov_dump.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>      89       0       0      89      59 _gcov_interval_profiler.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>      69       0       0      69      45 _gcov_pow2_profiler.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     115       0       0     115      73 _gcov_one_value_profiler.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     122       0       0     122      7a _gcov_indirect_call_profiler.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>      57       0       0      57      39 _gcov_average_profiler.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>      52       0       0      52      34 _gcov_ior_profiler.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>     125       0       0     125      7d _gcov_merge_ior.o (ex ./lib64/gcc/x86_64-suse-linux/4.8/libgcov.a)
>
-------------- next part --------------
2014-01-08  Rong Xu  <xur@google.com>

	* libgcc/libgcov-driver.c (this_prg): make it local to save
        bss space.
	(gcov_exit_compute_summary): Ditto.
	(gcov_exit_merge_gcda): Ditto.
	(gcov_exit_merge_summary): Ditto.
	(gcov_exit_dump_gcov): Ditto.

Index: libgcc/libgcov-driver.c
===================================================================
--- libgcc/libgcov-driver.c	(revision 206437)
+++ libgcc/libgcov-driver.c	(working copy)
@@ -303,8 +303,6 @@ gcov_compute_histogram (struct gcov_summary *sum)
     }
 }
 
-/* summary for program.  */
-static struct gcov_summary this_prg;
 /* gcda filename.  */
 static char *gi_filename;
 /* buffer for the fn_data from another program.  */
@@ -317,10 +315,10 @@ static struct gcov_summary_buffer *sum_buffer;
 static int run_accounted = 0;
 
 /* This funtions computes the program level summary and the histo-gram.
-   It computes and returns CRC32  and stored summari in THIS_PRG.  */
+   It computes and returns CRC32 and stored summary in THIS_PRG.  */
 
 static gcov_unsigned_t
-gcov_exit_compute_summary (void)
+gcov_exit_compute_summary (struct gcov_summary *this_prg)
 {
   struct gcov_info *gi_ptr;
   const struct gcov_fn_info *gfi_ptr;
@@ -332,7 +330,7 @@ static gcov_unsigned_t
   gcov_unsigned_t crc32 = 0;
 
   /* Find the totals for this execution.  */
-  memset (&this_prg, 0, sizeof (this_prg));
+  memset (this_prg, 0, sizeof (*this_prg));
   for (gi_ptr = gcov_list; gi_ptr; gi_ptr = gi_ptr->next)
     {
       crc32 = crc32_unsigned (crc32, gi_ptr->stamp);
@@ -357,7 +355,7 @@ static gcov_unsigned_t
               if (!gi_ptr->merge[t_ix])
                 continue;
 
-              cs_ptr = &this_prg.ctrs[t_ix];
+              cs_ptr = &(this_prg->ctrs[t_ix]);
               cs_ptr->num += ci_ptr->num;
               crc32 = crc32_unsigned (crc32, ci_ptr->num);
 
@@ -371,7 +369,7 @@ static gcov_unsigned_t
             }
         }
     }
-  gcov_compute_histogram (&this_prg);
+  gcov_compute_histogram (this_prg);
   return crc32;
 }
 
@@ -393,6 +391,7 @@ struct gcov_filename_aux{
 static int
 gcov_exit_merge_gcda (struct gcov_info *gi_ptr,
                       struct gcov_summary *prg_p,
+                      struct gcov_summary *this_prg,
                       gcov_position_t *summary_pos_p,
                       gcov_position_t *eof_pos_p,
 		      gcov_unsigned_t crc32)
@@ -446,7 +445,7 @@ gcov_exit_merge_gcda (struct gcov_info *gi_ptr,
         goto next_summary;
 
       for (t_ix = 0; t_ix != GCOV_COUNTERS_SUMMABLE; t_ix++)
-        if (tmp.ctrs[t_ix].num != this_prg.ctrs[t_ix].num)
+        if (tmp.ctrs[t_ix].num != this_prg->ctrs[t_ix].num)
           goto next_summary;
       *prg_p = tmp;
       *summary_pos_p = *eof_pos_p;
@@ -636,7 +635,7 @@ gcov_exit_write_gcda (const struct gcov_info *gi_p
 
 static int
 gcov_exit_merge_summary (const struct gcov_info *gi_ptr, struct gcov_summary *prg,
-			 gcov_unsigned_t crc32,
+                         struct gcov_summary *this_prg, gcov_unsigned_t crc32,
 			 struct gcov_summary *all_prg __attribute__ ((unused)))
 {
   struct gcov_ctr_summary *cs_prg, *cs_tprg;
@@ -650,7 +649,7 @@ gcov_exit_merge_summary (const struct gcov_info *g
   for (t_ix = 0; t_ix < GCOV_COUNTERS_SUMMABLE; t_ix++)
     {
       cs_prg = &(prg->ctrs[t_ix]);
-      cs_tprg = &this_prg.ctrs[t_ix];
+      cs_tprg = &(this_prg->ctrs[t_ix]);
 
       if (gi_ptr->merge[t_ix])
         {
@@ -719,7 +718,8 @@ gcov_exit_merge_summary (const struct gcov_info *g
 
 static void
 gcov_exit_dump_gcov (struct gcov_info *gi_ptr, struct gcov_filename_aux *gf,
-		     gcov_unsigned_t crc32, struct gcov_summary *all_prg)
+		     gcov_unsigned_t crc32, struct gcov_summary *all_prg,
+                     struct gcov_summary *this_prg)
 {
   struct gcov_summary prg; /* summary for this object over all program.  */
   int error;
@@ -743,7 +743,7 @@ gcov_exit_dump_gcov (struct gcov_info *gi_ptr, str
           gcov_error ("profiling:%s:Not a gcov data file\n", gi_filename);
           goto read_fatal;
         }
-      error = gcov_exit_merge_gcda (gi_ptr, &prg, &summary_pos, &eof_pos,
+      error = gcov_exit_merge_gcda (gi_ptr, &prg, this_prg, &summary_pos, &eof_pos,
 				    crc32);
       if (error == -1)
         goto read_fatal;
@@ -757,7 +757,7 @@ gcov_exit_dump_gcov (struct gcov_info *gi_ptr, str
       summary_pos = eof_pos;
     }
 
-  error = gcov_exit_merge_summary (gi_ptr, &prg, crc32, all_prg);
+  error = gcov_exit_merge_summary (gi_ptr, &prg, this_prg, crc32, all_prg);
   if (error == -1)
     goto read_fatal;
 
@@ -787,13 +787,14 @@ gcov_exit (void)
   struct gcov_filename_aux gf;
   gcov_unsigned_t crc32;
   struct gcov_summary all_prg;
+  struct gcov_summary this_prg;
 
   /* Prevent the counters from being dumped a second time on exit when the
      application already wrote out the profile using __gcov_dump().  */
   if (gcov_dump_complete)
     return;
 
-  crc32 = gcov_exit_compute_summary ();
+  crc32 = gcov_exit_compute_summary (&this_prg);
 
   allocate_filename_struct (&gf);
 #if !GCOV_LOCKED
@@ -802,7 +803,7 @@ gcov_exit (void)
 
   /* Now merge each file.  */
   for (gi_ptr = gcov_list; gi_ptr; gi_ptr = gi_ptr->next)
-    gcov_exit_dump_gcov (gi_ptr, &gf, crc32, &all_prg);
+    gcov_exit_dump_gcov (gi_ptr, &gf, crc32, &all_prg, &this_prg);
   run_accounted = 1;
 
   if (gi_filename)


More information about the Gcc-patches mailing list