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

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)
>
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.

> 
> 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.

(v)fork, failing exec and other cases can lead to gcov_flush being called
several times  and that calls gcov_exit that uses the global state.

I believe that a simple program containing two threads that both are executing
execve of non-existing file will trigger concurent writes on systems not
having __gthread_mutex_lock  that seems to be in place to prevent it.

I wonder if we should move move locking into gcov_exit itself?
Is there somethign that will promise us that paralellel streaming invoked by
other thread at the failing execve is not going to end up in parallel with
gcov_exit called via atexit handler?

In any case for sanity of setups without gthread support, I think we need to
keep eye on not doing something evil in this case - like writting into random
file names or corrupting memory/files.

> 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.

This is OK, thanks!
Honza