I agree with the staged checkin plan proposed. Teresa, can you help
with spliting out the libgcov.h/gcov-io.h change out (Rong is on a
trip..)
thanks,
David
On Fri, Dec 6, 2013 at 6:23 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> Hi, all
>>
>> This is the new patch for gcov-tool (previously profile-tool).
>>
>> Honza: can you comment on the new merge interface? David posted some
>> comments in an earlier email and we want to know what's your opinion.
>>
>> Test patch has been tested with boostrap, regresssion,
>> profiledbootstrap and SPEC2006.
>>
>> Noticeable changes from the earlier version:
>>
>> 1. create a new file libgcov.h and move libgcov-*.h headers to libgcov.h
>> So we can included multiple libgcov-*.c without adding new macros.
>>
>> 2. split libgcov.h specific code in gcvo-io.h to libcc/libgcov.h
>> Avoid multiple-page of code under IN_LIBGCOV macro -- this
>> improves the readability.
>>
>> 3. make gcov_var static, and move the definition from gcov-io.h to
>> gcov-io.c. Also
>> move some static functions accessing gcov_var to gcvo-io.c
>> Current code rely on GCOV_LINKAGE tricks to avoid multi-definition. I don't
>> see
>> a reason that gcov_var needs to exposed as a global.
>>
>> 4. expose gcov_write_strings() and gcov_sync() to gcov_tool usage
>>
>> 5. rename profile-tool to gcov-tool per Honza's suggestion.
>>
>> Thanks,
>
> Hi,
> I did not read in deatil the gcov-tool source itself, but lets first make the
> interface changes
> needed.
>
>> 2013-11-18 Rong Xu <x...@google.com>
>>
>> * gcc/gcov-io.c (gcov_var): Moved from gcov-io.h and make it static.
>> (gcov_position): Move from gcov-io.h
>> (gcov_is_error): Ditto.
>> (gcov_rewrite): Ditto.
>> * gcc/gcov-io.h: Re-factoring. Move gcov_var to gcov-io.h and
>> move the libgcov only part of libgcc/libgcov.h.
>> * libgcc/libgcov.h: New common header files for libgcov-*.h
>> * libgcc/Makefile.in: Add dependence to libgcov.h
>> * libgcc/libgcov-profiler.c: Use libgcov.h
>> * libgcc/libgcov-driver.c: Ditto.
>> * libgcc/libgcov-interface.c: Ditto.
>> * libgcc/libgcov-driver-system.c (allocate_filename_struct): use
>> xmalloc instread of malloc.
>> * libgcc/libgcov-merge.c (void __gcov_merge_delta): Add more
>> parameters to merge function.
>> (__gcov_merge_add): Ditto.
>> (__gcov_merge_ior): Ditto.
>> (__gcov_merge_time_profile): Ditto.
>> (__gcov_merge_single): Ditto.
>> (__gcov_merge_delta): Ditto.
>> * libgcc/libgcov-tool.c (void gcov_tool_set_verbose): New for
>> gcov-tool support.
>> (set_fn_ctrs): Ditto.
>> (tag_function): Ditto.
>> (tag_blocks): Ditto.
>> (tag_arcs): Ditto.
>> (tag_lines): Ditto.
>> (tag_counters): Ditto.
>> (tag_summary): Ditto.
>> (read_gcda_finalize): Ditto.
>> (read_gcda_file): Ditto.
>> (ftw_read_file): Ditto.
>> (read_profile_dir_init) Ditto.:
>> (gcov_read_profile_dir): Ditto.
>> (gcov_merge): Ditto.
>> (find_match_gcov_inf Ditto.o):
>> (gcov_profile_merge): Ditto.
>> (__gcov_scale_add): Ditto.
>> (__gcov_scale_ior): Ditto.
>> (__gcov_scale_delta): Ditto.
>> (__gcov_scale_single): Ditto.
>> (gcov_profile_scale): Ditto.
>> (gcov_profile_normalize): Ditto.
>> (__gcov_scale2_add): Ditto.
>> (__gcov_scale2_ior): Ditto.
>> (__gcov_scale2_delta): Ditto.
>> (__gcov_scale2_single): Ditto.
>> (gcov_profile_scale2): Ditto.
>> * gcc/gcov-tool.c (unlink_file): Gcov-tool driver support.
>> (unlink_dir): Ditto.
>> (profile_merge): Ditto.
>> (print_merge_usage_message): Ditto.
>> (merge_usage): Ditto.
>> (do_merge): Ditto.
>> (profile_rewrite2): Ditto.
>> (profile_rewrite): Ditto.
>> (print_rewrite_usage_message): Ditto.
>> (rewrite_usage): Ditto.
>> (do_rewrite): Ditto.
>> (print_usage): Ditto.
>> (print_version): Ditto.
>> (process_args): Ditto.
>> (main): Ditto.
>> * gcc/Makefile.in: Build and install gcov-tool.
>
>> Index: gcc/gcov-io.c
>> ===================================================================
>> --- gcc/gcov-io.c (revision 204895)
>> +++ gcc/gcov-io.c (working copy)
>> @@ -36,6 +36,37 @@ static const gcov_unsigned_t *gcov_read_words (uns
>> static void gcov_allocate (unsigned);
>> #endif
>>
>> +/* Moved for gcov-io.h and make it static. */
>> +static struct gcov_var gcov_var;
>
> This is more an changelog message than a comment in source file.
> Just describe what gcov_var is.
>
> 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.
>
> 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).
>
> 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)
>
>> Index: gcc/gcov-io.h
>> ===================================================================
>> --- gcc/gcov-io.h (revision 204895)
>> +++ gcc/gcov-io.h (working copy)
>> @@ -164,51 +164,7 @@ see the files COPYING3 and COPYING.RUNTIME respect
>> #ifndef GCC_GCOV_IO_H
>> #define GCC_GCOV_IO_H
>>
>> -#if IN_LIBGCOV
>> -/* About the target */
>> -
>> -#if BITS_PER_UNIT == 8
>> -typedef unsigned gcov_unsigned_t __attribute__ ((mode (SI)));
>> -typedef unsigned gcov_position_t __attribute__ ((mode (SI)));
>> -#if LONG_LONG_TYPE_SIZE > 32
>> -typedef signed gcov_type __attribute__ ((mode (DI)));
>> -typedef unsigned gcov_type_unsigned __attribute__ ((mode (DI)));
>> -#else
>> -typedef signed gcov_type __attribute__ ((mode (SI)));
>> -typedef unsigned gcov_type_unsigned __attribute__ ((mode (SI)));
>> -#endif
>> -#else
>> -#if BITS_PER_UNIT == 16
>> -typedef unsigned gcov_unsigned_t __attribute__ ((mode (HI)));
>> -typedef unsigned gcov_position_t __attribute__ ((mode (HI)));
>> -#if LONG_LONG_TYPE_SIZE > 32
>> -typedef signed gcov_type __attribute__ ((mode (SI)));
>> -typedef unsigned gcov_type_unsigned __attribute__ ((mode (SI)));
>> -#else
>> -typedef signed gcov_type __attribute__ ((mode (HI)));
>> -typedef unsigned gcov_type_unsigned __attribute__ ((mode (HI)));
>> -#endif
>> -#else
>> -typedef unsigned gcov_unsigned_t __attribute__ ((mode (QI)));
>> -typedef unsigned gcov_position_t __attribute__ ((mode (QI)));
>> -#if LONG_LONG_TYPE_SIZE > 32
>> -typedef signed gcov_type __attribute__ ((mode (HI)));
>> -typedef unsigned gcov_type_unsigned __attribute__ ((mode (HI)));
>> -#else
>> -typedef signed gcov_type __attribute__ ((mode (QI)));
>> -typedef unsigned gcov_type_unsigned __attribute__ ((mode (QI)));
>> -#endif
>> -#endif
>> -#endif
>
> So this part basically moves libgcov specific bits into libgcov.h? That OK
> fine by
> itself.
>> Index: libgcc/libgcov-profiler.c
>> ===================================================================
>> --- libgcc/libgcov-profiler.c (revision 204895)
>> +++ libgcc/libgcov-profiler.c (working copy)
>> @@ -23,15 +23,8 @@ a copy of the GCC Runtime Library Exception along
>> see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
>> <http://www.gnu.org/licenses/>. */
>>
>> -#include "tconfig.h"
>> -#include "tsystem.h"
>> -#include "coretypes.h"
>> -#include "tm.h"
>> -#include "libgcc_tm.h"
>> -
>> +#include "libgcov.h"
>> #if !defined(inhibit_libc)
>> -#define IN_LIBGCOV 1
>> -#include "gcov-io.h"
>
> I did not pay that much attention into the current include file changes, but
> wasn't
> idea to avoid #include file to include random other #includes?
> So perhaps the first block of includes should stay, followed by libgcov.h and
> gcov-io.h
> last?
>> @@ -325,6 +311,9 @@ static struct gcov_summary all_prg;
>> #endif
>> /* crc32 for this program. */
>> static gcov_unsigned_t crc32;
>> +/* Use this summary checksum rather the computed one if the value is
>> + * non-zero. */
>> +static gcov_unsigned_t saved_summary_checksum;
>
> Why do you need to save the checksum? Won't it reset summary back with
> multiple streaming?
>
> I would really like to avoid introducing those static vars that are used
> exclusively
> by gcov_exit. What about putting them into an gcov_context structure that
> is passed around the functions that was broken out?
>> Index: libgcc/libgcov-merge.c
>> ===================================================================
>> --- libgcc/libgcov-merge.c (revision 204895)
>> +++ libgcc/libgcov-merge.c (working copy)
>> @@ -23,107 +23,150 @@ a copy of the GCC Runtime Library Exception along
>> see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
>> <http://www.gnu.org/licenses/>. */
>>
>> -#include "tconfig.h"
>> -#include "tsystem.h"
>> -#include "coretypes.h"
>> -#include "tm.h"
>> -#include "libgcc_tm.h"
>> +#include "libgcov.h"
>>
>> -#if defined(inhibit_libc)
>> -#define IN_LIBGCOV (-1)
>> -#else
>> -#define IN_LIBGCOV 1
>> +#include "gcov-io-libgcov.h"
>> #endif
>>
>> -#include "gcov-io.h"
>> -
>> #if defined(inhibit_libc)
>> /* If libc and its header files are not available, provide dummy functions.
>> */
>>
>> #ifdef L_gcov_merge_add
>> void __gcov_merge_add (gcov_type *counters __attribute__ ((unused)),
>> - unsigned n_counters __attribute__ ((unused))) {}
>> + unsigned n_counters __attribute__ ((unused)),
>> + unsigned gcov_type *src __attribute__ ((unused)),
>> + unsigned w __attribute__ ((unused))) {}
>> #endif
>>
>> #ifdef L_gcov_merge_single
>> void __gcov_merge_single (gcov_type *counters __attribute__ ((unused)),
>> - unsigned n_counters __attribute__ ((unused))) {}
>> + unsigned n_counters __attribute__ ((unused)),
>> + unsigned gcov_type *src __attribute__ ((unused)),
>> + unsigned w __attribute__ ((unused))) {}
>> #endif
>>
>> #ifdef L_gcov_merge_delta
>> void __gcov_merge_delta (gcov_type *counters __attribute__ ((unused)),
>> - unsigned n_counters __attribute__ ((unused))) {}
>> + unsigned n_counters __attribute__ ((unused)),
>> + unsigned gcov_type *src __attribute__ ((unused)),
>> + unsigned w __attribute__ ((unused))) {}
>> #endif
>>
>> #else
>>
>> #ifdef L_gcov_merge_add
>> /* The profile merging function that just adds the counters. It is given
>> - an array COUNTERS of N_COUNTERS old counters and it reads the same number
>> - of counters from the gcov file. */
>> + an array COUNTERS of N_COUNTERS old counters.
>> + When SRC==NULL, it reads the same number of counters from the gcov file.
>> + Otherwise, it reads from SRC array. These read values will be multiplied
>> + by weight W before adding to the old counters. */
>> void
>> -__gcov_merge_add (gcov_type *counters, unsigned n_counters)
>> +__gcov_merge_add (gcov_type *counters, unsigned n_counters,
>> + gcov_type *src, unsigned w)
>> {
>> + int in_mem = (src != 0);
>> +
>> for (; n_counters; counters++, n_counters--)
>> - *counters += gcov_read_counter ();
>> + {
>> + gcov_type value;
>> +
>> + if (in_mem)
>> + value = *(src++);
>> + else
>> + value = gcov_read_counter ();
>> +
>> + *counters += w * value;
>> + }
>> }
>> #endif /* L_gcov_merge_add */
>>
>> #ifdef L_gcov_merge_ior
>> /* The profile merging function that just adds the counters. It is given
>> - an array COUNTERS of N_COUNTERS old counters and it reads the same number
>> - of counters from the gcov file. */
>> + an array COUNTERS of N_COUNTERS old counters.
>> + When SRC==NULL, it reads the same number of counters from the gcov file.
>> + Otherwise, it reads from SRC array. */
>> void
>> -__gcov_merge_ior (gcov_type *counters, unsigned n_counters)
>> +__gcov_merge_ior (gcov_type *counters, unsigned n_counters,
>> + gcov_type *src, unsigned w __attribute__ ((unused)))
>
> So the new in-memory variants are introduced for merging tool, while libgcc
> use gcov_read_counter
> interface?
> Perhaps we can actually just duplicate the functions to avoid runtime to do
> all the scalling
> and in_mem tests it won't need?
>
> I would suggest going with libgcov.h changes and clenaups first, with
> interface changes next
> and the gcov-tool is probably quite obvious at the end?
> Do you think you can split the patch this way?
>
> Thanks and sorry for taking long to review. I should have more time again now.
> Honza
