This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [RFC] libgcov.c re-factoring and offline profile-tool
- From: Xinliang David Li <davidxl at google dot com>
- To: Teresa Johnson <tejohnson at google dot com>
- Cc: Jan Hubicka <hubicka at ucw dot cz>, Rong Xu <xur at google dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 17 Dec 2013 09:58:17 -0800
- Subject: Re: [RFC] libgcov.c re-factoring and offline profile-tool
- Authentication-results: sourceware.org; auth=none
- References: <20131111150245 dot GB6009 at atrey dot karlin dot mff dot cuni dot cz> <CAF1bQ=RSUQdE0tkeqeHnbiNkC=PFPDU6O+zLFNA6jUxqV_aAbg at mail dot gmail dot com> <CAF1bQ=SCtewMrgjcYg4v21jA8uWy_9U=9sB9ULznS=9tdJD8mQ at mail dot gmail dot com> <CAF1bQ=TyNC_rAo1V8Zk9ovOvYAtpqVtep_h7WNLk6v6ZerdcaQ at mail dot gmail dot com> <20131206142319 dot GC32664 at kam dot mff dot cuni dot cz> <CAAe5K+UouGttAzYMwqHtyMSu2sD=KSVhGZ=5oySq=a-7oy2rwg at mail dot gmail dot com> <CAAe5K+XCo9S4Sx3wbeQcPHvyssPGV9Njoz3s7EK6j+kFkZZ9dg at mail dot gmail dot com> <CAAkRFZJ12Z_DOLgbGNqk0iFkx=aai4JxWgp2vaZdM2-4VWPWKg at mail dot gmail dot com> <CAAe5K+WZTGm-7=L-p2FO9qY4xOCteNQDsE=Eb_bPo+FPdvMR1g at mail dot gmail dot com> <CAAkRFZKq90ULrKLL30dGtHuHmX+BuRytdMzcCfz4kjy_5=JF3w at mail dot gmail dot com> <20131216223441 dot GC2519 at kam dot mff dot cuni dot cz> <CAAkRFZJ0XdzDvT4XPvHum+0ArzjYDUpepkJwJMg0=UfRk1GJiw at mail dot gmail dot com> <CAAe5K+Xas0esLkpBJBcZnPpnFf+UpPAPSDUYoXD0_wnWe83r-g at mail dot gmail dot com>
On Tue, Dec 17, 2013 at 7:48 AM, Teresa Johnson <tejohnson@google.com> wrote:
> On Mon, Dec 16, 2013 at 2:48 PM, Xinliang David Li <davidxl@google.com> wrote:
>> Ok -- gcov_write_counter and gcov_write_tag_length are qualified as
>> low level primitives for basic gcov format and probably should be kept
>> in gcov-io.c.
>>
>> gcov_rewrite is petty much libgcov runtime implementation details so I
>> think it should be moved out. gcov_write_summary is not related to
>> gcov low level format either, neither is gcov_seek. Ok for them to be
>> moved?
>
> After looking at these some more, with the idea that gcov-io.c should
> encapsulate the lower level IO routines, then I think all of these
> (including gcov_rewrite) should remain in gcov-io.c. I think
> gcov_write_summary belongs there since all of the other gcov_write_*
> are there. And gcov_seek and gcov_rewrite are both adjusting gcov_var
> fields to affect the file IO operations. And there are currently no
> references to gcov_var within libgcc/libgcov* files.
>
> So I think we should leave the patch as-is.
Sounds fine to me.
David
> Honza, is the current
> patch ok for trunk?
>
> Thanks,
> Teresa
>
>>
>> thanks,
>>
>> David
>>
>>
>> On Mon, Dec 16, 2013 at 2:34 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>> I think so -- they are private to libgcov. Honza, what do you think?
>>>
>>> Hmm, the purpose of gcov-io was to be low level IO library for the basic
>>> gcov file format. I am not sure if gcov_write_tag_length should really resist
>>> in other file than gcov_write_tag.
>>>
>>> I see a desire to isolate actual stdio calls so one can have replacement driver
>>> for i.e. Linux kernel. For that reason things like gcov_seek and friends probably
>>> should be separated, but what is reason for splitting the file handling itself?
>>>
>>> Honza
>>>>
>>>> thanks,
>>>>
>>>> David
>>>>
>>>> On Mon, Dec 16, 2013 at 1:17 PM, Teresa Johnson <tejohnson@google.com> wrote:
>>>> > On Mon, Dec 16, 2013 at 12:55 PM, Xinliang David Li <davidxl@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 <tejohnson@google.com> wrote:
>>>> >>> On Wed, Dec 11, 2013 at 10:05 PM, Teresa Johnson <tejohnson@google.com> wrote:
>>>> >>>> On Fri, Dec 6, 2013 at 6:23 AM, Jan Hubicka <hubicka@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 <xur@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 | tejohnson@google.com | 408-460-2413
>>>> >>>
>>>> >>>
>>>> >>>
>>>> >>> --
>>>> >>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>>>> >
>>>> >
>>>> >
>>>> > --
>>>> > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413