> On Wed, Aug 29, 2012 at 6:12 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
> >> Index: libgcc/libgcov.c
> >> ===================================================================
> >> --- libgcc/libgcov.c (revision 190736)
> >> +++ libgcc/libgcov.c (working copy)
> >> @@ -276,6 +276,78 @@ gcov_version (struct gcov_info *ptr, gcov_unsigned
> >> return 1;
> >> }
> >>
> >> +/* Insert counter VALUE into HISTOGRAM. */
> >> +
> >> +static void
> >> +gcov_histogram_insert(gcov_bucket_type *histogram, gcov_type value)
> >> +{
> >> + unsigned i;
> >> +
> >> + i = gcov_histo_index(value);
> >> + gcc_assert (i < GCOV_HISTOGRAM_SIZE);
> > Does checking_assert work in libgcov? I do not think internal consistency
> > check
> > should go to --enable-checking=release libgcov. We want to maintain it as
> > lightweight as possible. (I see there are two existing gcc_asserts, since
> > they
> > report file format corruption, I think they should give better diagnostic).
>
> gcc_checking_assert isn't available, since tsystem.h not system.h is
> included. I could probably just remove the assert (to be safe,
> silently return if i is out of bounds?).
I think just removing the assert is fine here: only way it can trigger is when
GCOV_HISTOGRAM_SIZE is wrong and it ought not to be.
>
> >From my understanding of the mode attribute meanings, which I thought
> are defined in terms of the number of smallest addressable units, the
> code in gcov-io.h that sets up the gcov_type typedef will always end
> up with a gcov_type that is 32 or 64 bits? I.e. when BITS_PER_UNIT is
> 8 it will use either SI or DI which will end up either 32 or 64, and
> when BITS_PER_UNIT is 16 it would use either HI or SI which would
> again be either 32 or 64. Is that wrong and we can end up with a 16
> bit gcov_type?
I see, the code simplified a bit since we dropped support for some of more
exotic
targets. The type should be either 32bit or 64.
>
> The GCOV_TYPE_SIZE was being defined everywhere except when IN_GOV (so
> it was being defined IN_LIBGCOV), but I wanted it defined
> unconditionally because I am using it to determine the required number
> of histogram entries.
>
> In any case, I think it will be more straightforward to define the
> number of histogram entries based on the max possible gcov_type size
> which is 64 (so 256 entries). This will make implementing the bit mask
> simpler, since it will always require the same number of gcov_unsigned
> ints (8).
Sounds good to me. 64bit should be enough for everyone. Coverage is kind of
useless
for smaller types and for really restricted targets we more likely will want to
disable
histogram generation rather than making it a bit smaller.
>
> >
> > Patch is OK if it passed profiledbootstrap modulo the comments above.
>
> Ok, thanks. Working on the fixes above.
OK, thanks!
Honza
>
> Teresa
>
> > Thanks!
> > Honza
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
