Re: [PATCH] offline gcda profile processing tool

[Jan Hubicka <hubicka@ucw.cz>]

> >>  /* 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.

Note sure if code efficiency wins here over convolutin APIs, but I guess
it is OK.
Please update comment to explain how it is used, so we keep track of it
more easily.

> 2014-05-20  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 functions 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.
> 	* gcc/doc/gcc.texi: Include gcov-tool.texi.
> 	* gcc/doc/gcov-tool.texi: Document for gcov-tool.

OK, with changes bellow.
I apologize for the delay - it is a long patch and I am bit swamped in tasks
these days.

> Index: gcc/gcov-tool.c
> ===================================================================
> --- gcc/gcov-tool.c	(revision 0)
> +++ gcc/gcov-tool.c	(revision 0)
> @@ -0,0 +1,466 @@
> +/* Gcc offline profile processing tool support. */
> +/* Compile this one with gcc.  */

What "compile this one with gcc" means? :)
> +
> +static int verbose;

Perhaps in C++ times, it could be bool and have comment in front of it
(per coding standards)
> +
> +/* Remove file NAME if it has a gcda suffix. */
> +
> +static int
> +unlink_gcda_file (const char *name,
> +                  const struct stat *status ATTRIBUTE_UNUSED,
> +                  int type ATTRIBUTE_UNUSED,
> +                  struct FTW *ftwbuf ATTRIBUTE_UNUSED)
> +{
> +  int ret = 0;
> +  int len = strlen (name);
> +  int len1 = strlen (GCOV_DATA_SUFFIX);
> +
> +  if (len > len1 && !strncmp (len -len1 + name, GCOV_DATA_SUFFIX, len1))
> +    remove (name);
> +
> +  if (ret)

ret is not set here.

> +    {
> +      fnotice (stderr, "error in removing %s\n", name);
> +      exit (FATAL_EXIT_CODE);
> +    }

I think you want to use fatal here, that exists for you.

> +
> +/* Merging profile D1 and D2 with weight as W1 and W2, respectively.
> +   The result profile is written to directory OUT.
> +   Return 0 on success.  */
> +
> +static int
> +profile_merge (const char *d1, const char *d2, const char *out, int w1, int w2)
> +{
> +  char *pwd;
> +  int ret;
> +  struct gcov_info * d1_profile;
> +  struct gcov_info * d2_profile;
> +
> +
> +  d1_profile = gcov_read_profile_dir (d1, 0);
> +  if (!d1_profile)
> +    return 1;
> +
> +  if (d2)
> +    {
> +      d2_profile = gcov_read_profile_dir (d2, 0);
> +      if (!d2_profile)
> +        return 1;
> +
> +      /* The actual merge: we overwrite to d1_profile.  */
> +      ret = gcov_profile_merge (d1_profile, d2_profile, w1, w2);
> +
> +      if (ret)
> +        return ret;
> +    }
> +
> +  /* Output new profile.  */
> +  unlink_profile_dir (out);
> +  mkdir (out, 0755);
> +  pwd = getcwd (NULL, 0);
> +  gcc_assert (pwd);
> +  ret = chdir (out);
> +  gcc_assert (ret == 0);
> +
> +  set_gcov_list (d1_profile);
> +  gcov_exit ();
> +
> +  ret = chdir (pwd);

ret is unused here, I suppose we should fatal if it fails.
I wonder how safe this is considered in command tools (i.e. change directory and go back)
> +  free (pwd);
> +  return 0;
> +}
> +
> +/* Usage message for profile merge.  */
> +
> +static void
> +print_merge_usage_message (int error_p)
> +{
> +  FILE *file = error_p ? stderr : stdout;
> +
> +  fnotice (file, "  merge [options] <dir1> <dir2>         Merge coverage file contents\n");
> +  fnotice (file, "    -v, --verbose                       Verbose mode\n");
> +  fnotice (file, "    -o, --output <dir>                  Output directory\n");
> +  fnotice (file, "    -w, --weight <w1,w2>                Set weights (float point values)\n");
> +}
> +
> +static const struct option merge_options[] =
> +{
> +  { "verbose",                no_argument,       NULL, 'v' },
> +  { "output",                 required_argument, NULL, 'o' },
> +  { "weight",                 required_argument, NULL, 'w' },
> +  { 0, 0, 0, 0 }
> +};
> +
> +/* Print merge usage and exit.  */
> +
> +static void
> +merge_usage (void)
> +{
> +  fnotice (stderr, "Merge subcomand usage:");
> +  print_merge_usage_message (true);
> +  exit (FATAL_EXIT_CODE);
> +}
> +

> +/* If N_VAL is no-zero, normalize the profile by setting the largest counter
> +   counter value to N_VAL and scale others counters proportionally.
> +   Otherwise, multiply the all counters by SCALE.  */
> +
> +static int
> +profile_rewrite (const char *d1, const char *out, long long n_val,
> +                 float scale, int n, int d)
> +{
> +  char *pwd;
> +  int ret;
> +  struct gcov_info * d1_profile;
> +
> +
> +  d1_profile = gcov_read_profile_dir (d1, 0);
> +  if (!d1_profile)
> +    return 1;
> +
> +  /* Output new profile.  */
> +  unlink_profile_dir (out);
> +  mkdir (out, 0755);
> +  pwd = getcwd (NULL, 0);
> +  gcc_assert (pwd);
> +  ret = chdir (out);
> +  gcc_assert (ret == 0);

Perhaps we want user readable error messages in both cases? 
I think both can legally fail if you remove the CWD or when you specify wrong
output directory.
> +
> +  ret = chdir (pwd);
> +  free (pwd);
Again, I think you want to check (probably have chdir wrapper that will do the fatal for you?)
> +        case 's':
> +          ret = 0;
> +          do_scaling = 1;
> +          if (strstr (optarg, "/"))
> +            {
> +              ret = sscanf (optarg, "%d/%d", &numerator, &denominator);
> +              if (ret == 2)
> +                {
> +                  gcc_assert (numerator >= 0);
> +                  gcc_assert (denominator > 0);
> +                }
> +            }
> +          if (ret != 2)
> +            {
> +              ret = sscanf (optarg, "%f", &scale);
> +              gcc_assert (ret == 1);
> +              denominator = 0;
> +            }

Probably user readable sanity checking here, too (rather than assert)

> +
> +          if (scale < 0.0)
> +            {
> +              fnotice (stderr, "scale needs to be non-negative\n");
> +              exit (FATAL_EXIT_CODE);

fatal (here and at other cases)

> +/* This part of the code is to merge profile counters.  */
> +
> +static gcov_type *gcov_value_buf;
> +static gcov_unsigned_t gcov_value_buf_size;
> +static gcov_unsigned_t gcov_value_buf_pos;
> +static unsigned gcov_merge_weight;

Add comments for individual vars.

> +@c Copyright (C) 1996-2014 Free Software Foundation, Inc.
> +@c This is part of the GCC manual.
> +@c For copying conditions, see the file gcc.texi.
> +
> +@ignore
> +@c man begin COPYRIGHT
> +Copyright @copyright{} 1996-2014 Free Software Foundation, Inc.

Perhaps just 2014?
> +
> +Permission is granted to copy, distribute and/or modify this document
> +under the terms of the GNU Free Documentation License, Version 1.3 or
> +any later version published by the Free Software Foundation; with the
> +Invariant Sections being ``GNU General Public License'' and ``Funding
> +Free Software'', the Front-Cover texts being (a) (see below), and with
> +the Back-Cover Texts being (b) (see below).  A copy of the license is
> +included in the gfdl(7) man page.
> +
> +(a) The FSF's Front-Cover Text is:
> +
> +     A GNU Manual
> +
> +(b) The FSF's Back-Cover Text is:
> +
> +     You have freedom to copy and modify this GNU Manual, like GNU
> +     software.  Copies published by the Free Software Foundation raise
> +     funds for GNU development.
> +@c man end
> +@c Set file name and title for the man page.
> +@setfilename gcov-tool
> +@settitle offline gcda profile processing tool
> +@end ignore
> +
> +@node Gcov-tool
> +@chapter @command{gcov-tool}---an offline gcda profile processing tool
> +
> +@command{gcov-tool} is a tool you can use in conjunction with GCC to
> +manipulate or process gcda profile files offline.
> +
> +@menu
> +* Gcov-tool Intro::             Introduction to gcov-tool.
> +* Invoking Gcov-tool::          How to use gcov-tool.
> +@end menu
> +
> +@node Gcov-tool Intro
> +@section Introduction to @command{gcov-tool}
> +@c man begin DESCRIPTION
> +
> +@command{gcov-tool} is an offline tool to process gcc's gcda profile files.
> +
> +Current gcov-tool supports the following functionalities:
> +
> +@itemize @bullet
> +@item
> +merge two sets of profiles with weights.
> +
> +@item
> +read one set of profile and rewrite profile contents. One can scale or
> +normalize the count values.
> +@end itemize
> +
> +Note that for the merging operation, this profile generated offline may
> +contain a slight different values from the online merged profile. Here are
> +a list of typical differences:
> +
> +@itemize @bullet
> +@item
> +histogram difference: This offline tool recomputes the histogram after merging
> +the counters. The resulting histogram, therefore, is precise. The online
> +merging does not have this capability -- the histogram is merged from two
> +histograms and the result is an approximation.
> +
> +@item
> +summary checksum difference: Summary checksum uses a CRC32 operation. The value
> +depends on the link list order of gcov-info objects. This order is different in
> +gcov-tool from that in the online merge. It's expected to have different
> +summary checksums. It does not really matter as the compiler does not use this
> +checksum anywhere.
> +
> +@item
> +value profile counter values difference: Some counter values for value profile
> +are runtime dependent, like heap addresses. It's normal to see some difference
> +in these kind of counters.
> +@end itemize
> +
> +@c man end
> +
> +@node Invoking Gcov-tool
> +@section Invoking @command{gcov-tool}
> +
> +@smallexample
> +gcov-tool @r{[}@var{global-options}@r{]} SUB_COMMAND
> +@r{[}@var{sub_command-options}@r{]} @var{profile_dir}
> +@end smallexample
> +
> +@command{gcov-tool} accepts the following options:
> +
> +@ignore
> +@c man begin SYNOPSIS
> +gcov-tool [@option{-v}|@option{--version}] [@option{-h}|@option{--help}]
> +
> +gcov-tool merge [merge-options] @var{directory1} @var{directory2}
> +     [@option{-v}|@option{--verbose}]
> +     [@option{-o}|@option{ --output} @var{directory}]
> +     [@option{-w}|@option{--weight} @var{w1,w2}]
> +
> +gcov-tool rewrite [rewrite-options] @var{directory}
> +     [@option{-v}|@option{--verbose}]
> +     [@option{-o}|@option{--output} @var{directory}]
> +     [@option{-s}|@option{--scale} @var{float_or_simple-frac_value}]
> +     [@option{-n}|@option{--normalize} @var{long_long_value}]
> +@c man end
> +@c man begin SEEALSO
> +gpl(7), gfdl(7), fsf-funding(7), gcc(1), gcov(1) and the Info entry for
> +@file{gcc}.
> +@c man end
> +@end ignore
> +
> +@c man begin OPTIONS
> +@table @gcctabopt
> +@item -h
> +@itemx --help
> +Display help about using @command{gcov-tool} (on the standard output), and
> +exit without doing any further processing.
> +
> +@item -v
> +@itemx --version
> +Display the @command{gcov-tool} version number (on the standard output),
> +and exit without doing any further processing.
> +
> +@item merge
> +Merge two profile directories.
> +
> +@table @gcctabopt
> +@item -v
> +@itemx --verbose
> +Set the verbose mode.
> +
> +@item -o @var{directory}
> +@itemx --output @var{directory}
> +Set the output profile directory. Default output directory name is
> +@var{merged_profile}.
> +
> +@item -w @var{w1,w2}
> +@itemx --weight @var{w1,w2}
> +Set the merge weights of the @var{directory1} and @var{directory2},
> +respectively. The default weights are 1 for both.
> +@end table
> +
> +@item rewrite
> +Read the specified profile directory and rewrite to a new directory.
> +
> +@table @gcctabopt
> +@item -v
> +@itemx --verbose
> +Set the verbose mode.
> +
> +@item -o @var{directory}
> +@itemx --output @var{directory}
> +Set the output profile directory. Default output name is @var{rewrite_profile}.
> +
> +@item -s @var{float_or_simple-frac_value}
> +@itemx --scale @var{float_or_simple-frac_value}
> +Scale the profile counters. The specified value can be in floating point value,
> +or simple fraction value form, such 1, 2, 2/3, and 5/3.
> +
> +@item -n @var{long_long_value}
> +@itemx --normalize <long_long_value>
> +Normalize the profile. The specified value is the max counter value
> +in the new profile.
> +
> +@end table
> +@end table
> +
> +@c man end

Thanks for writting the docs.  Perhaps you could add bit extra information about when one would
like to do the supported operations.

Thanks!
Honza