Resending in plain text mode so that it goes through to mailing list...
On Wed, Jul 25, 2012 at 11:32 AM, Teresa Johnson <tejohn...@google.com> wrote:
>
>
>
>
> On Tue, Jul 24, 2012 at 3:03 PM, Chris Manghane <cm...@google.com> wrote:
>>
>> This patch modifies pmu-profile to allow gooda_feedback access to
>>
>>
> Needs some more context about perf.data -> gcda conversion.
>
>>
>> important gcov to gcda conversion functions and also
>
>
> Not really gcov to gcda conversion functions, but gcov io functions.
>
>> modifies the gcov pmu format store indices
>
> "format to store indices into a..."
>
>>
>> in a string table of filenames as opposed to storing the filename inside of
>> every pmu entry. This reduces the amount of size bloat in gcda, especially
>> since the gcda files do a per asm line analysis and this leads to many
>> entries sharing the same filename data anyways. gooda_feedback considers
>> this and outputs the string following the load latency and branch
>> misprediction information.
>>
>> The changes made belong to one of the following categories:
>> 1. Removing static declaration/defition of certain function that are
>> needed by gooda_feedback (pmu-profile.c)
>> 2. Modifying the gcov format to use indices instead of string for the
>> filename (gcov-io.h)
>> 3. Modifying gcda writing functions to write the index instead of the
>> filename (pmu-profile.c)
>> 4. Removing references to filename (gcov-io.c, pmu-profile.c)
>>
>> This was tested by using crosstool-validate with --testers=crosstool. No
>> other testcases seemed necessary since this patch only modifies data that is
>> relevant to pfmon, which is considered dead now.
>
>
> Needs more testing (discussed with Chris offline).
>
>>
>>
>> The patch should be applied to google/gcc-4_7
>>
>> The CL for gooda_feedback can be found at Google ref c/31972005
>>
>>
>> 2012-07-24 Chris Manghane <cm...@google.com>
>>
>> * libgcc/pmu-profile.c (static int parse_pfmon_load_latency):
>> filename field no longer exists
>
>
> Also mention the removal of "static" from the declarations.
>
>>
>> (parse_load_latency_line): filename field no longer exists
>> (parse_branch_mispredict_line): filename field no longer exists
>
>
> For repeating descriptions use or "Ditto." or "Likewise."
>
>> (__gcov_stop_pmu_profiler): filename field no longer exists
>> (gcov_write_ll_line): now writes string table index instead of
>> filename
>> (gcov_write_branch_mispredict_line): now writes string table index
>> instead of filename
>> (gcov_write_load_latency_infos): filename field no longer exists
>> (gcov_write_branch_mispredict_infos): filename field no longer exists
>> (gcov_tag_pmu_tool_header_length): filename field no longer exists
>> (gcov_write_tool_header): filename field no longer exists
>> * gcc/gcov.c (release_structures): filename field no longer exists
>> (filter_pmu_data_lines): filename field no longer exists
>> * gcc/gcov-io.c (gcov_read_pmu_load_latency_info): removed filename
>> field and added string table index
>> (gcov_read_pmu_branch_mispredict_info): removed filename field and
>> added string table index
>> (print_load_latency_line): filename field no longer exists
>> (print_branch_mispredict_line): filename field no longer exists
>> * gcc/gcov-io.h: added new tag for string table printing
>> * gcc/gcov-dump.c (tag_pmu_load_latency_info): filename field no
>> longer exists
>> (tag_pmu_branch_mispredict_info): filename field no longer exists
>>
>> Index: libgcc/pmu-profile.c
>> ===================================================================
>> --- libgcc/pmu-profile.c (revision 189823)
>> +++ libgcc/pmu-profile.c (working copy)
>> @@ -232,11 +232,11 @@ static int parse_pfmon_load_latency (char *filenam
>> static int parse_pfmon_branch_mispredicts (char *filename, void *pmu_data);
>> static gcov_unsigned_t gcov_tag_pmu_tool_header_length
>> (gcov_pmu_tool_header_t
>> *header);
>> -static void gcov_write_tool_header (gcov_pmu_tool_header_t *header);
>> -static void gcov_write_load_latency_infos (void *info);
>> -static void gcov_write_branch_mispredict_infos (void *info);
>> -static void gcov_write_ll_line (const gcov_pmu_ll_info_t *ll_info);
>> -static void gcov_write_branch_mispredict_line (const gcov_pmu_brm_info_t
>> +void gcov_write_tool_header (gcov_pmu_tool_header_t *header);
>> +void gcov_write_load_latency_infos (void *info);
>> +void gcov_write_branch_mispredict_infos (void *info);
>> +void gcov_write_ll_line (const gcov_pmu_ll_info_t *ll_info);
>> +void gcov_write_branch_mispredict_line (const gcov_pmu_brm_info_t
>> *brm_info);
>
>
> They should be moved to gcov-io.h with linkage GCOV_LINKAGE which will pick
> extern when building libgcov (see examples in gcov-io.h).
>
>>
>> static int start_addr2line_symbolizer (pid_t pid);
>> static void end_addr2line_symbolizer (void);
>> @@ -749,14 +749,12 @@ parse_load_latency_line (char *line, gcov_pmu_ll_i
>
>
> I think it would be cleaner just to remove all the pfmon specific handling as
> part of this checkin, since that is obsolete and wouldn't work with your
> changes anyway.
>
>>
>> if (!sep)
>> {
>> /* Assume entire string is srcfile. */
>> - ll_info->filename = (char *)sym_info;
>> ll_info->line = 0;
>> }
>> else
>> {
>> /* Terminate the filename string at the separator. */
>> *sep = 0;
>> - ll_info->filename = (char *)sym_info;
>> /* Convert rest of the sym info to a line number. */
>> ll_info->line = atol (sep+1);
>> }
>> @@ -765,7 +763,6 @@ parse_load_latency_line (char *line, gcov_pmu_ll_i
>> else
>> {
>> /* No symbolizer available. */
>> - ll_info->filename = NULL;
>> ll_info->line = 0;
>> ll_info->discriminator = 0;
>> }
>> @@ -815,14 +812,12 @@ parse_branch_mispredict_line (char *line, gcov_pmu
>> if (!sep)
>> {
>> /* Assume entire string is srcfile. */
>> - brm_info->filename = sym_info;
>> brm_info->line = 0;
>> }
>> else
>> {
>> /* Terminate the filename string at the separator. */
>> *sep = 0;
>> - brm_info->filename = sym_info;
>> /* Convert rest of the sym info to a line number. */
>> brm_info->line = atol (sep+1);
>> }
>> @@ -831,7 +826,6 @@ parse_branch_mispredict_line (char *line, gcov_pmu
>> else
>> {
>> /* No symbolizer available. */
>> - brm_info->filename = NULL;
>> brm_info->line = 0;
>> brm_info->discriminator = 0;
>> }
>> @@ -1361,11 +1355,10 @@ __gcov_stop_pmu_profiler (void)
>>
>> /* Write the load latency information LL_INFO into the gcda file. */
>>
>> -static void
>> +void
>> gcov_write_ll_line (const gcov_pmu_ll_info_t *ll_info)
>> {
>> - gcov_unsigned_t len = GCOV_TAG_PMU_LOAD_LATENCY_LENGTH
>> (ll_info->filename);
>> - gcov_write_tag_length (GCOV_TAG_PMU_LOAD_LATENCY_INFO, len);
>> + gcov_write_tag_length (GCOV_TAG_PMU_LOAD_LATENCY_INFO, 1);
>
>
> The length of 1 isn't correct - it should be the length of the data
> associated with this tag. See GCOV_TAG_PMU_LOAD_LATENCY_LENGTH for how it is
> computing this (and you can just modify that to remove the filename parameter
> and add 1 for your new tag).
>
>> gcov_write_unsigned (ll_info->counts);
>> gcov_write_unsigned (ll_info->self);
>> gcov_write_unsigned (ll_info->cum);
>> @@ -1379,31 +1372,29 @@ gcov_write_ll_line (const gcov_pmu_ll_info_t *ll_i
>> gcov_write_counter (ll_info->code_addr);
>> gcov_write_unsigned (ll_info->line);
>> gcov_write_unsigned (ll_info->discriminator);
>> - gcov_write_string (ll_info->filename);
>> + gcov_write_unsigned (ll_info->filetag);
>> }
>>
>>
>> /* Write the branch mispredict information BRM_INFO into the gcda file. */
>>
>> -static void
>> +void
>> gcov_write_branch_mispredict_line (const gcov_pmu_brm_info_t *brm_info)
>> {
>> - gcov_unsigned_t len = GCOV_TAG_PMU_BRANCH_MISPREDICT_LENGTH (
>> - brm_info->filename);
>> - gcov_write_tag_length (GCOV_TAG_PMU_BRANCH_MISPREDICT_INFO, len);
>> + gcov_write_tag_length (GCOV_TAG_PMU_BRANCH_MISPREDICT_INFO, 1);
>
>
> Same thing here with the length computation (see above comment).
>
>>
>> gcov_write_unsigned (brm_info->counts);
>> gcov_write_unsigned (brm_info->self);
>> gcov_write_unsigned (brm_info->cum);
>> gcov_write_counter (brm_info->code_addr);
>> gcov_write_unsigned (brm_info->line);
>> gcov_write_unsigned (brm_info->discriminator);
>> - gcov_write_string (brm_info->filename);
>> + gcov_write_unsigned (brm_info->filetag);
>> }
>>
>> /* Write load latency information INFO into the gcda file. The gcda
>> file has already been opened and is available for writing. */
>>
>> -static void
>> +void
>> gcov_write_load_latency_infos (void *info)
>> {
>> unsigned i;
>> @@ -1429,7 +1420,7 @@ gcov_write_load_latency_infos (void *info)
>> /* Write branch mispredict information INFO into the gcda file. The
>> gcda file has already been opened and is available for writing. */
>>
>> -static void
>> +void
>> gcov_write_branch_mispredict_infos (void *info)
>> {
>> unsigned i;
>> @@ -1470,7 +1461,7 @@ gcov_tag_pmu_tool_header_length (gcov_pmu_tool_hea
>> /* Write tool header into the gcda file. It assumes that the gcda file
>> has already been opened and is available for writing. */
>>
>> -static void
>> +void
>> gcov_write_tool_header (gcov_pmu_tool_header_t *header)
>> {
>> gcov_unsigned_t len = gcov_tag_pmu_tool_header_length (header);
>> Index: gcc/gcov.c
>> ===================================================================
>> --- gcc/gcov.c (revision 189823)
>> +++ gcc/gcov.c (working copy)
>> @@ -1008,8 +1008,6 @@ release_structures (void)
>> /* delete each element */
>> for (i = 0; i < ll_infos->ll_count; ++i)
>> {
>> - if (ll_infos->ll_array[i]->filename)
>> - XDELETE (ll_infos->ll_array[i]->filename);
>> XDELETE (ll_infos->ll_array[i]);
>> }
>> /* delete the array itself */
>> @@ -1026,8 +1024,6 @@ release_structures (void)
>> /* delete each element */
>> for (i = 0; i < brm_infos->brm_count; ++i)
>> {
>> - if (brm_infos->brm_array[i]->filename)
>> - XDELETE (brm_infos->brm_array[i]->filename);
>> XDELETE (brm_infos->brm_array[i]);
>> }
>> /* delete the array itself */
>> @@ -2277,58 +2273,6 @@ filter_pmu_data_lines (source_t *src)
>> ll_infos->ll_array = 0;
>> brm_infos->brm_array = 0;
>>
>> - /* Go over all the load latency entries and save the ones
>> - corresponding to this source file. */
>
>
> Don't want to remove this handling, as it is doing the job of deciding which
> pmu data matches the current file being compiled. You just need to change the
> handling of how to access the filename.
>
>>
>> - for (i = 0; i < pmu_global_info.ll_infos.ll_count; ++i)
>> - {
>> - gcov_pmu_ll_info_t *ll_info = pmu_global_info.ll_infos.ll_array[i];
>> - if (0 == strcmp (src->name, ll_info->filename))
>> - {
>> - if (!ll_infos->ll_array)
>> - {
>> - ll_infos->ll_count = 0;
>> - ll_infos->alloc_ll_count = 64;
>> - ll_infos->ll_array = XCNEWVEC (gcov_pmu_ll_info_t *,
>> - ll_infos->alloc_ll_count);
>> - }
>> - /* Found a matching entry, save it. */
>> - ll_infos->ll_count++;
>> - if (ll_infos->ll_count >= ll_infos->alloc_ll_count)
>> - {
>> - /* need to realloc */
>> - ll_infos->ll_array = (gcov_pmu_ll_info_t **)
>> - xrealloc (ll_infos->ll_array, 2 * ll_infos->alloc_ll_count);
>> - }
>> - ll_infos->ll_array[ll_infos->ll_count - 1] = ll_info;
>> - }
>> - }
>> -
>> - /* Go over all the branch mispredict entries and save the ones
>> - corresponding to this source file. */
>> - for (i = 0; i < pmu_global_info.brm_infos.brm_count; ++i)
>> - {
>> - gcov_pmu_brm_info_t *brm_info =
>> pmu_global_info.brm_infos.brm_array[i];
>> - if (0 == strcmp (src->name, brm_info->filename))
>> - {
>> - if (!brm_infos->brm_array)
>> - {
>> - brm_infos->brm_count = 0;
>> - brm_infos->alloc_brm_count = 64;
>> - brm_infos->brm_array = XCNEWVEC (gcov_pmu_brm_info_t *,
>> - brm_infos->alloc_brm_count);
>> - }
>> - /* Found a matching entry, save it. */
>> - brm_infos->brm_count++;
>> - if (brm_infos->brm_count >= brm_infos->alloc_brm_count)
>> - {
>> - /* need to realloc */
>> - brm_infos->brm_array = (gcov_pmu_brm_info_t **)
>> - xrealloc (brm_infos->brm_array, 2 *
>> brm_infos->alloc_brm_count);
>> - }
>> - brm_infos->brm_array[brm_infos->brm_count - 1] = brm_info;
>> - }
>> - }
>> -
>> /* Sort the load latency data according to the line numbers because
>> we later iterate over sources in line number order. Normally we
>> expect the PMU tool to provide sorted data, but a few entries can
>> Index: gcc/gcov-io.c
>> ===================================================================
>> --- gcc/gcov-io.c (revision 189823)
>> +++ gcc/gcov-io.c (working copy)
>> @@ -242,7 +242,6 @@ GCOV_LINKAGE void
>> gcov_read_pmu_load_latency_info (gcov_pmu_ll_info_t *ll_info,
>> gcov_unsigned_t len ATTRIBUTE_UNUSED)
>> {
>> - const char *filename;
>> ll_info->counts = gcov_read_unsigned ();
>> ll_info->self = gcov_read_unsigned ();
>> ll_info->cum = gcov_read_unsigned ();
>> @@ -256,11 +255,7 @@ gcov_read_pmu_load_latency_info (gcov_pmu_ll_info_
>> ll_info->code_addr = gcov_read_counter ();
>> ll_info->line = gcov_read_unsigned ();
>> ll_info->discriminator = gcov_read_unsigned ();
>> - filename = gcov_read_string ();
>> - if (filename)
>> - ll_info->filename = gcov_canonical_filename (xstrdup (filename));
>> - else
>> - ll_info->filename = 0;
>> + ll_info->filetag = gcov_read_unsigned ();
>> }
>>
>> /* Read LEN words and construct branch mispredict info BRM_INFO. */
>> @@ -269,18 +264,13 @@ GCOV_LINKAGE void
>> gcov_read_pmu_branch_mispredict_info (gcov_pmu_brm_info_t *brm_info,
>> gcov_unsigned_t len ATTRIBUTE_UNUSED)
>> {
>> - const char *filename;
>> brm_info->counts = gcov_read_unsigned ();
>> brm_info->self = gcov_read_unsigned ();
>> brm_info->cum = gcov_read_unsigned ();
>> brm_info->code_addr = gcov_read_counter ();
>> brm_info->line = gcov_read_unsigned ();
>> brm_info->discriminator = gcov_read_unsigned ();
>> - filename = gcov_read_string ();
>> - if (filename)
>> - brm_info->filename = gcov_canonical_filename (xstrdup (filename));
>> - else
>> - brm_info->filename = 0;
>> + brm_info->filetag = gcov_read_unsigned ();
>> }
>>
>> /* Read LEN words from an open gcov file and construct data into pmu
>> @@ -779,7 +769,7 @@ print_load_latency_line (FILE *fp, const gcov_pmu_
>> if (!ll_info)
>> return;
>> fprintf (fp, " %u %.2f%% %.2f%% %.2f%% %.2f%% %.2f%% %.2f%% %.2f%% "
>> - "%.2f%% %.2f%% " HOST_WIDEST_INT_PRINT_HEX " %s %d %d",
>> + "%.2f%% %.2f%% " HOST_WIDEST_INT_PRINT_HEX " %d %d %d",
>
>
> Instead of changing this to print the tag, it would be useful to print both
> the tag and the filename (as accessed via your string table). Ditto below for
> the branch mispredict info.
>
>>
>> ll_info->counts,
>> convert_unsigned_to_pct (ll_info->self),
>> convert_unsigned_to_pct (ll_info->cum),
>> @@ -791,7 +781,7 @@ print_load_latency_line (FILE *fp, const gcov_pmu_
>> convert_unsigned_to_pct (ll_info->gt_1024),
>> convert_unsigned_to_pct (ll_info->wself),
>> ll_info->code_addr,
>> - ll_info->filename,
>> + ll_info->filetag,
>> ll_info->line,
>> ll_info->discriminator);
>> if (newline == add_newline)
>> @@ -807,12 +797,12 @@ print_branch_mispredict_line (FILE *fp, const gcov
>> {
>> if (!brm_info)
>> return;
>> - fprintf (fp, " %u %.2f%% %.2f%% " HOST_WIDEST_INT_PRINT_HEX " %s %d %d",
>> + fprintf (fp, " %u %.2f%% %.2f%% " HOST_WIDEST_INT_PRINT_HEX " %d %d %d",
>> brm_info->counts,
>> convert_unsigned_to_pct (brm_info->self),
>> convert_unsigned_to_pct (brm_info->cum),
>> brm_info->code_addr,
>> - brm_info->filename,
>> + brm_info->filetag,
>> brm_info->line,
>> brm_info->discriminator);
>> if (newline == add_newline)
>
>
> Need to add i/o handling of new string table.
>
>> Index: gcc/gcov-io.h
>> ===================================================================
>> --- gcc/gcov-io.h (revision 189823)
>> +++ gcc/gcov-io.h (working copy)
>> @@ -449,6 +449,7 @@ typedef HOST_WIDEST_INT gcov_type;
>> (gcov_string_length (filename) + 5 + 2)
>> #define GCOV_TAG_PMU_TOOL_HEADER ((gcov_unsigned_t)0xa9000000)
>> #define GCOV_TAG_MODULE_INFO ((gcov_unsigned_t)0xab000000)
>> +#define GCOV_TAG_PMU_STRING_TABLE_ENTRY ((gcov_unsigned_t)0xad000000)
>>
>> /* Counters that are collected. */
>> #define GCOV_COUNTER_ARCS 0 /* Arc transitions. */
>> @@ -635,7 +636,7 @@ typedef struct gcov_pmu_load_latency_info
>> gcov_type code_addr; /* the actual miss address (pc+1 for Intel) */
>> gcov_unsigned_t line; /* line number corresponding to this miss */
>> gcov_unsigned_t discriminator; /* discriminator information for this
>> miss */
>> - char *filename; /* filename corresponding to this miss */
>> + gcov_unsigned_t filetag; /* location in string table of filename
>> corresponding to event */
>> } gcov_pmu_ll_info_t;
>>
>> /* This structure is used during runtime as well as in gcov. */
>> @@ -665,7 +666,7 @@ typedef struct gcov_pmu_branch_mispredict_info
>> gcov_type code_addr; /* the actual mispredict address */
>> gcov_unsigned_t line; /* line number corresponding to this event */
>> gcov_unsigned_t discriminator; /* discriminator for this event */
>> - char *filename; /* filename corresponding to this event */
>> + gcov_unsigned_t filetag; /* location in string table of filename
>> corresponding to event */
>> } gcov_pmu_brm_info_t;
>>
>> /* This structure is used during runtime as well as in gcov. */
>> Index: gcc/gcov-dump.c
>> ===================================================================
>> --- gcc/gcov-dump.c (revision 189823)
>> +++ gcc/gcov-dump.c (working copy)
>> @@ -573,7 +573,6 @@ tag_pmu_load_latency_info (const char *filename AT
>>
>> gcov_pmu_ll_info_t ll_info;
>> gcov_read_pmu_load_latency_info (&ll_info, length);
>> print_load_latency_line (stdout, &ll_info, no_newline);
>> - free (ll_info.filename);
>> }
>>
>> /* Read gcov tag GCOV_TAG_PMU_BRANCH_MISPREDICT_INFO from the gcda
>> @@ -586,7 +585,6 @@ tag_pmu_branch_mispredict_info (const char *filena
>> gcov_pmu_brm_info_t brm_info;
>> gcov_read_pmu_branch_mispredict_info (&brm_info, length);
>> print_branch_mispredict_line (stdout, &brm_info, no_newline);
>> - free (brm_info.filename);
>> }
>
>
> Need to add printing of new string table.
>
> Teresa
>>
>>
>>
>>
>> --
>> This patch is available for review at http://codereview.appspot.com/6427063
>
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
>
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
