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

[cmang at google dot com]

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?

> David

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



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