[google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[Teresa Johnson]

On Mon, Aug 27, 2012 at 10:55 AM,  <cmang@google.com> wrote:
> On 2012/08/24 23:20:21, davidxl wrote:
>>
>> On Fri, Aug 24, 2012 at 3:56 PM,  <mailto:cmang@google.com> wrote:
>> >
>> > http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h
>> > File gcc/gcov-io.h (right):
>> >
>> >
>
> http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode688
>>
>> > gcc/gcov-io.h:688: gcov_unsigned_t index;   /* The corresponding
>
> string
>>
>> > table index */
>> > On 2012/08/24 22:30:30, davidxl wrote:
>> >>
>> >> Is this field necessary?
>> >
>> >
>> > For the purposes of gcov_dump output we wanted to output not only
>
> the
>>
>> > string table entry but also its index in the string table just in
>
> case
>>
>> > there were any problems with ordering that might map filetags to the
>> > wrong filename. Because of this and the fact that gcov.c and
>
> gcov-dump.c
>>
>> > read one entry at a time, it seemed better to have the index be
>> > self-contained within the struct to avoid extra logic.
>
>
>
>> That is fine -- but you probably need to add assertion check when the
>> string table is read in.
>
>
>> >
>> > Also, since the string table entry has to be written with its
>> > corresponding index for gcov-dump, the gcov_write_string_table_entry
>> > function could have a similar syntax to the ll/brm writing
>
> functions.
>>
>> >
>> >
>> >
>
> http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode689
>>
>> > gcc/gcov-io.h:689: char* filename;          /* The filename that
>
> belongs
>>
>> > at this index */
>> > On 2012/08/24 22:30:30, davidxl wrote:
>> >>
>> >> Can this field name be generalized?
>> >
>> >
>> > Generalized in what way? I used this name because it was used before
>
> in
>>
>> > the old gcda format.
>
>
>> Since it is string table, it can be something like str_ or strval_
>
> etc.
>
>
> Done. filename will be changed to str in an upcoming patch.
>
>
>> >
>> >
>> >
>
> http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c
>>
>> > File libgcc/pmu-profile.c (right):
>> >
>> >
>
>
> http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c#newcode83
>>
>> > libgcc/pmu-profile.c:83:
>> > On 2012/08/24 22:30:30, davidxl wrote:
>> >>
>> >> Who is the caller to this interface?
>> >
>> >
>> >> It should be declared in gcov-io.h
>> >
>> >
>> > The only current caller of this is
>> > https://critique.corp.google.com/#review/31972005. This is also the
>
> same
>>
>> > for the other two ll/brm writing functions. Should I declare those
>
> in
>>
>> > gcov-io.h as well?
>
>
>> Any utilities functions that may be be used by clients other than gcov
>> tool should be in the common header.
>
>
>
> However, I don't really know how to answer this. It seems that if I move
> the mentioned utility functions to gcov-io.h, I should also move their
> helper functions, like convert_unsigned_to_pct, to gcov-io.h as well and
> completely remove pmu-profile.c. Does this seem like a reasonable
> solution?

I think the idea is to put the declarations into gcov-io.h since they
are no longer static. But you could leave the implementation in
pmu-profile.c

Teresa

>
>> David
>
>
>> >
>> > http://codereview.appspot.com/6427063/
>
>
>
>
> http://codereview.appspot.com/6427063/



-- 
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413