Re: [RFC] libgcov.c re-factoring and offline profile-tool

[Rong Xu <xur at google dot com>]

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,

-Rong

On Tue, Nov 12, 2013 at 4:27 PM, Rong Xu <xur@google.com> wrote:
> The patch was checked in as r204730.
>
> Tested with
> configure with --enable-languages=all,obj-c++
> (ada is currently broken, so I was not be able test)
> bootstrap and profiledbootstrap
> regression for -m32 and -m64.
> spec2006
>
> The only semantic change is __gcov_flush() used to be under L_gcov.
> I put the function to libgcov-interface.c and made it under
> L_gcov_flush (newly added).
> So if you need this function, you have to enable L_gcov_flush in the
> libgcc/Makefile.in.
>
> Thanks,
>
> -Rong
>
> On Mon, Nov 11, 2013 at 10:19 AM, Rong Xu <xur@google.com> wrote:
>> On Mon, Nov 11, 2013 at 7:02 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>> 2013-11-04  Rong Xu  <xur@google.com>
>>>>
>>>>       * libgcc/libgcov.c: Delete as part of re-factoring.
>>>>       * libgcc/libgcov-profiler.c (__gcov_interval_profiler): Moved from
>>>>       libgcov.c
>>>>       (__gcov_pow2_profiler): Ditto.
>>>>       (__gcov_one_value_profiler_body): Ditto.
>>>>       (__gcov_one_value_profiler): Ditto.
>>>>       (__gcov_indirect_call_profiler): Ditto.
>>>>       (__gcov_indirect_call_profiler_v2): Ditto.
>>>>       (__gcov_average_profiler): Ditto.
>>>>       (__gcov_ior_profiler): Ditto.
>>>>       * libgcc/libgcov-driver.c (this_prg): Make it file scope
>>>>       static variable.
>>>>       (all_prg): Ditto.
>>>>       (crc32): Ditto.
>>>>       (gi_filename): Ditto.
>>>>       (fn_buffer): Ditto.
>>>>       (sum_buffer): Ditto.
>>>>         (struct gcov_filename_aux): New types to store auxiliary information
>>>>         for gi_filename.
>>>>       (gcov_version): Moved from libgcov.c.
>>>>       (crc32_unsigned): Ditto.
>>>>       (gcov_histogram_insert): Ditto.
>>>>       (gcov_compute_histogram): Ditto.
>>>>       (gcov_exit): Ditto.
>>>>       (gcov_clear): Ditto.
>>>>       (__gcov_init): Ditto.
>>>>       (gcov_exit_compute_summary): New function split from gcov_exit().
>>>>       (gcov_exit_merge_gcda): Ditto.
>>>>       (gcov_exit_write_gcda): Ditto.
>>>>       (gcov_exit_dump_gcov): Ditto.
>>>>       * libgcc/libgcov-interface.c (init_mx): Moved from libgcov.c.
>>>>       (init_mx_once): Ditto.
>>>>       (__gcov_flush): Ditto.
>>>>       (__gcov_reset): Ditto.
>>>>       (__gcov_dump): Ditto.
>>>>       (__gcov_fork): Ditto.
>>>>       (__gcov_execl): Ditto.
>>>>       (__gcov_execlp): Ditto.
>>>>       (__gcov_execle): Ditto.
>>>>       (__gcov_execv): Ditto.
>>>>       (__gcov_execvp): Ditto.
>>>>       (__gcov_execve): Ditto.
>>>>       * libgcc/libgcov-driver-system.c (gcov_error): New utility function.
>>>>       (allocate_filename_struct): New function split from gcov_exit().
>>>>       (gcov_exit_open_gcda_file): Ditto.
>>>>         (create_file_directory): Moved from libgcov.c.
>>>>       * libgcc/libgcov-merge.c:
>>>>       (__gcov_merge_add): Moved from libgcov.c.
>>>>       (__gcov_merge_ior): Ditto.
>>>>       (__gcov_merge_single): Ditto.
>>>>       (__gcov_merge_delta): Ditto.
>>>>       * libgcc/Makefile.in: Change to build newly added files.
>>>>       * gcc/gcov-io.h (__gcov_merge_ior): Add the decl to avoid warning.
>>>>
>>>
>>> Hi,
>>> the patch looks resonable. I take your word on the fact that there are no changes
>>> in the code, I did not read it all ;)))
>>
>> We did split gcov_exit() into more functions.
>> But, semantically the code is the same.
>>
>>>> Index: libgcc/Makefile.in
>>>> ===================================================================
>>>> --- libgcc/Makefile.in        (revision 204285)
>>>> +++ libgcc/Makefile.in        (working copy)
>>>> @@ -853,17 +853,37 @@ include $(iterator)
>>>>  # Build libgcov components.
>>>>
>>>>  # Defined in libgcov.c, included only in gcov library
>>>> -LIBGCOV = _gcov _gcov_merge_add _gcov_merge_single _gcov_merge_delta \
>>>> -    _gcov_fork _gcov_execl _gcov_execlp _gcov_execle \
>>>> -    _gcov_execv _gcov_execvp _gcov_execve _gcov_reset _gcov_dump \
>>>> -    _gcov_interval_profiler _gcov_pow2_profiler _gcov_one_value_profiler \
>>>> +##LIBGCOV = _gcov _gcov_merge_add _gcov_merge_single _gcov_merge_delta \
>>>> +##    _gcov_fork _gcov_execl _gcov_execlp _gcov_execle \
>>>> +##    _gcov_execv _gcov_execvp _gcov_execve _gcov_reset _gcov_dump \
>>>> +##    _gcov_interval_profiler _gcov_pow2_profiler _gcov_one_value_profiler \
>>>> +##    _gcov_indirect_call_profiler _gcov_average_profiler _gcov_ior_profiler \
>>>> +##    _gcov_merge_ior _gcov_indirect_call_profiler_v2
>>>
>>> Probably no need for this commnet block.
>>
>> Yes. I ready remove in the most recently patch.
>>
>>>> +
>>>> +LIBGCOV_MERGE = _gcov_merge_add _gcov_merge_single _gcov_merge_delta _gcov_merge_ior
>>>> +LIBGCOV_PROFILER = _gcov_interval_profiler _gcov_pow2_profiler _gcov_one_value_profiler \
>>>>      _gcov_indirect_call_profiler _gcov_average_profiler _gcov_ior_profiler \
>>>> -    _gcov_merge_ior _gcov_indirect_call_profiler_v2
>>>> +    _gcov_indirect_call_profiler_v2
>>>> +LIBGCOV_INTERFACE = _gcov_flush _gcov_fork _gcov_execl _gcov_execlp _gcov_execle \
>>>> +    _gcov_execv _gcov_execvp _gcov_execve _gcov_reset _gcov_dump
>>>> +LIBGCOV_DRIVER = _gcov
>>>>
>>>> -libgcov-objects = $(patsubst %,%$(objext),$(LIBGCOV))
>>>> +libgcov-merge-objects = $(patsubst %,%$(objext),$(LIBGCOV_MERGE))
>>>> +libgcov-profiler-objects = $(patsubst %,%$(objext),$(LIBGCOV_PROFILER))
>>>> +libgcov-interface-objects = $(patsubst %,%$(objext),$(LIBGCOV_INTERFACE))
>>>> +libgcov-driver-objects = $(patsubst %,%$(objext),$(LIBGCOV_DRIVER))
>>>> +libgcov-objects = $(libgcov-merge-objects) $(libgcov-profiler-objects) \
>>>> +                 $(libgcov-interface-objects) $(libgcov-driver-objects)
>>>>
>>>> -$(libgcov-objects): %$(objext): $(srcdir)/libgcov.c
>>>> -     $(gcc_compile) -DL$* -c $(srcdir)/libgcov.c
>>>> +$(libgcov-merge-objects): %$(objext): $(srcdir)/libgcov-merge.c
>>>> +     $(gcc_compile) -DL$* -c $(srcdir)/libgcov-merge.c
>>>> +$(libgcov-profiler-objects): %$(objext): $(srcdir)/libgcov-profiler.c
>>>> +     $(gcc_compile) -DL$* -c $(srcdir)/libgcov-profiler.c
>>>> +$(libgcov-interface-objects): %$(objext): $(srcdir)/libgcov-interface.c
>>>> +     $(gcc_compile) -DL$* -c $(srcdir)/libgcov-interface.c
>>>> +$(libgcov-driver-objects): %$(objext): $(srcdir)/libgcov-driver.c \
>>>> +  $(srcdir)/libgcov-driver-system.c
>>>> +     $(gcc_compile) -DL$* -c $(srcdir)/libgcov-driver.c
>>>>
>>>>
>>>>  # Static libraries.
>>>> Index: gcc/gcov-io.h
>>>> ===================================================================
>>>> --- gcc/gcov-io.h     (revision 204285)
>>>> +++ gcc/gcov-io.h     (working copy)
>>>> @@ -515,6 +515,8 @@ extern void __gcov_merge_ior (gcov_type *, unsigne
>>>>  extern void __gcov_interval_profiler (gcov_type *, gcov_type, int, unsigned);
>>>>  extern void __gcov_pow2_profiler (gcov_type *, gcov_type);
>>>>  extern void __gcov_one_value_profiler (gcov_type *, gcov_type);
>>>> +extern void __gcov_indirect_call_profiler (gcov_type*, gcov_type,
>>>> +                                           void*, void*);
>>>>  extern void __gcov_indirect_call_profiler_v2 (gcov_type, void *);
>>>>  extern void __gcov_average_profiler (gcov_type *, gcov_type);
>>>>  extern void __gcov_ior_profiler (gcov_type *, gcov_type);
>>>
>>> The patch is OK with the change above.
>>
>> Thanks for the OK. I'll do some final tests on a clean client. Then I'll commit.
>>
>>> Honza

Attachment: gcovtool_patch3.txt
Description: Text document