gcov_rewrite function is only needed (and defined) with IN_LIBGCOV. Should it be moved from common file gcov-io.c to libgcov.c?
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