This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] offline gcda profile processing tool


> 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....
> +#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.
> +
> +/* Arc transitions.  */
> +DEF_GCOV_COUNTER(GCOV_COUNTER_ARCS=0, "arcs", __gcov_merge_add)

Is =0 really needed here? It looks bit ugly ;)
> 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?

>  
>  /* 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?
> +
> +/*extern gcov_type gcov_read_counter_mem();
> +extern unsigned gcov_get_merge_weight(); */
> +
> +/* TBD: xur */

Forgoten hacks?  What is xur?
> +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
> +
> +/* 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?
> +      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.

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)

Thanks,
Honza


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]