From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14232 invoked by alias); 15 May 2014 20:37:09 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 14219 invoked by uid 89); 15 May 2014 20:37:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: nikam.ms.mff.cuni.cz Received: from nikam.ms.mff.cuni.cz (HELO nikam.ms.mff.cuni.cz) (195.113.20.16) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 15 May 2014 20:37:05 +0000 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id B7E9A5430C6; Thu, 15 May 2014 22:37:00 +0200 (CEST) Date: Thu, 15 May 2014 20:37:00 -0000 From: Jan Hubicka To: Rong Xu Cc: Jan Hubicka , GCC Patches , Xinliang David Li , Teresa Johnson , Dehao Chen Subject: Re: [PATCH] offline gcda profile processing tool Message-ID: <20140515203700.GA29194@kam.mff.cuni.cz> References: <20140415213850.GA23141@atrey.karlin.mff.cuni.cz> <20140417033448.GC3157@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-05/txt/msg01228.txt.bz2 > 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 > > * 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 > +#include 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