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
