Re: [PATCH] gcov-tool: Allow merging of empty profile lists

["Martin Liška" <mliska@suse.cz>]

On 3/23/22 10:34, Sebastian Huber wrote:

Hello.

Thanks for the patch. Note we're in stage4, so the patch can eventually go
in in the next stage1.

> The gcov_profile_merge() already had code to deal with profile information
> which had no counterpart to merge with.  For profile information from files
> with no associated counterpart, the profile information is simply used as is
> with the weighting transformation applied.  Make sure that gcov_profile_merge()
> works with an empty target profile list.  Return the merged profile list.

Can you please provide a simple demo with a pair of profiles
where I'll see what changes?

> 
> gcc/
> 	* gcov-tool.cc (gcov_profile_merge): Adjust return type.
> 	(profile_merge): Allow merging of directories which contain no profile
> 	files.
> 
> libgcc/
> 	* libgcov-util.c (gcov_profile_merge): Return the list of merged
> 	profiles.  Accept empty target and source profile lists.
> ---
>   gcc/gcov-tool.cc      | 27 ++++++++++-----------------
>   libgcc/libgcov-util.c | 15 +++++++++------
>   2 files changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/gcc/gcov-tool.cc b/gcc/gcov-tool.cc
> index f4e42ae763c..2e4083a664d 100644
> --- a/gcc/gcov-tool.cc
> +++ b/gcc/gcov-tool.cc
> @@ -40,7 +40,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>   #endif
>   #include <getopt.h>
>   
> -extern int gcov_profile_merge (struct gcov_info*, struct gcov_info*, int, int);
> +extern struct gcov_info *gcov_profile_merge (struct gcov_info*,
> +					     struct gcov_info*, int, int);
>   extern int gcov_profile_overlap (struct gcov_info*, struct gcov_info*);
>   extern int gcov_profile_normalize (struct gcov_info*, gcov_type);
>   extern int gcov_profile_scale (struct gcov_info*, float, int, int);
> @@ -141,26 +142,18 @@ profile_merge (const char *d1, const char *d2, const char *out, int w1, int w2)
>   {
>     struct gcov_info *d1_profile;
>     struct gcov_info *d2_profile;
> -  int ret;
> +  struct gcov_info *merged_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;
> +  d2_profile = gcov_read_profile_dir (d2, 0);

Is it fine calling 'gcov_read_profile_dir (d2, 0)' without 'if (d2)'?

>   
> -      /* The actual merge: we overwrite to d1_profile.  */
> -      ret = gcov_profile_merge (d1_profile, d2_profile, w1, w2);
> +  /* The actual merge: we overwrite to d1_profile.  */
> +  merged_profile = gcov_profile_merge (d1_profile, d2_profile, w1, w2);
>   
> -      if (ret)
> -        return ret;
> -    }
> -
> -  gcov_output_files (out, d1_profile);
> +  if (merged_profile)
> +    gcov_output_files (out, merged_profile);
> +  else if (verbose)
> +    fnotice (stdout, "no profile files were merged\n");
>   
>     return 0;
>   }
> diff --git a/libgcc/libgcov-util.c b/libgcc/libgcov-util.c
> index ba7fb924b53..e5496f4ade2 100644
> --- a/libgcc/libgcov-util.c
> +++ b/libgcc/libgcov-util.c
> @@ -677,13 +677,13 @@ find_match_gcov_info (struct gcov_info **array, int size,
>      Return 0 on success: without mismatch.
>      Reutrn 1 on error.  */

Please adjust the function comment.

Cheers,
Martin

>   
> -int
> +struct gcov_info *
>   gcov_profile_merge (struct gcov_info *tgt_profile, struct gcov_info *src_profile,
>                       int w1, int w2)
>   {
>     struct gcov_info *gi_ptr;
>     struct gcov_info **tgt_infos;
> -  struct gcov_info *tgt_tail;
> +  struct gcov_info **tgt_tail;
>     struct gcov_info **in_src_not_tgt;
>     unsigned tgt_cnt = 0, src_cnt = 0;
>     unsigned unmatch_info_cnt = 0;
> @@ -703,7 +703,10 @@ gcov_profile_merge (struct gcov_info *tgt_profile, struct gcov_info *src_profile
>     for (gi_ptr = tgt_profile, i = 0; gi_ptr; gi_ptr = gi_ptr->next, i++)
>       tgt_infos[i] = gi_ptr;
>   
> -  tgt_tail = tgt_infos[tgt_cnt - 1];
> +  if (tgt_cnt)
> +     tgt_tail = &tgt_infos[tgt_cnt - 1]->next;
> +  else
> +     tgt_tail = &tgt_profile;
>   
>     /* First pass on tgt_profile, we multiply w1 to all counters.  */
>     if (w1 > 1)
> @@ -732,14 +735,14 @@ gcov_profile_merge (struct gcov_info *tgt_profile, struct gcov_info *src_profile
>         gi_ptr = in_src_not_tgt[i];
>         gcov_merge (gi_ptr, gi_ptr, w2 - 1);
>         gi_ptr->next = NULL;
> -      tgt_tail->next = gi_ptr;
> -      tgt_tail = gi_ptr;
> +      *tgt_tail = gi_ptr;
> +      tgt_tail = &gi_ptr->next;
>       }
>   
>     free (in_src_not_tgt);
>     free (tgt_infos);
>   
> -  return 0;
> +  return tgt_profile;
>   }
>   
>   typedef gcov_type (*counter_op_fn) (gcov_type, void*, void*);