> 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 <x...@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
