Re: [PATCH] Fix support for multiple counters sections

[Zdenek Dvorak <rakdver at atrey dot karlin dot mff dot cuni dot cz>]

Hello,

> >Changelog:
> >
> >	* libgcov.c (gcov_exit): Cleanup and fix.
> I can't tell what is rearrangement and what is fix.

The problem was that the values array was not initialized for members above
GCOV_COUNTERS_SUMMABLE.

> >Index: libgcov.c
> >===================================================================
> >RCS file: /cvs/gcc/gcc/gcc/libgcov.c,v
> >retrieving revision 1.20
> >diff -c -3 -p -r1.20 libgcov.c
> >*** libgcov.c	10 Jul 2003 14:13:00 -0000	1.20
> >--- libgcov.c	31 Jul 2003 17:21:03 -0000
> >*************** gcov_exit (void)
> >*** 122,153 ****
> 
> >    /* Now merge each file.  */
> >--- 122,153 ----
> >    struct gcov_info *gi_ptr;
> >    struct gcov_summary this_program;
> >    struct gcov_summary all;
> >+   struct gcov_ctr_summary *cs_ptr;
> >+   const struct gcov_ctr_info *ci_ptr;
> >+   unsigned t_ix;
> >+   gcov_unsigned_t c_num;
> why have you placed these outside of the loops? Please put loop
> variable inside the smallest scope they are used in.

Because they are used in several loops with exactly the same meaning.
Declaring them separately in every of these loops is senseless and
missleading.

> >    memset (&all, 0, sizeof (all));
> >    /* Find the totals for this execution.  */
> >    memset (&this_program, 0, sizeof (this_program));
> >    for (gi_ptr = gcov_list; gi_ptr; gi_ptr = gi_ptr->next)
> >      {
> >!       ci_ptr = gi_ptr->counts;
> >!       for (t_ix = 0; t_ix < GCOV_COUNTERS_SUMMABLE; t_ix++)
> >! 	{
> >! 	  if (!((1 << t_ix) & gi_ptr->ctr_mask))
> >! 	    continue;
> >  
> >! 	  cs_ptr = &this_program.ctrs[t_ix];
> >! 	  cs_ptr->num += ci_ptr->num;
> >! 	  for (c_num = 0; c_num < ci_ptr->num; c_num++)
> >! 	    {
> >!       	      cs_ptr->sum_all += ci_ptr->values[c_num];
> >! 	      if (cs_ptr->run_max < ci_ptr->values[c_num])
> >! 		cs_ptr->run_max = ci_ptr->values[c_num];
> >! 	    }
> >! 	  ci_ptr++;
> >! 	}
> why have you split this into two loops?

I am not quite sure what you refer to here.

> >!       unsigned c_ix, f_ix, n_counts;
> >        struct gcov_ctr_summary *cs_obj, *cs_tobj, *cs_prg, *cs_tprg, 
> >        *cs_all;
> >        int error = 0;
> >        gcov_unsigned_t tag, length;
> >        gcov_position_t summary_pos = 0;
> >+       gcov_type *c_ptr;
> >+       gcov_merge_fn merge;
> Again, AFAICT these variables are only used inside a loop, there declaration
> should be there.

Done.  I was just too active when I moved the variables above.

> >        /* Totals for this object file.  */
> >!       ci_ptr = gi_ptr->counts;
> >!       for (t_ix = 0; t_ix < GCOV_COUNTERS_SUMMABLE; t_ix++)
> >! 	{
> >! 	  if (!((1 << t_ix) & gi_ptr->ctr_mask))
> >! 	    continue;
> ...
> >! 	  ci_ptr++;
> >! 	}
> >! 
> >!       c_ix = 0;
> >!       for (t_ix = 0; t_ix < GCOV_COUNTERS; t_ix++)
> >  	if ((1 << t_ix) & gi_ptr->ctr_mask)
> >  	  {
> >! 	    values[c_ix] = gi_ptr->counts[c_ix].values;
> >  	    c_ix++;
> >  	  }
> Again, why have you split the loop?

To fix the bug described above.  I have considered just adding the
initialization for the missing elements, but the performance gain
is imho negligible and it is clearer this way.

> >+       c_ix = 0;
> >+       for (t_ix = 0; t_ix < GCOV_COUNTERS; t_ix++)
> >+ 	if ((1 << t_ix) & gi_ptr->ctr_mask)
> >+ 	  {
> >+ 	    values[c_ix] = gi_ptr->counts[c_ix].values;
> >+ 	    c_ix++;
> >+ 	  }
> Why have you added this loop?

The same reason (I again just split of the loop from the preceding one).

> I'm curious whether your changes have a significant effect on the
> object size.

Why should they?  Our loop optimizer is lame, but I doubt that that much
lame.

Zdenek