Re: [PATCH] offline gcda profile processing tool

[Rong Xu <xur at google dot com>]

On Thu, May 15, 2014 at 1:37 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Hi, Honza,
>>
>> Please find the new patch in the attachment. This patch integrated
>> your earlier suggestions. The noticeable changes are:
>> (1) build specialized object for libgcov-driver.c and libgcov-merge.c
>> and link into gcov-tool, rather including the source file.
>> (2) split some gcov counter definition code to gcov-counter.def to
>> avoid code duplication.
>> (3) use a macro for gcov_read_counter(), so gcov-tool can use existing
>> code in libgcov-merge.c with minimal change.
>>
>> This patch does not address the suggestion of creating a new directory
>> for libgcov. I agree with you that's a much better
>> and cleaner structure we should go for. We can do that in follow-up patches.
>
> Yep, lets do this incrementally. thanks!
>>
>> I tested this patch with boostrap and profiledbootstrap. Other tests
>> are on-going.
>>
>> Thanks,
>>
>> -Rong
>> 2014-05-01  Rong Xu  <xur@google.com>
>>
>>       * gcc/gcov-io.c (gcov_position): Make avaialble to gcov-tool.
>>       (gcov_is_error): Ditto.
>>       (gcov_read_string): Ditto.
>>       (gcov_read_sync): Ditto.
>>       * gcc/gcov-io.h: Move counter defines to gcov-counter.def.
>>       * gcc/gcov-dump.c (tag_counters): Use gcov-counter.def.
>>       * gcc/coverage.c: Ditto.
>>       * gcc/gcov-tool.c: Offline gcda profile processing tool.
>>         (unlink_gcda_file): Remove one gcda file.
>>       (unlink_profile_dir): Remove gcda files from the profile path.
>>       (profile_merge): Merge two profiles in directory.
>>       (print_merge_usage_message): Print merge usage.
>>       (merge_usage): Print merge usage and exit.
>>       (do_merge): Driver for profile merge sub-command.
>>       (profile_rewrite): Rewrite profile.
>>       (print_rewrite_usage_message): Print rewrite usage.
>>       (rewrite_usage): Print rewrite usage and exit.
>>       (do_rewrite): Driver for profile rewrite sub-command.
>>       (print_usage): Print gcov-info usage and exit.
>>       (print_version): Print gcov-info version.
>>       (process_args): Process arguments.
>>       (main): Main routine for gcov-tool.
>>       * gcc/Makefile.in: Build and install gcov-tool.
>>       * gcc/gcov-counter.def: New file split from gcov-io.h.
>>       * libgcc/libgcov-driver.c (gcov_max_filename): Make available
>>         to gcov-tool.
>>       * libgcc/libgcov-merge.c (__gcov_merge_add): Replace
>>         gcov_read_counter() with a Macro.
>>       (__gcov_merge_ior): Ditto.
>>       (__gcov_merge_time_profile): Ditto.
>>       (__gcov_merge_single): Ditto.
>>       (__gcov_merge_delta): Ditto.
>>       * libgcc/libgcov-util.c (void gcov_set_verbose): Set the verbose flag
>>         in the utility functions.
>>       (set_fn_ctrs): Utility function for reading gcda files to in-memory
>>         gcov_list object link lists.
>>       (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_read_counter_mem): Ditto.
>>       (gcov_get_merge_weight): Ditto.
>>       (merge_wrapper): A wrapper function that calls merging handler.
>>       (gcov_merge): Merge two gcov_info objects with weights.
>>       (find_match_gcov_info): Find the matched gcov_info in the list.
>>       (gcov_profile_merge): Merge two gcov_info object lists.
>>       (__gcov_add_counter_op): Process edge profile counter values.
>>       (__gcov_ior_counter_op): Process IOR profile counter values.
>>       (__gcov_delta_counter_op): Process delta profile counter values.
>>       (__gcov_single_counter_op): Process single  profile counter values.
>>       (fp_scale): Callback function for float-point scaling.
>>       (int_scale): Callback function for integer fraction scaling.
>>       (gcov_profile_scale): Scaling profile counters.
>>       (gcov_profile_normalize): Normalize profile counters.
>>       * libgcc/libgcov.h: Add headers and macros for gcov-tool use.
>>         (GCOV_GET_COUNTER): New.
>>         (GCOV_GET_COUNTER_TARGET): Ditto.
>>         (struct gcov_info): Make the functions field mutable in gcov-tool
>>         compilation.
>>
>> Index: gcc/gcov-io.c
>> ===================================================================
>> --- gcc/gcov-io.c     (revision 209981)
>> +++ gcc/gcov-io.c     (working copy)
>> @@ -64,7 +64,11 @@ GCOV_LINKAGE struct gcov_var
>>  } gcov_var;
>>
>>  /* Save the current position in the gcov file.  */
>> -static inline gcov_position_t
>> +/* We need to expose this function when compiling for gcov-tool.  */
>> +#ifndef IN_GCOV_TOOL
>> +static inline
>> +#endif
>> +gcov_position_t
>>  gcov_position (void)
>>  {
>>    gcc_assert (gcov_var.mode > 0);
>> @@ -72,7 +76,11 @@ gcov_position (void)
>>  }
>>
>>  /* Return nonzero if the error flag is set.  */
>> -static inline int
>> +/* We need to expose this function when compiling for gcov-tool.  */
>> +#ifndef IN_GCOV_TOOL
>> +static inline
>> +#endif
>> +int
>
> I am still not too happy about the ifdef noise here, but I suppose it is bettter
> than bloating libgcov by making those hidden everywhere....

Thanks for the understanding. It does not look good to me either.
I'll keep this as it's. We can change it later if we find a better way.

>> +#ifdef DEF_GCOV_COUNTER
>> +#undef DEF_GCOV_COUNTER
>> +#endif
>> +#define DEF_GCOV_COUNTER(COUNTER, NAME, MERGE_FN) COUNTER,
>> +enum {
>> +#include "gcov-counter.def"
>> +GCOV_COUNTERS
>> +};
>
> Please consistently undef DEF_GCOV_COUNTER after each use and
> remove the ifdef/undef/endif blocks
> I think it is cleaner, even though we seem to have multiple practices
> when dealing with .def files across the tree.

Fixed.

>> +
>> +/* Arc transitions.  */
>> +DEF_GCOV_COUNTER(GCOV_COUNTER_ARCS=0, "arcs", __gcov_merge_add)
>
> Is =0 really needed here? It looks bit ugly ;)

Not really needed as enum values start with 0 by default. Removed "=0".

>> Index: libgcc/libgcov-driver.c
>> ===================================================================
>> --- libgcc/libgcov-driver.c   (revision 209981)
>> +++ libgcc/libgcov-driver.c   (working copy)
>> @@ -77,7 +77,11 @@ set_gcov_list (struct gcov_info *head)
>>  }
>>
>>  /* Size of the longest file name. */
>> -static size_t gcov_max_filename = 0;
>> +/* We need to expose this static variable when compiling for gcov-tool.  */
>> +#ifndef IN_GCOV_TOOL
>> +static
>> +#endif
>> +size_t gcov_max_filename = 0;
>
>
> Why max_filename needs to be exported?

For code efficiency, we allocate the gi_filename buffer only once in
the dumping, using the maximum filename length, which
is set in gcov_init(). For gcov-tool, we don't have gcov_init, and we
set the value when reading the gcda files.
Now libgcov-driver.o is a separated compilation unit. I need to make
then static gi_filename global to allow the access.

>
>>
>>  /* Flag when the profile has already been dumped via __gcov_dump().  */
>>  static int gcov_dump_complete;
>> Index: libgcc/libgcov-merge.c
>> ===================================================================
>> --- libgcc/libgcov-merge.c    (revision 209981)
>> +++ libgcc/libgcov-merge.c    (working copy)
>> @@ -53,7 +53,7 @@ void
>>  __gcov_merge_add (gcov_type *counters, unsigned n_counters)
>>  {
>>    for (; n_counters; counters++, n_counters--)
>> -    *counters += gcov_read_counter ();
>> +    *counters += GCOV_GET_COUNTER;
>
> We seem to be on transition to C++ writting style, why we don't make
> GCOV_GET_COUNTER an inline function?

Sure, I changed them to static inline functions.

>> +
>> +/*extern gcov_type gcov_read_counter_mem();
>> +extern unsigned gcov_get_merge_weight(); */
>> +
>> +/* TBD: xur */
>
> Forgoten hacks?  What is xur?

That from previous patch that I forgot to delete. Cleaned.

>> +extern gcov_position_t gcov_position();
>> +extern int gcov_is_error();
>> +extern gcov_unsigned_t gcov_max_filename;
>> +
>> +/* We need the dumping and merge part of code in libgcov.  */
>> +/*#include "libgcov-driver.c"
>> +#include "libgcov-merge.c" */
>
> Here too

Cleaned.

>> +
>> +/* Verbose mode for debug.  */
>> +static int verbose;
>> +
>> +/* Set verbose flag.  */
>> +void gcov_set_verbose (void)
>> +{
>> +  verbose = 1;
>> +}
>> +
>> +/* The following part is to read Gcda and reconstruct GCOV_INFO.  */
>> +
>> +#include "obstack.h"
>> +#include <unistd.h>
>> +#include <ftw.h>
>
> Why the includes appear after definitions/inline functions?

There is no particular reason for this. I moved them to the head of the file.

>> +      if (gfi_ptr1->cfg_checksum != gfi_ptr2->cfg_checksum)
>> +        {
>> +          fprintf (stderr, "in %s, cfg_checksum mismatch, skipping\n",
>> +                  info1->filename);
>
> It is custom for GCC related tools to use error/warning/fnotice.  GCOV runtime is an exception
> since it is not linked with diagnostic.c, but otherwise I think we should use it in gcov-tool,
> too.  Please update it.

Thanks for this info. Updated all use to warning or fnotice.

>
> Please also add texinfo documentation for the tool, like there is gcov.texi.
> The patch looks OK with these changes (or rahter I think we can solve other issues
> incrementally)

Included in the new patch. Could you take a look? Thanks a lot!


>
> Thanks,
> Honza

Attachment: patch_set_4.txt
Description: Text document