> 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
