On Mon, Dec 16, 2013 at 12:55 PM, Xinliang David Li <davi...@google.com> wrote: > gcov_rewrite function is only needed (and defined) with IN_LIBGCOV. > Should it be moved from common file gcov-io.c to libgcov.c?
Possibly. I just looked through gcov-io.c and there are several additional functions that are only defined under "#ifdef IN_LIBGCOV" and only used in libgcov*c (or each other): gcov_write_counter gcov_write_tag_length gcov_write_summary gcov_seek Should they all, plus gcov_rewrite, be moved to libgcov-driver.c? Teresa > > > David > > On Thu, Dec 12, 2013 at 12:11 PM, Teresa Johnson <tejohn...@google.com> wrote: >> On Wed, Dec 11, 2013 at 10:05 PM, Teresa Johnson <tejohn...@google.com> >> wrote: >>> 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. >>> >>> I changed this so gcov_var is no longer static, but global as before. >>> >>>> >>>> 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? >>> >>> I'm not sure I understand the issue here? The patch basically moves >>> the same includes into libgcov.h, and includes that instead. I see >>> many other header files in gcc that include other headers. >>> >>>>> @@ -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? >>> >>> This has been removed. >>> >>>> >>>> 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 >>> >>> The libgcov.h related changes are in the attached patch. I think it >>> addresses your concerns. Bootstrapped and tested on >>> x86-64-unknown-linux-gnu. A profiledbootstrap is in progress. >>> >>> Ok for trunk if profiledbootstrap passes? >> >> Both a profiledbootstrap and LTO profiledbootstrap pass. >> >> Teresa >> >>> >>> Thanks, >>> Teresa >>> >>> >>> -- >>> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 >> >> >> >> -- >> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413