* FDO usability patch -- cfg and lineno checksum
@ 2011-04-13 22:26 Xinliang David Li
2011-04-13 22:34 ` Xinliang David Li
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Xinliang David Li @ 2011-04-13 22:26 UTC (permalink / raw)
To: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]
Hi, in current FDO implementation, the source file version used in
profile-generate needs to strictly match the version used in
profile-use -- simple formating changes will invalidate the profile
data (use of it will lead to compiler error or ICE). There are two
main problems that lead to the weakness:
1) the function checksum is computed based on line number and number
of basic blocks -- this means any line number change will invalidate
the profile data. In fact, line number should play very minimal role
in profile matching decision. Another problem is that number of basic
blocks is also not a good indicator whether CFG matches or not.
2) cgraph's pid is used as the key to find the matching profile data
for a function. If there is any new function added, or if the order of
the functions changes, the profile data is invalidated.
The attached is a patch from google that improves this.
1) function checksum is split into two parts: lineno checksum and cfg checksum
2) function assembler name is used in profile hashing.
Bootstrapped and regression tested on x86_64/linux.
Ok for trunk?
Thanks,
David
2011-04-13 Xinliang David Li <davidxl@google.com>
* tree.c (crc32_byte): New function.
* tree.h (crc32_byte): New function.
* gcov.c (function_info): Add new fields.
(read_graph_file): Handle new fields.
(read_count_file): Handle name.
* gcov-io.c (gcov_string_length): New function.
(gcov_write_words): Bug fix.
(gcov_read_words): Bug fix.
* gcov-io.h: New interfaces.
* profile.c (get_exec_counts): New parameter.
(compute_branch_probablilities): New parameter.
(compute_value_histogram): New parameter.
(branch_prob): Pass cfg_checksum.
* coverage.c (get_const_string_type): New function.
(hash_counts_entry_hash): Use string hash.
(hash_counts_entry_eq): Use string compare.
(htab_counts_entry_del): Delete name.
(read_count_file): Add handling of cfg checksum.
(get_coverage_counts): New parameter.
(xstrdup_mask_random): New function.
(coverage_compute_lineno_checksum): New function.
(coverage_compute_cfg_checksum): New function.
(coverage_begin_output): New parameter.
(coverage_end_function): New parameter.
(build_fn_info_type): Build new fields.
(build_fn_info_value): Build new field values.
* coverage.h: Interface changes.
* libgcov.c (gcov_exit): Dump new fields.
* gcov_dump.c (tag_function): Print new fields.
[-- Attachment #2: cfg_chksum.p --]
[-- Type: text/x-pascal, Size: 39966 bytes --]
Index: tree.c
===================================================================
--- tree.c (revision 172385)
+++ tree.c (working copy)
@@ -8508,14 +8508,12 @@ dump_tree_statistics (void)
\f
#define FILE_FUNCTION_FORMAT "_GLOBAL__%s_%s"
-/* Generate a crc32 of a string. */
+/* Generate a crc32 of a byte. */
unsigned
-crc32_string (unsigned chksum, const char *string)
+crc32_byte (unsigned chksum, char byte)
{
- do
- {
- unsigned value = *string << 24;
+ unsigned value = (unsigned) byte << 24;
unsigned ix;
for (ix = 8; ix--; value <<= 1)
@@ -8526,6 +8524,18 @@ crc32_string (unsigned chksum, const cha
chksum <<= 1;
chksum ^= feedback;
}
+ return chksum;
+}
+
+
+/* Generate a crc32 of a string. */
+
+unsigned
+crc32_string (unsigned chksum, const char *string)
+{
+ do
+ {
+ chksum = crc32_byte (chksum, *string);
}
while (*string++);
return chksum;
@@ -8549,8 +8559,10 @@ clean_symbol_name (char *p)
*p = '_';
}
-/* Generate a name for a special-purpose function function.
+/* Generate a name for a special-purpose function.
The generated name may need to be unique across the whole link.
+ Changes to this function may also require corresponding changes to
+ xstrdup_mask_random.
TYPE is some string to identify the purpose of this function to the
linker or collect2; it must start with an uppercase letter,
one of:
Index: tree.h
===================================================================
--- tree.h (revision 172385)
+++ tree.h (working copy)
@@ -4997,6 +4997,7 @@ inlined_function_outer_scope_p (const_tr
\f
/* In tree.c */
extern unsigned crc32_string (unsigned, const char *);
+extern unsigned crc32_byte (unsigned, char);
extern void clean_symbol_name (char *);
extern tree get_file_function_name (const char *);
extern tree get_callee_fndecl (const_tree);
Index: gcov.c
===================================================================
--- gcov.c (revision 172385)
+++ gcov.c (working copy)
@@ -54,6 +54,13 @@ along with Gcov; see the file COPYING3.
some places we make use of the knowledge of how profile.c works to
select particular algorithms here. */
+/* The code validates that the profile information read in corresponds
+ to the code currently being compiled. Rather than checking for
+ identical files, the code below computes a checksum on the CFG
+ (based on the order of basic blocks and the arcs in the CFG). If
+ the CFG checksum in the gcda file match the CFG checksum for the
+ code currently being compiled, the profile data will be used. */
+
/* This is the size of the buffer used to read in source file lines. */
#define STRING_SIZE 200
@@ -161,7 +168,8 @@ typedef struct function_info
/* Name of function. */
char *name;
unsigned ident;
- unsigned checksum;
+ unsigned lineno_checksum;
+ unsigned cfg_checksum;
/* Array of basic blocks. */
block_t *blocks;
@@ -809,12 +817,14 @@ read_graph_file (void)
if (tag == GCOV_TAG_FUNCTION)
{
char *function_name;
- unsigned ident, checksum, lineno;
+ unsigned ident, lineno;
+ unsigned lineno_checksum, cfg_checksum;
source_t *src;
function_t *probe, *prev;
ident = gcov_read_unsigned ();
- checksum = gcov_read_unsigned ();
+ lineno_checksum = gcov_read_unsigned ();
+ cfg_checksum = gcov_read_unsigned ();
function_name = xstrdup (gcov_read_string ());
src = find_source (gcov_read_string ());
lineno = gcov_read_unsigned ();
@@ -822,7 +832,8 @@ read_graph_file (void)
fn = XCNEW (function_t);
fn->name = function_name;
fn->ident = ident;
- fn->checksum = checksum;
+ fn->lineno_checksum = lineno_checksum;
+ fn->cfg_checksum = cfg_checksum;
fn->src = src;
fn->line = lineno;
@@ -1039,6 +1050,7 @@ read_count_file (void)
unsigned tag;
function_t *fn = NULL;
int error = 0;
+ const char *fn_name;
if (!gcov_open (da_file_name, 1))
{
@@ -1109,13 +1121,20 @@ read_count_file (void)
if (!fn)
;
- else if (gcov_read_unsigned () != fn->checksum)
+ else if (gcov_read_unsigned () != fn->lineno_checksum
+ || gcov_read_unsigned () != fn->cfg_checksum)
{
mismatch:;
fnotice (stderr, "%s:profile mismatch for '%s'\n",
da_file_name, fn->name);
goto cleanup;
}
+
+ fn_name = gcov_read_string ();
+ if (strcmp (fn_name, fn->name) != 0)
+ fnotice (stderr, "%s:profile mismatch:"
+ " expected function name '%s' but found '%s'\n",
+ da_file_name, fn->name, fn_name);
}
else if (tag == GCOV_TAG_FOR_COUNTER (GCOV_COUNTER_ARCS) && fn)
{
Index: testsuite/gcc.dg/tree-prof/prof-robust-1.c
===================================================================
--- testsuite/gcc.dg/tree-prof/prof-robust-1.c (revision 0)
+++ testsuite/gcc.dg/tree-prof/prof-robust-1.c (revision 0)
@@ -0,0 +1,21 @@
+/* { dg-options "-O2 -w" } */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#ifdef _PROFILE_USE
+int foo(int x) {
+ return 3 * x;
+}
+#endif
+
+int x = 1000;
+
+int main(int argc, char *argv[]) {
+ int i;
+ int sum = 0;
+ for (i = 0; i < x; i++)
+ sum += i;
+ printf("%d\n", sum);
+ return 0;
+}
Index: testsuite/g++.dg/prof-robust-1.C
===================================================================
--- testsuite/g++.dg/prof-robust-1.C (revision 0)
+++ testsuite/g++.dg/prof-robust-1.C (revision 0)
@@ -0,0 +1,24 @@
+/* { dg-options "-O2 -fno-weak" } */
+
+#include <stdio.h>
+
+namespace {
+ namespace {
+
+ class MyClass {
+ public:
+ void foo() const;
+ ~MyClass() { foo(); }
+ };
+
+ void MyClass::foo() const { printf("Goodbye World\n"); }
+
+ }
+
+ static MyClass variable;
+
+}
+
+int main() {
+ return 0;
+}
Index: gcov-io.c
===================================================================
--- gcov-io.c (revision 172385)
+++ gcov-io.c (working copy)
@@ -28,6 +28,12 @@ see the files COPYING3 and COPYING.RUNTI
/* Routines declared in gcov-io.h. This file should be #included by
another source file, after having #included gcov-io.h. */
+/* Redefine these here, rather than using the ones in system.h since
+ * including system.h leads to conflicting definitions of other
+ * symbols and macros. */
+#undef MIN
+#define MIN(X,Y) ((X) < (Y) ? (X) : (Y))
+
#if !IN_GCOV
static void gcov_write_block (unsigned);
static gcov_unsigned_t *gcov_write_words (unsigned);
@@ -228,6 +234,27 @@ gcov_write_block (unsigned size)
gcov_var.offset -= size;
}
+#if IN_LIBGCOV
+
+/* These functions are guarded by #if to avoid compile time warning. */
+
+/* Return the number of words STRING would need including the length
+ field in the output stream itself. This should be identical to
+ "alloc" calculation in gcov_write_string(). */
+
+GCOV_LINKAGE gcov_unsigned_t
+gcov_string_length (const char *string)
+{
+ gcov_unsigned_t len = (string) ? strlen (string) : 0;
+ /* + 1 because of the length field. */
+ gcov_unsigned_t alloc = 1 + ((len + 4) >> 2);
+
+ /* Can not write a bigger than GCOV_BLOCK_SIZE string yet */
+ gcc_assert (alloc < GCOV_BLOCK_SIZE);
+ return alloc;
+}
+#endif
+
/* Allocate space to write BYTES bytes to the gcov file. Return a
pointer to those bytes, or NULL on failure. */
@@ -238,13 +265,15 @@ gcov_write_words (unsigned words)
gcc_assert (gcov_var.mode < 0);
#if IN_LIBGCOV
- if (gcov_var.offset >= GCOV_BLOCK_SIZE)
+ if (gcov_var.offset + words >= GCOV_BLOCK_SIZE)
{
- gcov_write_block (GCOV_BLOCK_SIZE);
+ gcov_write_block (MIN (gcov_var.offset, GCOV_BLOCK_SIZE));
if (gcov_var.offset)
{
- gcc_assert (gcov_var.offset == 1);
- memcpy (gcov_var.buffer, gcov_var.buffer + GCOV_BLOCK_SIZE, 4);
+ gcc_assert (gcov_var.offset < GCOV_BLOCK_SIZE);
+ memcpy (gcov_var.buffer,
+ gcov_var.buffer + GCOV_BLOCK_SIZE,
+ gcov_var.offset << 2);
}
}
#else
@@ -285,7 +314,6 @@ gcov_write_counter (gcov_type value)
}
#endif /* IN_LIBGCOV */
-#if !IN_LIBGCOV
/* Write STRING to coverage file. Sets error flag on file
error, overflow flag on overflow */
@@ -308,7 +336,7 @@ gcov_write_string (const char *string)
buffer[alloc] = 0;
memcpy (&buffer[1], string, length);
}
-#endif
+
#if !IN_LIBGCOV
/* Write a tag TAG and reserve space for the record length. Return a
@@ -396,14 +424,15 @@ gcov_read_words (unsigned words)
unsigned excess = gcov_var.length - gcov_var.offset;
gcc_assert (gcov_var.mode > 0);
+ gcc_assert (words < GCOV_BLOCK_SIZE);
if (excess < words)
{
gcov_var.start += gcov_var.offset;
#if IN_LIBGCOV
if (excess)
{
- gcc_assert (excess == 1);
- memcpy (gcov_var.buffer, gcov_var.buffer + gcov_var.offset, 4);
+ gcc_assert (excess < GCOV_BLOCK_SIZE);
+ memmove (gcov_var.buffer, gcov_var.buffer + gcov_var.offset, excess * 4);
}
#else
memmove (gcov_var.buffer, gcov_var.buffer + gcov_var.offset, excess * 4);
@@ -411,8 +440,7 @@ gcov_read_words (unsigned words)
gcov_var.offset = 0;
gcov_var.length = excess;
#if IN_LIBGCOV
- gcc_assert (!gcov_var.length || gcov_var.length == 1);
- excess = GCOV_BLOCK_SIZE;
+ excess = (sizeof (gcov_var.buffer) / sizeof (gcov_var.buffer[0])) - gcov_var.length;
#else
if (gcov_var.length + words > gcov_var.alloc)
gcov_allocate (gcov_var.length + words);
@@ -472,7 +500,6 @@ gcov_read_counter (void)
buffer, or NULL on empty string. You must copy the string before
calling another gcov function. */
-#if !IN_LIBGCOV
GCOV_LINKAGE const char *
gcov_read_string (void)
{
@@ -483,7 +510,7 @@ gcov_read_string (void)
return (const char *) gcov_read_words (length);
}
-#endif
+
GCOV_LINKAGE void
gcov_read_summary (struct gcov_summary *summary)
Index: gcov-io.h
===================================================================
--- gcov-io.h (revision 172385)
+++ gcov-io.h (working copy)
@@ -101,9 +101,10 @@ see the files COPYING3 and COPYING.RUNTI
The basic block graph file contains the following records
note: unit function-graph*
- unit: header int32:checksum string:source
+ unit: header int32:checksum int32: string:source
function-graph: announce_function basic_blocks {arcs | lines}*
- announce_function: header int32:ident int32:checksum
+ announce_function: header int32:ident
+ int32:lineno_checksum int32:cfg_checksum
string:name string:source int32:lineno
basic_block: header int32:flags*
arcs: header int32:block_no arc*
@@ -132,7 +133,9 @@ see the files COPYING3 and COPYING.RUNTI
data: {unit function-data* summary:object summary:program*}*
unit: header int32:checksum
function-data: announce_function arc_counts
- announce_function: header int32:ident int32:checksum
+ announce_function: header int32:ident
+ int32:lineno_checksum int32:cfg_checksum
+ string:name
arc_counts: header int64:count*
summary: int32:checksum {count-summary}GCOV_COUNTERS
count-summary: int32:num int32:runs int64:sum
@@ -243,13 +246,16 @@ typedef HOST_WIDEST_INT gcov_type;
#define gcov_write_unsigned __gcov_write_unsigned
#define gcov_write_counter __gcov_write_counter
#define gcov_write_summary __gcov_write_summary
+#define gcov_write_string __gcov_write_string
+#define gcov_string_length __gcov_string_length
#define gcov_read_unsigned __gcov_read_unsigned
#define gcov_read_counter __gcov_read_counter
+#define gcov_read_string __gcov_read_string
#define gcov_read_summary __gcov_read_summary
/* Poison these, so they don't accidentally slip in. */
-#pragma GCC poison gcov_write_string gcov_write_tag gcov_write_length
-#pragma GCC poison gcov_read_string gcov_sync gcov_time gcov_magic
+#pragma GCC poison gcov_write_tag gcov_write_length
+#pragma GCC poison gcov_sync gcov_time gcov_magic
#ifdef HAVE_GAS_HIDDEN
#define ATTRIBUTE_HIDDEN __attribute__ ((__visibility__ ("hidden")))
@@ -294,7 +300,7 @@ typedef HOST_WIDEST_INT gcov_type;
file marker -- it is not required to be present. */
#define GCOV_TAG_FUNCTION ((gcov_unsigned_t)0x01000000)
-#define GCOV_TAG_FUNCTION_LENGTH (2)
+#define GCOV_TAG_FUNCTION_LENGTH (3)
#define GCOV_TAG_BLOCKS ((gcov_unsigned_t)0x01410000)
#define GCOV_TAG_BLOCKS_LENGTH(NUM) (NUM)
#define GCOV_TAG_BLOCKS_NUM(LENGTH) (LENGTH)
@@ -411,11 +417,20 @@ struct gcov_summary
/* Information about a single function. This uses the trailing array
idiom. The number of counters is determined from the counter_mask
in gcov_info. We hold an array of function info, so have to
- explicitly calculate the correct array stride. */
+ explicitly calculate the correct array stride.
+
+ "ident" is no longer used in hash computation. Additionally,
+ "name" is used in hash computation. This makes the profile data
+ not compatible across function name changes.
+ Also, the function pointer is stored for later use for indirect
+ function call name resolution.
+ */
struct gcov_fn_info
{
gcov_unsigned_t ident; /* unique ident of function */
- gcov_unsigned_t checksum; /* function checksum */
+ gcov_unsigned_t lineno_checksum; /* function lineo_checksum */
+ gcov_unsigned_t cfg_checksum; /* function cfg checksum */
+ const char *name; /* name of the function */
unsigned n_ctrs[0]; /* instrumented counters */
};
@@ -543,6 +558,7 @@ static int gcov_is_error (void);
GCOV_LINKAGE gcov_unsigned_t gcov_read_unsigned (void) ATTRIBUTE_HIDDEN;
GCOV_LINKAGE gcov_type gcov_read_counter (void) ATTRIBUTE_HIDDEN;
GCOV_LINKAGE void gcov_read_summary (struct gcov_summary *) ATTRIBUTE_HIDDEN;
+GCOV_LINKAGE const char *gcov_read_string (void) ATTRIBUTE_HIDDEN;
#if IN_LIBGCOV
/* Available only in libgcov */
@@ -554,9 +570,9 @@ GCOV_LINKAGE void gcov_write_summary (gc
ATTRIBUTE_HIDDEN;
static void gcov_rewrite (void);
GCOV_LINKAGE void gcov_seek (gcov_position_t /*position*/) ATTRIBUTE_HIDDEN;
+GCOV_LINKAGE gcov_unsigned_t gcov_string_length (const char *) ATTRIBUTE_HIDDEN;
#else
/* Available outside libgcov */
-GCOV_LINKAGE const char *gcov_read_string (void);
GCOV_LINKAGE void gcov_sync (gcov_position_t /*base*/,
gcov_unsigned_t /*length */);
#endif
@@ -564,11 +580,11 @@ GCOV_LINKAGE void gcov_sync (gcov_positi
#if !IN_GCOV
/* Available outside gcov */
GCOV_LINKAGE void gcov_write_unsigned (gcov_unsigned_t) ATTRIBUTE_HIDDEN;
+GCOV_LINKAGE void gcov_write_string (const char *) ATTRIBUTE_HIDDEN;
#endif
#if !IN_GCOV && !IN_LIBGCOV
/* Available only in compiler */
-GCOV_LINKAGE void gcov_write_string (const char *);
GCOV_LINKAGE gcov_position_t gcov_write_tag (gcov_unsigned_t);
GCOV_LINKAGE void gcov_write_length (gcov_position_t /*position*/);
#endif
Index: profile.c
===================================================================
--- profile.c (revision 172385)
+++ profile.c (working copy)
@@ -102,13 +102,6 @@ static int total_num_branches;
/* Forward declarations. */
static void find_spanning_tree (struct edge_list *);
-static unsigned instrument_edges (struct edge_list *);
-static void instrument_values (histogram_values);
-static void compute_branch_probabilities (void);
-static void compute_value_histograms (histogram_values);
-static gcov_type * get_exec_counts (void);
-static basic_block find_group (basic_block);
-static void union_groups (basic_block, basic_block);
/* Add edge instrumentation code to the entire insn chain.
@@ -233,10 +226,12 @@ instrument_values (histogram_values valu
}
\f
-/* Computes hybrid profile for all matching entries in da_file. */
+/* Computes hybrid profile for all matching entries in da_file.
+
+ CFG_CHECKSUM is the precomputed checksum for the CFG. */
static gcov_type *
-get_exec_counts (void)
+get_exec_counts (unsigned cfg_checksum)
{
unsigned num_edges = 0;
basic_block bb;
@@ -253,7 +248,8 @@ get_exec_counts (void)
num_edges++;
}
- counts = get_coverage_counts (GCOV_COUNTER_ARCS, num_edges, &profile_info);
+ counts = get_coverage_counts (GCOV_COUNTER_ARCS, num_edges, cfg_checksum,
+ &profile_info);
if (!counts)
return NULL;
@@ -442,10 +438,12 @@ read_profile_edge_counts (gcov_type *exe
}
/* Compute the branch probabilities for the various branches.
- Annotate them accordingly. */
+ Annotate them accordingly.
+
+ CFG_CHECKSUM is the precomputed checksum for the CFG. */
static void
-compute_branch_probabilities (void)
+compute_branch_probabilities (unsigned cfg_checksum)
{
basic_block bb;
int i;
@@ -454,7 +452,7 @@ compute_branch_probabilities (void)
int passes;
int hist_br_prob[20];
int num_branches;
- gcov_type *exec_counts = get_exec_counts ();
+ gcov_type *exec_counts = get_exec_counts (cfg_checksum);
int inconsistent = 0;
/* Very simple sanity checks so we catch bugs in our profiling code. */
@@ -772,10 +770,12 @@ compute_branch_probabilities (void)
}
/* Load value histograms values whose description is stored in VALUES array
- from .gcda file. */
+ from .gcda file.
+
+ CFG_CHECKSUM is the precomputed checksum for the CFG. */
static void
-compute_value_histograms (histogram_values values)
+compute_value_histograms (histogram_values values, unsigned cfg_checksum)
{
unsigned i, j, t, any;
unsigned n_histogram_counters[GCOV_N_VALUE_COUNTERS];
@@ -803,7 +803,7 @@ compute_value_histograms (histogram_valu
histogram_counts[t] =
get_coverage_counts (COUNTER_FOR_HIST_TYPE (t),
- n_histogram_counters[t], NULL);
+ n_histogram_counters[t], cfg_checksum, NULL);
if (histogram_counts[t])
any = 1;
act_count[t] = histogram_counts[t];
@@ -906,6 +906,7 @@ branch_prob (void)
unsigned num_instrumented;
struct edge_list *el;
histogram_values values = NULL;
+ unsigned cfg_checksum, lineno_checksum;
total_num_times_called++;
@@ -1059,11 +1060,19 @@ branch_prob (void)
if (dump_file)
fprintf (dump_file, "%d ignored edges\n", ignored_edges);
+
+ /* Compute two different checksums. Note that we want to compute
+ the checksum in only once place, since it depends on the shape
+ of the control flow which can change during
+ various transformations. */
+ cfg_checksum = coverage_compute_cfg_checksum ();
+ lineno_checksum = coverage_compute_lineno_checksum ();
+
/* Write the data from which gcov can reconstruct the basic block
graph. */
/* Basic block flags */
- if (coverage_begin_output ())
+ if (coverage_begin_output (lineno_checksum, cfg_checksum))
{
gcov_position_t offset;
@@ -1080,7 +1089,7 @@ branch_prob (void)
EXIT_BLOCK_PTR->index = last_basic_block;
/* Arcs */
- if (coverage_begin_output ())
+ if (coverage_begin_output (lineno_checksum, cfg_checksum))
{
gcov_position_t offset;
@@ -1121,7 +1130,7 @@ branch_prob (void)
}
/* Line numbers. */
- if (coverage_begin_output ())
+ if (coverage_begin_output (lineno_checksum, cfg_checksum))
{
/* Initialize the output. */
output_location (NULL, 0, NULL, NULL);
@@ -1176,9 +1185,9 @@ branch_prob (void)
if (flag_branch_probabilities)
{
- compute_branch_probabilities ();
+ compute_branch_probabilities (cfg_checksum);
if (flag_profile_values)
- compute_value_histograms (values);
+ compute_value_histograms (values, cfg_checksum);
}
remove_fake_edges ();
@@ -1206,7 +1215,7 @@ branch_prob (void)
VEC_free (histogram_value, heap, values);
free_edge_list (el);
- coverage_end_function ();
+ coverage_end_function (lineno_checksum, cfg_checksum);
}
\f
/* Union find algorithm implementation for the basic blocks using
Index: coverage.c
===================================================================
--- coverage.c (revision 172385)
+++ coverage.c (working copy)
@@ -51,13 +51,17 @@ along with GCC; see the file COPYING3.
#include "intl.h"
#include "filenames.h"
+#include "gcov-io.h"
#include "gcov-io.c"
struct function_list
{
struct function_list *next; /* next function */
unsigned ident; /* function ident */
- unsigned checksum; /* function checksum */
+ unsigned lineno_checksum; /* function lineno checksum */
+ unsigned cfg_checksum; /* function cfg checksum */
+ const char *name; /* function name */
+ tree decl; /* function decl */
unsigned n_ctrs[GCOV_COUNTERS];/* number of counters. */
};
@@ -67,9 +71,11 @@ typedef struct counts_entry
/* We hash by */
unsigned ident;
unsigned ctr;
+ char *name;
/* Store */
- unsigned checksum;
+ unsigned lineno_checksum;
+ unsigned cfg_checksum;
gcov_type *counts;
struct gcov_ctr_summary summary;
@@ -114,14 +120,13 @@ static hashval_t htab_counts_entry_hash
static int htab_counts_entry_eq (const void *, const void *);
static void htab_counts_entry_del (void *);
static void read_counts_file (void);
-static unsigned compute_checksum (void);
-static unsigned coverage_checksum_string (unsigned, const char *);
static tree build_fn_info_type (unsigned);
static tree build_fn_info_value (const struct function_list *, tree);
static tree build_ctr_info_type (void);
static tree build_ctr_info_value (unsigned, tree);
static tree build_gcov_info (void);
static void create_coverage (void);
+static char * xstrdup_mask_random (const char *);
\f
/* Return the type node for gcov_type. */
@@ -139,12 +144,21 @@ get_gcov_unsigned_t (void)
return lang_hooks.types.type_for_size (32, true);
}
\f
+/* Return the type node for const char *. */
+
+static tree
+get_const_string_type (void)
+{
+ return build_pointer_type
+ (build_qualified_type (char_type_node, TYPE_QUAL_CONST));
+}
+\f
static hashval_t
htab_counts_entry_hash (const void *of)
{
const counts_entry_t *const entry = (const counts_entry_t *) of;
- return entry->ident * GCOV_COUNTERS + entry->ctr;
+ return htab_hash_string (entry->name) + entry->ctr;
}
static int
@@ -153,7 +167,8 @@ htab_counts_entry_eq (const void *of1, c
const counts_entry_t *const entry1 = (const counts_entry_t *) of1;
const counts_entry_t *const entry2 = (const counts_entry_t *) of2;
- return entry1->ident == entry2->ident && entry1->ctr == entry2->ctr;
+ return (entry1->ctr == entry2->ctr
+ && strcmp (entry1->name, entry2->name) == 0);
}
static void
@@ -161,6 +176,7 @@ htab_counts_entry_del (void *of)
{
counts_entry_t *const entry = (counts_entry_t *) of;
+ free (entry->name);
free (entry->counts);
free (entry);
}
@@ -171,11 +187,13 @@ static void
read_counts_file (void)
{
gcov_unsigned_t fn_ident = 0;
- gcov_unsigned_t checksum = -1;
+ char *name = NULL;
counts_entry_t *summaried = NULL;
unsigned seen_summary = 0;
gcov_unsigned_t tag;
int is_error = 0;
+ unsigned lineno_checksum = 0;
+ unsigned cfg_checksum = 0;
if (!gcov_open (da_file_name, 1))
return;
@@ -214,8 +232,21 @@ read_counts_file (void)
offset = gcov_position ();
if (tag == GCOV_TAG_FUNCTION)
{
+ const char *nm;
fn_ident = gcov_read_unsigned ();
- checksum = gcov_read_unsigned ();
+ lineno_checksum = gcov_read_unsigned ();
+ cfg_checksum = gcov_read_unsigned ();
+ nm = gcov_read_string();
+ if (!nm)
+ {
+ /* Error. Die! */
+ fatal_error ("corrupted profile - name of the function"
+ " (ident = 0x%x lineno_checksum 0x%x "
+ "ccfg_checksum 0x%x)"
+ " is NULL.", fn_ident, lineno_checksum,
+ cfg_checksum);
+ }
+ name = xstrdup (nm);
if (seen_summary)
{
/* We have already seen a summary, this means that this
@@ -257,6 +288,7 @@ read_counts_file (void)
unsigned ix;
elt.ident = fn_ident;
+ elt.name = name;
elt.ctr = GCOV_COUNTER_FOR_TAG (tag);
slot = (counts_entry_t **) htab_find_slot
@@ -265,17 +297,22 @@ read_counts_file (void)
if (!entry)
{
*slot = entry = XCNEW (counts_entry_t);
- entry->ident = elt.ident;
+ entry->ident = fn_ident;
+ entry->name = name;
entry->ctr = elt.ctr;
- entry->checksum = checksum;
+ entry->lineno_checksum = lineno_checksum;
+ entry->cfg_checksum = cfg_checksum;
entry->summary.num = n_counts;
entry->counts = XCNEWVEC (gcov_type, n_counts);
}
- else if (entry->checksum != checksum)
+ else if (entry->lineno_checksum != lineno_checksum
+ || entry->cfg_checksum != cfg_checksum)
{
error ("coverage mismatch for function %u while reading execution counters",
fn_ident);
- error ("checksum is %x instead of %x", entry->checksum, checksum);
+ error ("checksum is (%x,%x) instead of (%x,%x)",
+ entry->lineno_checksum, entry->cfg_checksum,
+ lineno_checksum, cfg_checksum);
htab_delete (counts_hash);
break;
}
@@ -323,11 +360,10 @@ read_counts_file (void)
/* Returns the counters for a particular tag. */
gcov_type *
-get_coverage_counts (unsigned counter, unsigned expected,
+get_coverage_counts (unsigned counter, unsigned expected, unsigned cfg_checksum,
const struct gcov_ctr_summary **summary)
{
counts_entry_t *entry, elt;
- gcov_unsigned_t checksum = -1;
/* No hash table, no counts. */
if (!counts_hash)
@@ -343,6 +379,8 @@ get_coverage_counts (unsigned counter, u
}
elt.ident = current_function_funcdef_no + 1;
+ elt.name = xstrdup_mask_random
+ (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (current_function_decl)));
elt.ctr = counter;
entry = (counts_entry_t *) htab_find (counts_hash, &elt);
if (!entry)
@@ -352,8 +390,7 @@ get_coverage_counts (unsigned counter, u
return NULL;
}
- checksum = compute_checksum ();
- if (entry->checksum != checksum
+ if (entry->cfg_checksum != cfg_checksum
|| entry->summary.num != expected)
{
static int warned = 0;
@@ -366,9 +403,9 @@ get_coverage_counts (unsigned counter, u
"%qE while reading counter %qs", id, ctr_names[counter]);
if (warning_printed)
{
- if (entry->checksum != checksum)
- inform (input_location, "checksum is %x instead of %x",
- entry->checksum, checksum);
+ if (entry->cfg_checksum != cfg_checksum)
+ inform (input_location, "cfg_checksum is %x instead of %x",
+ entry->cfg_checksum, cfg_checksum);
else
inform (input_location, "number of counters is %d instead of %d",
entry->summary.num, expected);
@@ -467,79 +504,125 @@ tree_coverage_counter_addr (unsigned cou
NULL, NULL));
}
\f
-/* Generate a checksum for a string. CHKSUM is the current
- checksum. */
-
-static unsigned
-coverage_checksum_string (unsigned chksum, const char *string)
+/* Mask out the random part of the identifier name.
+ Returns a pointer to the newly allocated string
+ that contains random part masked out to 0. */
+
+static char *
+xstrdup_mask_random (const char *string)
{
- int i;
- char *dup = NULL;
+ char *dup = xstrdup (string);
+ char *ptr = dup;
/* Look for everything that looks if it were produced by
get_file_function_name and zero out the second part
- that may result from flag_random_seed. This is not critical
- as the checksums are used only for sanity checking. */
- for (i = 0; string[i]; i++)
- {
- int offset = 0;
- if (!strncmp (string + i, "_GLOBAL__N_", 11))
- offset = 11;
- if (!strncmp (string + i, "_GLOBAL__", 9))
- offset = 9;
-
- /* C++ namespaces do have scheme:
- _GLOBAL__N_<filename>_<wrongmagicnumber>_<magicnumber>functionname
- since filename might contain extra underscores there seems
- to be no better chance then walk all possible offsets looking
- for magicnumber. */
- if (offset)
- {
- for (i = i + offset; string[i]; i++)
- if (string[i]=='_')
- {
- int y;
-
- for (y = 1; y < 9; y++)
- if (!(string[i + y] >= '0' && string[i + y] <= '9')
- && !(string[i + y] >= 'A' && string[i + y] <= 'F'))
- break;
- if (y != 9 || string[i + 9] != '_')
- continue;
- for (y = 10; y < 18; y++)
- if (!(string[i + y] >= '0' && string[i + y] <= '9')
- && !(string[i + y] >= 'A' && string[i + y] <= 'F'))
- break;
- if (y != 18)
- continue;
- if (!dup)
- string = dup = xstrdup (string);
- for (y = 10; y < 18; y++)
- dup[i + y] = '0';
- }
- break;
- }
+ that may result from flag_random_seed. This is critical
+ since we use the function name as the key for finding
+ the profile entry. */
+#define GLOBAL_PREFIX "_GLOBAL__"
+#define TRAILING_N "N_"
+#define ISCAPXDIGIT(a) (((a) >= '0' && (a) <= '9') || ((a) >= 'A' && (a) <= 'F'))
+ /* Thanks to anonymous namespace, we can have a function name
+ with multiple GLOBAL_PREFIX. */
+ while ((ptr = strstr (ptr, GLOBAL_PREFIX)))
+ {
+ char *temp_ptr;
+ /* Skip _GLOBAL__. */
+ ptr += strlen (GLOBAL_PREFIX);
+
+ /* Skip optional N_ (in case __GLOBAL_N__). */
+ if (!strncmp (ptr, TRAILING_N, strlen (TRAILING_N)))
+ ptr += strlen (TRAILING_N);
+ /* At this point, ptr should point after "_GLOBAL__N_" or "_GLOBAL__". */
+
+ while ((temp_ptr = strchr (ptr, '_')) != NULL)
+ {
+ int y;
+
+ ptr = temp_ptr;
+ /* For every "_" in the rest of the string,
+ try the follwing pattern matching */
+
+ /* Skip over '_'. */
+ ptr++;
+#define NDIGITS (8)
+ /* Try matching the pattern:
+ <8-digit hex>_<8-digit hex>
+ The second number is randomly generated
+ so we want to mask it out before computing the checksum. */
+ for (y = 0; *ptr != 0 && y < NDIGITS; y++, ptr++)
+ if (!ISCAPXDIGIT (*ptr))
+ break;
+ if (y != NDIGITS || *ptr != '_')
+ continue;
+ /* Skip over '_' again. */
+ ptr++;
+ for (y = 0; *ptr != 0 && y < NDIGITS; y++, ptr++)
+ if (!ISCAPXDIGIT (*ptr))
+ break;
+
+ if (y == NDIGITS)
+ {
+ /* We have a match.
+ Mask out the second 8-digit number. */
+ for(y = -NDIGITS - 1 ; y < 0; y++)
+ ptr[y] = '0';
+ break;
+ }
+ }
}
- chksum = crc32_string (chksum, string);
- if (dup)
- free (dup);
-
- return chksum;
+ return dup;
}
-/* Compute checksum for the current function. We generate a CRC32. */
-static unsigned
-compute_checksum (void)
+/* Compute checksum for the current function. We generate a CRC32.
+ TODO: This checksum can be made more stringent by computing
+ crc32 over the filename/lineno output in gcno. */
+
+unsigned
+coverage_compute_lineno_checksum (void)
{
expanded_location xloc
= expand_location (DECL_SOURCE_LOCATION (current_function_decl));
+ char *name = xstrdup_mask_random
+ (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (current_function_decl)));
unsigned chksum = xloc.line;
- chksum = coverage_checksum_string (chksum, xloc.file);
- chksum = coverage_checksum_string
- (chksum, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (current_function_decl)));
+ chksum = crc32_string (chksum, xloc.file);
+ chksum = crc32_string (chksum, name);
+
+ free (name);
+
+ return chksum;
+}
+
+
+/* Compute cfg checksum for the current function.
+ The checksum is calculated carefully so that
+ source code changes that doesn't affect the control flow graph
+ won't change the checksum.
+ This is to make the profile data useable across source code change.
+ The downside of this is that the compiler may use potentially
+ wrong profile data - that the source code change has non-trivial impact
+ on the validity of profile data (e.g. the reversed condition)
+ but the compiler won't detect the change and use the wrong profile data. */
+
+unsigned
+coverage_compute_cfg_checksum (void)
+{
+ basic_block bb;
+ unsigned chksum = n_basic_blocks;
+
+ FOR_EACH_BB (bb)
+ {
+ edge e;
+ edge_iterator ei;
+ FOR_EACH_EDGE (e, ei, bb->succs)
+ {
+ chksum = crc32_byte (chksum, e->dest->index);
+ }
+ }
return chksum;
}
@@ -550,7 +633,7 @@ compute_checksum (void)
should be output. */
int
-coverage_begin_output (void)
+coverage_begin_output (unsigned lineno_checksum, unsigned cfg_checksum)
{
/* We don't need to output .gcno file unless we're under -ftest-coverage
(e.g. -fprofile-arcs/generate/use don't need .gcno to work). */
@@ -562,6 +645,7 @@ coverage_begin_output (void)
expanded_location xloc
= expand_location (DECL_SOURCE_LOCATION (current_function_decl));
unsigned long offset;
+ char *name;
if (!bbg_file_opened)
{
@@ -576,17 +660,22 @@ coverage_begin_output (void)
bbg_file_opened = 1;
}
+ name = xstrdup_mask_random
+ (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (current_function_decl)));
+
/* Announce function */
offset = gcov_write_tag (GCOV_TAG_FUNCTION);
gcov_write_unsigned (current_function_funcdef_no + 1);
- gcov_write_unsigned (compute_checksum ());
- gcov_write_string (IDENTIFIER_POINTER
- (DECL_ASSEMBLER_NAME (current_function_decl)));
+ gcov_write_unsigned (lineno_checksum);
+ gcov_write_unsigned (cfg_checksum);
+ gcov_write_string (name);
gcov_write_string (xloc.file);
gcov_write_unsigned (xloc.line);
gcov_write_length (offset);
bbg_function_announced = 1;
+
+ free (name);
}
return !gcov_is_error ();
}
@@ -595,7 +684,7 @@ coverage_begin_output (void)
error has occurred. Save function coverage counts. */
void
-coverage_end_function (void)
+coverage_end_function (unsigned lineno_checksum, unsigned cfg_checksum)
{
unsigned i;
@@ -608,15 +697,22 @@ coverage_end_function (void)
if (fn_ctr_mask)
{
struct function_list *item;
+ const char *name;
item = XNEW (struct function_list);
*functions_tail = item;
functions_tail = &item->next;
+ name = xstrdup_mask_random
+ (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (current_function_decl)));
+
item->next = 0;
item->ident = current_function_funcdef_no + 1;
- item->checksum = compute_checksum ();
+ item->decl = current_function_decl;
+ item->name = name;
+ item->lineno_checksum = lineno_checksum;
+ item->cfg_checksum = cfg_checksum;
for (i = 0; i != GCOV_COUNTERS; i++)
{
item->n_ctrs[i] = fn_n_ctrs[i];
@@ -641,13 +737,24 @@ build_fn_info_type (unsigned int counter
/* ident */
fields = build_decl (BUILTINS_LOCATION,
FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
-
- /* checksum */
+ /* lineno_checksum */
field = build_decl (BUILTINS_LOCATION,
FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
DECL_CHAIN (field) = fields;
fields = field;
+ /* cfg checksum */
+ field = build_decl (BUILTINS_LOCATION,
+ FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
+ DECL_CHAIN (field) = fields;
+ fields = field;
+
+ /* name */
+ field = build_decl (BUILTINS_LOCATION,
+ FIELD_DECL, NULL_TREE, get_const_string_type ());
+ DECL_CHAIN (field) = fields;
+ fields = field;
+
array_type = build_int_cst (NULL_TREE, counters - 1);
array_type = build_index_type (array_type);
array_type = build_array_type (get_gcov_unsigned_t (), array_type);
@@ -681,10 +788,22 @@ build_fn_info_value (const struct functi
function->ident));
fields = DECL_CHAIN (fields);
- /* checksum */
+ /* lineno_checksum */
+ CONSTRUCTOR_APPEND_ELT (v1, fields,
+ build_int_cstu (get_gcov_unsigned_t (),
+ function->lineno_checksum));
+ fields = DECL_CHAIN (fields);
+
+ /* cfg_checksum */
CONSTRUCTOR_APPEND_ELT (v1, fields,
build_int_cstu (get_gcov_unsigned_t (),
- function->checksum));
+ function->cfg_checksum));
+ fields = DECL_CHAIN (fields);
+
+ /* name */
+ CONSTRUCTOR_APPEND_ELT (v1, fields,
+ build_string_literal (strlen (function->name) + 1,
+ function->name));
fields = DECL_CHAIN (fields);
/* counters */
Index: coverage.h
===================================================================
--- coverage.h (revision 172385)
+++ coverage.h (working copy)
@@ -28,11 +28,17 @@ extern void coverage_finish (void);
/* Complete the coverage information for the current function. Once
per function. */
-extern void coverage_end_function (void);
+extern void coverage_end_function (unsigned, unsigned);
/* Start outputting coverage information for the current
function. Repeatable per function. */
-extern int coverage_begin_output (void);
+extern int coverage_begin_output (unsigned, unsigned);
+
+/* Compute the control flow checksum for the current function. */
+extern unsigned coverage_compute_cfg_checksum (void);
+
+/* Compute the line number checksum for the current function. */
+extern unsigned coverage_compute_lineno_checksum (void);
/* Allocate some counters. Repeatable per function. */
extern int coverage_counter_alloc (unsigned /*counter*/, unsigned/*num*/);
@@ -44,6 +50,7 @@ extern tree tree_coverage_counter_addr (
/* Get all the counters for the current function. */
extern gcov_type *get_coverage_counts (unsigned /*counter*/,
unsigned /*expected*/,
+ unsigned /*checksum*/,
const struct gcov_ctr_summary **);
extern tree get_gcov_type (void);
Index: libgcov.c
===================================================================
--- libgcov.c (revision 172385)
+++ libgcov.c (working copy)
@@ -372,9 +372,10 @@ gcov_exit (void)
/* Check function. */
if (tag != GCOV_TAG_FUNCTION
- || length != GCOV_TAG_FUNCTION_LENGTH
|| gcov_read_unsigned () != fi_ptr->ident
- || gcov_read_unsigned () != fi_ptr->checksum)
+ || gcov_read_unsigned () != fi_ptr->lineno_checksum
+ || gcov_read_unsigned () != fi_ptr->cfg_checksum
+ || strcmp (gcov_read_string (), fi_ptr->name))
{
read_mismatch:;
fprintf (stderr, "profiling:%s:Merge mismatch for %s\n",
@@ -515,9 +516,13 @@ gcov_exit (void)
((const char *) gi_ptr->functions + f_ix * fi_stride);
/* Announce function. */
- gcov_write_tag_length (GCOV_TAG_FUNCTION, GCOV_TAG_FUNCTION_LENGTH);
+ gcov_write_tag_length
+ (GCOV_TAG_FUNCTION,
+ GCOV_TAG_FUNCTION_LENGTH + gcov_string_length (fi_ptr->name));
gcov_write_unsigned (fi_ptr->ident);
- gcov_write_unsigned (fi_ptr->checksum);
+ gcov_write_unsigned (fi_ptr->lineno_checksum);
+ gcov_write_unsigned (fi_ptr->cfg_checksum);
+ gcov_write_string (fi_ptr->name);
c_ix = 0;
for (t_ix = 0; t_ix < GCOV_COUNTERS; t_ix++)
Index: gcov-dump.c
===================================================================
--- gcov-dump.c (revision 172385)
+++ gcov-dump.c (working copy)
@@ -265,21 +265,18 @@ tag_function (const char *filename ATTRI
unsigned tag ATTRIBUTE_UNUSED, unsigned length ATTRIBUTE_UNUSED)
{
unsigned long pos = gcov_position ();
+ const char *name;
printf (" ident=%u", gcov_read_unsigned ());
- printf (", checksum=0x%08x", gcov_read_unsigned ());
+ printf (", lineno_checksum=0x%08x", gcov_read_unsigned ());
+ printf (", cfg_checksum=0x%08x", gcov_read_unsigned ());
- if (gcov_position () - pos < length)
- {
- const char *name;
-
- name = gcov_read_string ();
- printf (", `%s'", name ? name : "NULL");
name = gcov_read_string ();
printf (" %s", name ? name : "NULL");
+
+ if (gcov_position () - pos < length)
printf (":%u", gcov_read_unsigned ());
}
-}
static void
tag_blocks (const char *filename ATTRIBUTE_UNUSED,
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-13 22:26 FDO usability patch -- cfg and lineno checksum Xinliang David Li
@ 2011-04-13 22:34 ` Xinliang David Li
[not found] ` <BANLkTinVavyXCqy88TpqLzmexp2LQ1Pwqw@mail.gmail.com>
2011-04-17 17:15 ` Jan Hubicka
2 siblings, 0 replies; 19+ messages in thread
From: Xinliang David Li @ 2011-04-13 22:34 UTC (permalink / raw)
To: GCC Patches; +Cc: nvachhar
This was contributed by:
2011-04-13 Neil Vachharajani <nvachhar@gmail.com>
On Wed, Apr 13, 2011 at 3:25 PM, Xinliang David Li <davidxl@google.com> wrote:
> Hi, in current FDO implementation, the source file version used in
> profile-generate needs to strictly match the version used in
> profile-use -- simple formating changes will invalidate the profile
> data (use of it will lead to compiler error or ICE). There are two
> main problems that lead to the weakness:
>
> 1) the function checksum is computed based on line number and number
> of basic blocks -- this means any line number change will invalidate
> the profile data. In fact, line number should play very minimal role
> in profile matching decision. Another problem is that number of basic
> blocks is also not a good indicator whether CFG matches or not.
> 2) cgraph's pid is used as the key to find the matching profile data
> for a function. If there is any new function added, or if the order of
> the functions changes, the profile data is invalidated.
>
> The attached is a patch from google that improves this.
> 1) function checksum is split into two parts: lineno checksum and cfg checksum
> 2) function assembler name is used in profile hashing.
>
> Bootstrapped and regression tested on x86_64/linux.
>
> Ok for trunk?
>
> Thanks,
>
> David
>
> 2011-04-13 Xinliang David Li <davidxl@google.com>
>
> * tree.c (crc32_byte): New function.
> * tree.h (crc32_byte): New function.
> * gcov.c (function_info): Add new fields.
> (read_graph_file): Handle new fields.
> (read_count_file): Handle name.
> * gcov-io.c (gcov_string_length): New function.
> (gcov_write_words): Bug fix.
> (gcov_read_words): Bug fix.
> * gcov-io.h: New interfaces.
> * profile.c (get_exec_counts): New parameter.
> (compute_branch_probablilities): New parameter.
> (compute_value_histogram): New parameter.
> (branch_prob): Pass cfg_checksum.
> * coverage.c (get_const_string_type): New function.
> (hash_counts_entry_hash): Use string hash.
> (hash_counts_entry_eq): Use string compare.
> (htab_counts_entry_del): Delete name.
> (read_count_file): Add handling of cfg checksum.
> (get_coverage_counts): New parameter.
> (xstrdup_mask_random): New function.
> (coverage_compute_lineno_checksum): New function.
> (coverage_compute_cfg_checksum): New function.
> (coverage_begin_output): New parameter.
> (coverage_end_function): New parameter.
> (build_fn_info_type): Build new fields.
> (build_fn_info_value): Build new field values.
> * coverage.h: Interface changes.
> * libgcov.c (gcov_exit): Dump new fields.
> * gcov_dump.c (tag_function): Print new fields.
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
[not found] ` <BANLkTinVavyXCqy88TpqLzmexp2LQ1Pwqw@mail.gmail.com>
@ 2011-04-15 16:54 ` Xinliang David Li
0 siblings, 0 replies; 19+ messages in thread
From: Xinliang David Li @ 2011-04-15 16:54 UTC (permalink / raw)
To: GCC Patches; +Cc: Jan Hubicka, Neil Vachharajani
Resent in plain text mode ..
David
On Fri, Apr 15, 2011 at 9:28 AM, Xinliang David Li <davidxl@google.com> wrote:
>
> Honza, do you have a chance to take a look at it?
> Thanks,
>
> David
>
> On Wed, Apr 13, 2011 at 3:25 PM, Xinliang David Li <davidxl@google.com> wrote:
>>
>> Hi, in current FDO implementation, the source file version used in
>> profile-generate needs to strictly match the version used in
>> profile-use -- simple formating changes will invalidate the profile
>> data (use of it will lead to compiler error or ICE). There are two
>> main problems that lead to the weakness:
>>
>> 1) the function checksum is computed based on line number and number
>> of basic blocks -- this means any line number change will invalidate
>> the profile data. In fact, line number should play very minimal role
>> in profile matching decision. Another problem is that number of basic
>> blocks is also not a good indicator whether CFG matches or not.
>> 2) cgraph's pid is used as the key to find the matching profile data
>> for a function. If there is any new function added, or if the order of
>> the functions changes, the profile data is invalidated.
>>
>> The attached is a patch from google that improves this.
>> 1) function checksum is split into two parts: lineno checksum and cfg checksum
>> 2) function assembler name is used in profile hashing.
>>
>> Bootstrapped and regression tested on x86_64/linux.
>>
>> Ok for trunk?
>>
>> Thanks,
>>
>> David
>>
>> 2011-04-13 Xinliang David Li <davidxl@google.com>
>>
>> * tree.c (crc32_byte): New function.
>> * tree.h (crc32_byte): New function.
>> * gcov.c (function_info): Add new fields.
>> (read_graph_file): Handle new fields.
>> (read_count_file): Handle name.
>> * gcov-io.c (gcov_string_length): New function.
>> (gcov_write_words): Bug fix.
>> (gcov_read_words): Bug fix.
>> * gcov-io.h: New interfaces.
>> * profile.c (get_exec_counts): New parameter.
>> (compute_branch_probablilities): New parameter.
>> (compute_value_histogram): New parameter.
>> (branch_prob): Pass cfg_checksum.
>> * coverage.c (get_const_string_type): New function.
>> (hash_counts_entry_hash): Use string hash.
>> (hash_counts_entry_eq): Use string compare.
>> (htab_counts_entry_del): Delete name.
>> (read_count_file): Add handling of cfg checksum.
>> (get_coverage_counts): New parameter.
>> (xstrdup_mask_random): New function.
>> (coverage_compute_lineno_checksum): New function.
>> (coverage_compute_cfg_checksum): New function.
>> (coverage_begin_output): New parameter.
>> (coverage_end_function): New parameter.
>> (build_fn_info_type): Build new fields.
>> (build_fn_info_value): Build new field values.
>> * coverage.h: Interface changes.
>> * libgcov.c (gcov_exit): Dump new fields.
>> * gcov_dump.c (tag_function): Print new fields.
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-13 22:26 FDO usability patch -- cfg and lineno checksum Xinliang David Li
2011-04-13 22:34 ` Xinliang David Li
[not found] ` <BANLkTinVavyXCqy88TpqLzmexp2LQ1Pwqw@mail.gmail.com>
@ 2011-04-17 17:15 ` Jan Hubicka
2011-04-17 19:22 ` Xinliang David Li
2011-04-19 20:37 ` Xinliang David Li
2 siblings, 2 replies; 19+ messages in thread
From: Jan Hubicka @ 2011-04-17 17:15 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
> Hi, in current FDO implementation, the source file version used in
> profile-generate needs to strictly match the version used in
> profile-use -- simple formating changes will invalidate the profile
> data (use of it will lead to compiler error or ICE). There are two
> main problems that lead to the weakness:
>
> 1) the function checksum is computed based on line number and number
> of basic blocks -- this means any line number change will invalidate
> the profile data. In fact, line number should play very minimal role
> in profile matching decision. Another problem is that number of basic
> blocks is also not a good indicator whether CFG matches or not.
> 2) cgraph's pid is used as the key to find the matching profile data
> for a function. If there is any new function added, or if the order of
> the functions changes, the profile data is invalidated.
>
> The attached is a patch from google that improves this.
> 1) function checksum is split into two parts: lineno checksum and cfg checksum
> 2) function assembler name is used in profile hashing.
Well, the overall idea was to make profile intolerant to pretty much any source
level change, since you never know if even when the CFG shape match, the actual
probabiliies did not changed.
I however see that during development it is not bad to make compiler grok
the profile as long as it seems consistent.
I did not looked in detail into the implementation yet, but it seems
sane at first glance. However what about issuing a warning when line number
information change telling user that profile might no longer be accurate
and that he may want to remove it and train again?
Honza
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-17 17:15 ` Jan Hubicka
@ 2011-04-17 19:22 ` Xinliang David Li
2011-04-19 20:37 ` Xinliang David Li
1 sibling, 0 replies; 19+ messages in thread
From: Xinliang David Li @ 2011-04-17 19:22 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, Neil Vachharajani
On Sun, Apr 17, 2011 at 8:36 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Hi, in current FDO implementation, the source file version used in
>> profile-generate needs to strictly match the version used in
>> profile-use -- simple formating changes will invalidate the profile
>> data (use of it will lead to compiler error or ICE). There are two
>> main problems that lead to the weakness:
>>
>> 1) the function checksum is computed based on line number and number
>> of basic blocks -- this means any line number change will invalidate
>> the profile data. In fact, line number should play very minimal role
>> in profile matching decision. Another problem is that number of basic
>> blocks is also not a good indicator whether CFG matches or not.
>> 2) cgraph's pid is used as the key to find the matching profile data
>> for a function. If there is any new function added, or if the order of
>> the functions changes, the profile data is invalidated.
>>
>> The attached is a patch from google that improves this.
>> 1) function checksum is split into two parts: lineno checksum and cfg checksum
>> 2) function assembler name is used in profile hashing.
>
> Well, the overall idea was to make profile intolerant to pretty much any source
> level change, since you never know if even when the CFG shape match, the actual
> probabiliies did not changed.
> I however see that during development it is not bad to make compiler grok
> the profile as long as it seems consistent.
>
Make FDO usable in development cycle is one usage of the better
matching tolerance. There is another bigger use case which we are
looking at -- using sample profiling data from production binary
collected by the profile server to guide optimization. Of course there
will be lots of other problems (e.g., compiler optimizations can
drastically change control flow, etc.) before this can become reality.
> I did not looked in detail into the implementation yet, but it seems
> sane at first glance. However what about issuing a warning when line number
> information change telling user that profile might no longer be accurate
> and that he may want to remove it and train again?
Yes, this may be good to have.
Thanks,
David
>
> Honza
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-17 17:15 ` Jan Hubicka
2011-04-17 19:22 ` Xinliang David Li
@ 2011-04-19 20:37 ` Xinliang David Li
2011-04-19 22:36 ` Jan Hubicka
2011-04-21 18:23 ` Diego Novillo
1 sibling, 2 replies; 19+ messages in thread
From: Xinliang David Li @ 2011-04-19 20:37 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1957 bytes --]
The attached is the revised patch with a warning suggested for cases
when CFG matches, but source locations change.
Ok for trunk?
thanks,
David
On Sun, Apr 17, 2011 at 8:36 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Hi, in current FDO implementation, the source file version used in
>> profile-generate needs to strictly match the version used in
>> profile-use -- simple formating changes will invalidate the profile
>> data (use of it will lead to compiler error or ICE). There are two
>> main problems that lead to the weakness:
>>
>> 1) the function checksum is computed based on line number and number
>> of basic blocks -- this means any line number change will invalidate
>> the profile data. In fact, line number should play very minimal role
>> in profile matching decision. Another problem is that number of basic
>> blocks is also not a good indicator whether CFG matches or not.
>> 2) cgraph's pid is used as the key to find the matching profile data
>> for a function. If there is any new function added, or if the order of
>> the functions changes, the profile data is invalidated.
>>
>> The attached is a patch from google that improves this.
>> 1) function checksum is split into two parts: lineno checksum and cfg checksum
>> 2) function assembler name is used in profile hashing.
>
> Well, the overall idea was to make profile intolerant to pretty much any source
> level change, since you never know if even when the CFG shape match, the actual
> probabiliies did not changed.
> I however see that during development it is not bad to make compiler grok
> the profile as long as it seems consistent.
>
> I did not looked in detail into the implementation yet, but it seems
> sane at first glance. However what about issuing a warning when line number
> information change telling user that profile might no longer be accurate
> and that he may want to remove it and train again?
>
> Honza
>
[-- Attachment #2: cfg_chksum2.p --]
[-- Type: text/x-pascal, Size: 40862 bytes --]
Index: tree.c
===================================================================
--- tree.c (revision 172693)
+++ tree.c (working copy)
@@ -8508,14 +8508,12 @@ dump_tree_statistics (void)
\f
#define FILE_FUNCTION_FORMAT "_GLOBAL__%s_%s"
-/* Generate a crc32 of a string. */
+/* Generate a crc32 of a byte. */
unsigned
-crc32_string (unsigned chksum, const char *string)
+crc32_byte (unsigned chksum, char byte)
{
- do
- {
- unsigned value = *string << 24;
+ unsigned value = (unsigned) byte << 24;
unsigned ix;
for (ix = 8; ix--; value <<= 1)
@@ -8526,6 +8524,18 @@ crc32_string (unsigned chksum, const cha
chksum <<= 1;
chksum ^= feedback;
}
+ return chksum;
+}
+
+
+/* Generate a crc32 of a string. */
+
+unsigned
+crc32_string (unsigned chksum, const char *string)
+{
+ do
+ {
+ chksum = crc32_byte (chksum, *string);
}
while (*string++);
return chksum;
@@ -8549,8 +8559,10 @@ clean_symbol_name (char *p)
*p = '_';
}
-/* Generate a name for a special-purpose function function.
+/* Generate a name for a special-purpose function.
The generated name may need to be unique across the whole link.
+ Changes to this function may also require corresponding changes to
+ xstrdup_mask_random.
TYPE is some string to identify the purpose of this function to the
linker or collect2; it must start with an uppercase letter,
one of:
Index: tree.h
===================================================================
--- tree.h (revision 172693)
+++ tree.h (working copy)
@@ -4998,6 +4998,7 @@ inlined_function_outer_scope_p (const_tr
\f
/* In tree.c */
extern unsigned crc32_string (unsigned, const char *);
+extern unsigned crc32_byte (unsigned, char);
extern void clean_symbol_name (char *);
extern tree get_file_function_name (const char *);
extern tree get_callee_fndecl (const_tree);
Index: gcov.c
===================================================================
--- gcov.c (revision 172693)
+++ gcov.c (working copy)
@@ -54,6 +54,13 @@ along with Gcov; see the file COPYING3.
some places we make use of the knowledge of how profile.c works to
select particular algorithms here. */
+/* The code validates that the profile information read in corresponds
+ to the code currently being compiled. Rather than checking for
+ identical files, the code below computes a checksum on the CFG
+ (based on the order of basic blocks and the arcs in the CFG). If
+ the CFG checksum in the gcda file match the CFG checksum for the
+ code currently being compiled, the profile data will be used. */
+
/* This is the size of the buffer used to read in source file lines. */
#define STRING_SIZE 200
@@ -161,7 +168,8 @@ typedef struct function_info
/* Name of function. */
char *name;
unsigned ident;
- unsigned checksum;
+ unsigned lineno_checksum;
+ unsigned cfg_checksum;
/* Array of basic blocks. */
block_t *blocks;
@@ -809,12 +817,14 @@ read_graph_file (void)
if (tag == GCOV_TAG_FUNCTION)
{
char *function_name;
- unsigned ident, checksum, lineno;
+ unsigned ident, lineno;
+ unsigned lineno_checksum, cfg_checksum;
source_t *src;
function_t *probe, *prev;
ident = gcov_read_unsigned ();
- checksum = gcov_read_unsigned ();
+ lineno_checksum = gcov_read_unsigned ();
+ cfg_checksum = gcov_read_unsigned ();
function_name = xstrdup (gcov_read_string ());
src = find_source (gcov_read_string ());
lineno = gcov_read_unsigned ();
@@ -822,7 +832,8 @@ read_graph_file (void)
fn = XCNEW (function_t);
fn->name = function_name;
fn->ident = ident;
- fn->checksum = checksum;
+ fn->lineno_checksum = lineno_checksum;
+ fn->cfg_checksum = cfg_checksum;
fn->src = src;
fn->line = lineno;
@@ -1039,6 +1050,7 @@ read_count_file (void)
unsigned tag;
function_t *fn = NULL;
int error = 0;
+ const char *fn_name;
if (!gcov_open (da_file_name, 1))
{
@@ -1109,13 +1121,20 @@ read_count_file (void)
if (!fn)
;
- else if (gcov_read_unsigned () != fn->checksum)
+ else if (gcov_read_unsigned () != fn->lineno_checksum
+ || gcov_read_unsigned () != fn->cfg_checksum)
{
mismatch:;
fnotice (stderr, "%s:profile mismatch for '%s'\n",
da_file_name, fn->name);
goto cleanup;
}
+
+ fn_name = gcov_read_string ();
+ if (strcmp (fn_name, fn->name) != 0)
+ fnotice (stderr, "%s:profile mismatch:"
+ " expected function name '%s' but found '%s'\n",
+ da_file_name, fn->name, fn_name);
}
else if (tag == GCOV_TAG_FOR_COUNTER (GCOV_COUNTER_ARCS) && fn)
{
Index: testsuite/gcc.dg/tree-prof/prof-robust-1.c
===================================================================
--- testsuite/gcc.dg/tree-prof/prof-robust-1.c (revision 0)
+++ testsuite/gcc.dg/tree-prof/prof-robust-1.c (revision 0)
@@ -0,0 +1,21 @@
+/* { dg-options "-O2 -w" } */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#ifdef _PROFILE_USE
+int foo(int x) {
+ return 3 * x;
+}
+#endif
+
+int x = 1000;
+
+int main(int argc, char *argv[]) {
+ int i;
+ int sum = 0;
+ for (i = 0; i < x; i++)
+ sum += i;
+ printf("%d\n", sum);
+ return 0;
+}
Index: testsuite/g++.dg/prof-robust-1.C
===================================================================
--- testsuite/g++.dg/prof-robust-1.C (revision 0)
+++ testsuite/g++.dg/prof-robust-1.C (revision 0)
@@ -0,0 +1,24 @@
+/* { dg-options "-O2 -fno-weak" } */
+
+#include <stdio.h>
+
+namespace {
+ namespace {
+
+ class MyClass {
+ public:
+ void foo() const;
+ ~MyClass() { foo(); }
+ };
+
+ void MyClass::foo() const { printf("Goodbye World\n"); }
+
+ }
+
+ static MyClass variable;
+
+}
+
+int main() {
+ return 0;
+}
Index: gcov-io.c
===================================================================
--- gcov-io.c (revision 172693)
+++ gcov-io.c (working copy)
@@ -28,6 +28,12 @@ see the files COPYING3 and COPYING.RUNTI
/* Routines declared in gcov-io.h. This file should be #included by
another source file, after having #included gcov-io.h. */
+/* Redefine these here, rather than using the ones in system.h since
+ * including system.h leads to conflicting definitions of other
+ * symbols and macros. */
+#undef MIN
+#define MIN(X,Y) ((X) < (Y) ? (X) : (Y))
+
#if !IN_GCOV
static void gcov_write_block (unsigned);
static gcov_unsigned_t *gcov_write_words (unsigned);
@@ -228,6 +234,27 @@ gcov_write_block (unsigned size)
gcov_var.offset -= size;
}
+#if IN_LIBGCOV
+
+/* These functions are guarded by #if to avoid compile time warning. */
+
+/* Return the number of words STRING would need including the length
+ field in the output stream itself. This should be identical to
+ "alloc" calculation in gcov_write_string(). */
+
+GCOV_LINKAGE gcov_unsigned_t
+gcov_string_length (const char *string)
+{
+ gcov_unsigned_t len = (string) ? strlen (string) : 0;
+ /* + 1 because of the length field. */
+ gcov_unsigned_t alloc = 1 + ((len + 4) >> 2);
+
+ /* Can not write a bigger than GCOV_BLOCK_SIZE string yet */
+ gcc_assert (alloc < GCOV_BLOCK_SIZE);
+ return alloc;
+}
+#endif
+
/* Allocate space to write BYTES bytes to the gcov file. Return a
pointer to those bytes, or NULL on failure. */
@@ -238,13 +265,15 @@ gcov_write_words (unsigned words)
gcc_assert (gcov_var.mode < 0);
#if IN_LIBGCOV
- if (gcov_var.offset >= GCOV_BLOCK_SIZE)
+ if (gcov_var.offset + words >= GCOV_BLOCK_SIZE)
{
- gcov_write_block (GCOV_BLOCK_SIZE);
+ gcov_write_block (MIN (gcov_var.offset, GCOV_BLOCK_SIZE));
if (gcov_var.offset)
{
- gcc_assert (gcov_var.offset == 1);
- memcpy (gcov_var.buffer, gcov_var.buffer + GCOV_BLOCK_SIZE, 4);
+ gcc_assert (gcov_var.offset < GCOV_BLOCK_SIZE);
+ memcpy (gcov_var.buffer,
+ gcov_var.buffer + GCOV_BLOCK_SIZE,
+ gcov_var.offset << 2);
}
}
#else
@@ -285,7 +314,6 @@ gcov_write_counter (gcov_type value)
}
#endif /* IN_LIBGCOV */
-#if !IN_LIBGCOV
/* Write STRING to coverage file. Sets error flag on file
error, overflow flag on overflow */
@@ -308,7 +336,7 @@ gcov_write_string (const char *string)
buffer[alloc] = 0;
memcpy (&buffer[1], string, length);
}
-#endif
+
#if !IN_LIBGCOV
/* Write a tag TAG and reserve space for the record length. Return a
@@ -396,14 +424,15 @@ gcov_read_words (unsigned words)
unsigned excess = gcov_var.length - gcov_var.offset;
gcc_assert (gcov_var.mode > 0);
+ gcc_assert (words < GCOV_BLOCK_SIZE);
if (excess < words)
{
gcov_var.start += gcov_var.offset;
#if IN_LIBGCOV
if (excess)
{
- gcc_assert (excess == 1);
- memcpy (gcov_var.buffer, gcov_var.buffer + gcov_var.offset, 4);
+ gcc_assert (excess < GCOV_BLOCK_SIZE);
+ memmove (gcov_var.buffer, gcov_var.buffer + gcov_var.offset, excess * 4);
}
#else
memmove (gcov_var.buffer, gcov_var.buffer + gcov_var.offset, excess * 4);
@@ -411,8 +440,7 @@ gcov_read_words (unsigned words)
gcov_var.offset = 0;
gcov_var.length = excess;
#if IN_LIBGCOV
- gcc_assert (!gcov_var.length || gcov_var.length == 1);
- excess = GCOV_BLOCK_SIZE;
+ excess = (sizeof (gcov_var.buffer) / sizeof (gcov_var.buffer[0])) - gcov_var.length;
#else
if (gcov_var.length + words > gcov_var.alloc)
gcov_allocate (gcov_var.length + words);
@@ -472,7 +500,6 @@ gcov_read_counter (void)
buffer, or NULL on empty string. You must copy the string before
calling another gcov function. */
-#if !IN_LIBGCOV
GCOV_LINKAGE const char *
gcov_read_string (void)
{
@@ -483,7 +510,7 @@ gcov_read_string (void)
return (const char *) gcov_read_words (length);
}
-#endif
+
GCOV_LINKAGE void
gcov_read_summary (struct gcov_summary *summary)
Index: gcov-io.h
===================================================================
--- gcov-io.h (revision 172693)
+++ gcov-io.h (working copy)
@@ -101,9 +101,10 @@ see the files COPYING3 and COPYING.RUNTI
The basic block graph file contains the following records
note: unit function-graph*
- unit: header int32:checksum string:source
+ unit: header int32:checksum int32: string:source
function-graph: announce_function basic_blocks {arcs | lines}*
- announce_function: header int32:ident int32:checksum
+ announce_function: header int32:ident
+ int32:lineno_checksum int32:cfg_checksum
string:name string:source int32:lineno
basic_block: header int32:flags*
arcs: header int32:block_no arc*
@@ -132,7 +133,9 @@ see the files COPYING3 and COPYING.RUNTI
data: {unit function-data* summary:object summary:program*}*
unit: header int32:checksum
function-data: announce_function arc_counts
- announce_function: header int32:ident int32:checksum
+ announce_function: header int32:ident
+ int32:lineno_checksum int32:cfg_checksum
+ string:name
arc_counts: header int64:count*
summary: int32:checksum {count-summary}GCOV_COUNTERS
count-summary: int32:num int32:runs int64:sum
@@ -243,13 +246,16 @@ typedef HOST_WIDEST_INT gcov_type;
#define gcov_write_unsigned __gcov_write_unsigned
#define gcov_write_counter __gcov_write_counter
#define gcov_write_summary __gcov_write_summary
+#define gcov_write_string __gcov_write_string
+#define gcov_string_length __gcov_string_length
#define gcov_read_unsigned __gcov_read_unsigned
#define gcov_read_counter __gcov_read_counter
+#define gcov_read_string __gcov_read_string
#define gcov_read_summary __gcov_read_summary
/* Poison these, so they don't accidentally slip in. */
-#pragma GCC poison gcov_write_string gcov_write_tag gcov_write_length
-#pragma GCC poison gcov_read_string gcov_sync gcov_time gcov_magic
+#pragma GCC poison gcov_write_tag gcov_write_length
+#pragma GCC poison gcov_sync gcov_time gcov_magic
#ifdef HAVE_GAS_HIDDEN
#define ATTRIBUTE_HIDDEN __attribute__ ((__visibility__ ("hidden")))
@@ -294,7 +300,7 @@ typedef HOST_WIDEST_INT gcov_type;
file marker -- it is not required to be present. */
#define GCOV_TAG_FUNCTION ((gcov_unsigned_t)0x01000000)
-#define GCOV_TAG_FUNCTION_LENGTH (2)
+#define GCOV_TAG_FUNCTION_LENGTH (3)
#define GCOV_TAG_BLOCKS ((gcov_unsigned_t)0x01410000)
#define GCOV_TAG_BLOCKS_LENGTH(NUM) (NUM)
#define GCOV_TAG_BLOCKS_NUM(LENGTH) (LENGTH)
@@ -411,11 +417,20 @@ struct gcov_summary
/* Information about a single function. This uses the trailing array
idiom. The number of counters is determined from the counter_mask
in gcov_info. We hold an array of function info, so have to
- explicitly calculate the correct array stride. */
+ explicitly calculate the correct array stride.
+
+ "ident" is no longer used in hash computation. Additionally,
+ "name" is used in hash computation. This makes the profile data
+ not compatible across function name changes.
+ Also, the function pointer is stored for later use for indirect
+ function call name resolution.
+ */
struct gcov_fn_info
{
gcov_unsigned_t ident; /* unique ident of function */
- gcov_unsigned_t checksum; /* function checksum */
+ gcov_unsigned_t lineno_checksum; /* function lineo_checksum */
+ gcov_unsigned_t cfg_checksum; /* function cfg checksum */
+ const char *name; /* name of the function */
unsigned n_ctrs[0]; /* instrumented counters */
};
@@ -543,6 +558,7 @@ static int gcov_is_error (void);
GCOV_LINKAGE gcov_unsigned_t gcov_read_unsigned (void) ATTRIBUTE_HIDDEN;
GCOV_LINKAGE gcov_type gcov_read_counter (void) ATTRIBUTE_HIDDEN;
GCOV_LINKAGE void gcov_read_summary (struct gcov_summary *) ATTRIBUTE_HIDDEN;
+GCOV_LINKAGE const char *gcov_read_string (void) ATTRIBUTE_HIDDEN;
#if IN_LIBGCOV
/* Available only in libgcov */
@@ -554,9 +570,9 @@ GCOV_LINKAGE void gcov_write_summary (gc
ATTRIBUTE_HIDDEN;
static void gcov_rewrite (void);
GCOV_LINKAGE void gcov_seek (gcov_position_t /*position*/) ATTRIBUTE_HIDDEN;
+GCOV_LINKAGE gcov_unsigned_t gcov_string_length (const char *) ATTRIBUTE_HIDDEN;
#else
/* Available outside libgcov */
-GCOV_LINKAGE const char *gcov_read_string (void);
GCOV_LINKAGE void gcov_sync (gcov_position_t /*base*/,
gcov_unsigned_t /*length */);
#endif
@@ -564,11 +580,11 @@ GCOV_LINKAGE void gcov_sync (gcov_positi
#if !IN_GCOV
/* Available outside gcov */
GCOV_LINKAGE void gcov_write_unsigned (gcov_unsigned_t) ATTRIBUTE_HIDDEN;
+GCOV_LINKAGE void gcov_write_string (const char *) ATTRIBUTE_HIDDEN;
#endif
#if !IN_GCOV && !IN_LIBGCOV
/* Available only in compiler */
-GCOV_LINKAGE void gcov_write_string (const char *);
GCOV_LINKAGE gcov_position_t gcov_write_tag (gcov_unsigned_t);
GCOV_LINKAGE void gcov_write_length (gcov_position_t /*position*/);
#endif
Index: profile.c
===================================================================
--- profile.c (revision 172693)
+++ profile.c (working copy)
@@ -102,13 +102,6 @@ static int total_num_branches;
/* Forward declarations. */
static void find_spanning_tree (struct edge_list *);
-static unsigned instrument_edges (struct edge_list *);
-static void instrument_values (histogram_values);
-static void compute_branch_probabilities (void);
-static void compute_value_histograms (histogram_values);
-static gcov_type * get_exec_counts (void);
-static basic_block find_group (basic_block);
-static void union_groups (basic_block, basic_block);
/* Add edge instrumentation code to the entire insn chain.
@@ -233,10 +226,12 @@ instrument_values (histogram_values valu
}
\f
-/* Computes hybrid profile for all matching entries in da_file. */
+/* Computes hybrid profile for all matching entries in da_file.
+
+ CFG_CHECKSUM is the precomputed checksum for the CFG. */
static gcov_type *
-get_exec_counts (void)
+get_exec_counts (unsigned cfg_checksum, unsigned lineno_checksum)
{
unsigned num_edges = 0;
basic_block bb;
@@ -253,7 +248,8 @@ get_exec_counts (void)
num_edges++;
}
- counts = get_coverage_counts (GCOV_COUNTER_ARCS, num_edges, &profile_info);
+ counts = get_coverage_counts (GCOV_COUNTER_ARCS, num_edges, cfg_checksum,
+ lineno_checksum, &profile_info);
if (!counts)
return NULL;
@@ -442,10 +438,12 @@ read_profile_edge_counts (gcov_type *exe
}
/* Compute the branch probabilities for the various branches.
- Annotate them accordingly. */
+ Annotate them accordingly.
+
+ CFG_CHECKSUM is the precomputed checksum for the CFG. */
static void
-compute_branch_probabilities (void)
+compute_branch_probabilities (unsigned cfg_checksum, unsigned lineno_checksum)
{
basic_block bb;
int i;
@@ -454,7 +452,7 @@ compute_branch_probabilities (void)
int passes;
int hist_br_prob[20];
int num_branches;
- gcov_type *exec_counts = get_exec_counts ();
+ gcov_type *exec_counts = get_exec_counts (cfg_checksum, lineno_checksum);
int inconsistent = 0;
/* Very simple sanity checks so we catch bugs in our profiling code. */
@@ -772,10 +770,13 @@ compute_branch_probabilities (void)
}
/* Load value histograms values whose description is stored in VALUES array
- from .gcda file. */
+ from .gcda file.
+
+ CFG_CHECKSUM is the precomputed checksum for the CFG. */
static void
-compute_value_histograms (histogram_values values)
+compute_value_histograms (histogram_values values, unsigned cfg_checksum,
+ unsigned lineno_checksum)
{
unsigned i, j, t, any;
unsigned n_histogram_counters[GCOV_N_VALUE_COUNTERS];
@@ -803,7 +804,8 @@ compute_value_histograms (histogram_valu
histogram_counts[t] =
get_coverage_counts (COUNTER_FOR_HIST_TYPE (t),
- n_histogram_counters[t], NULL);
+ n_histogram_counters[t], cfg_checksum,
+ lineno_checksum, NULL);
if (histogram_counts[t])
any = 1;
act_count[t] = histogram_counts[t];
@@ -906,6 +908,7 @@ branch_prob (void)
unsigned num_instrumented;
struct edge_list *el;
histogram_values values = NULL;
+ unsigned cfg_checksum, lineno_checksum;
total_num_times_called++;
@@ -1059,11 +1062,19 @@ branch_prob (void)
if (dump_file)
fprintf (dump_file, "%d ignored edges\n", ignored_edges);
+
+ /* Compute two different checksums. Note that we want to compute
+ the checksum in only once place, since it depends on the shape
+ of the control flow which can change during
+ various transformations. */
+ cfg_checksum = coverage_compute_cfg_checksum ();
+ lineno_checksum = coverage_compute_lineno_checksum ();
+
/* Write the data from which gcov can reconstruct the basic block
graph. */
/* Basic block flags */
- if (coverage_begin_output ())
+ if (coverage_begin_output (lineno_checksum, cfg_checksum))
{
gcov_position_t offset;
@@ -1080,7 +1091,7 @@ branch_prob (void)
EXIT_BLOCK_PTR->index = last_basic_block;
/* Arcs */
- if (coverage_begin_output ())
+ if (coverage_begin_output (lineno_checksum, cfg_checksum))
{
gcov_position_t offset;
@@ -1121,7 +1132,7 @@ branch_prob (void)
}
/* Line numbers. */
- if (coverage_begin_output ())
+ if (coverage_begin_output (lineno_checksum, cfg_checksum))
{
/* Initialize the output. */
output_location (NULL, 0, NULL, NULL);
@@ -1176,9 +1187,9 @@ branch_prob (void)
if (flag_branch_probabilities)
{
- compute_branch_probabilities ();
+ compute_branch_probabilities (cfg_checksum, lineno_checksum);
if (flag_profile_values)
- compute_value_histograms (values);
+ compute_value_histograms (values, cfg_checksum, lineno_checksum);
}
remove_fake_edges ();
@@ -1206,7 +1217,7 @@ branch_prob (void)
VEC_free (histogram_value, heap, values);
free_edge_list (el);
- coverage_end_function ();
+ coverage_end_function (lineno_checksum, cfg_checksum);
}
\f
/* Union find algorithm implementation for the basic blocks using
@@ -1373,4 +1384,3 @@ end_branch_prob (void)
}
}
}
-
Index: coverage.c
===================================================================
--- coverage.c (revision 172693)
+++ coverage.c (working copy)
@@ -51,13 +51,17 @@ along with GCC; see the file COPYING3.
#include "intl.h"
#include "filenames.h"
+#include "gcov-io.h"
#include "gcov-io.c"
struct function_list
{
struct function_list *next; /* next function */
unsigned ident; /* function ident */
- unsigned checksum; /* function checksum */
+ unsigned lineno_checksum; /* function lineno checksum */
+ unsigned cfg_checksum; /* function cfg checksum */
+ const char *name; /* function name */
+ tree decl; /* function decl */
unsigned n_ctrs[GCOV_COUNTERS];/* number of counters. */
};
@@ -67,9 +71,11 @@ typedef struct counts_entry
/* We hash by */
unsigned ident;
unsigned ctr;
+ char *name;
/* Store */
- unsigned checksum;
+ unsigned lineno_checksum;
+ unsigned cfg_checksum;
gcov_type *counts;
struct gcov_ctr_summary summary;
@@ -114,14 +120,13 @@ static hashval_t htab_counts_entry_hash
static int htab_counts_entry_eq (const void *, const void *);
static void htab_counts_entry_del (void *);
static void read_counts_file (void);
-static unsigned compute_checksum (void);
-static unsigned coverage_checksum_string (unsigned, const char *);
static tree build_fn_info_type (unsigned);
static tree build_fn_info_value (const struct function_list *, tree);
static tree build_ctr_info_type (void);
static tree build_ctr_info_value (unsigned, tree);
static tree build_gcov_info (void);
static void create_coverage (void);
+static char * xstrdup_mask_random (const char *);
\f
/* Return the type node for gcov_type. */
@@ -139,12 +144,21 @@ get_gcov_unsigned_t (void)
return lang_hooks.types.type_for_size (32, true);
}
\f
+/* Return the type node for const char *. */
+
+static tree
+get_const_string_type (void)
+{
+ return build_pointer_type
+ (build_qualified_type (char_type_node, TYPE_QUAL_CONST));
+}
+\f
static hashval_t
htab_counts_entry_hash (const void *of)
{
const counts_entry_t *const entry = (const counts_entry_t *) of;
- return entry->ident * GCOV_COUNTERS + entry->ctr;
+ return htab_hash_string (entry->name) + entry->ctr;
}
static int
@@ -153,7 +167,8 @@ htab_counts_entry_eq (const void *of1, c
const counts_entry_t *const entry1 = (const counts_entry_t *) of1;
const counts_entry_t *const entry2 = (const counts_entry_t *) of2;
- return entry1->ident == entry2->ident && entry1->ctr == entry2->ctr;
+ return (entry1->ctr == entry2->ctr
+ && strcmp (entry1->name, entry2->name) == 0);
}
static void
@@ -161,6 +176,7 @@ htab_counts_entry_del (void *of)
{
counts_entry_t *const entry = (counts_entry_t *) of;
+ free (entry->name);
free (entry->counts);
free (entry);
}
@@ -171,11 +187,13 @@ static void
read_counts_file (void)
{
gcov_unsigned_t fn_ident = 0;
- gcov_unsigned_t checksum = -1;
+ char *name = NULL;
counts_entry_t *summaried = NULL;
unsigned seen_summary = 0;
gcov_unsigned_t tag;
int is_error = 0;
+ unsigned lineno_checksum = 0;
+ unsigned cfg_checksum = 0;
if (!gcov_open (da_file_name, 1))
return;
@@ -214,8 +232,21 @@ read_counts_file (void)
offset = gcov_position ();
if (tag == GCOV_TAG_FUNCTION)
{
+ const char *nm;
fn_ident = gcov_read_unsigned ();
- checksum = gcov_read_unsigned ();
+ lineno_checksum = gcov_read_unsigned ();
+ cfg_checksum = gcov_read_unsigned ();
+ nm = gcov_read_string();
+ if (!nm)
+ {
+ /* Error. Die! */
+ fatal_error ("corrupted profile - name of the function"
+ " (ident = 0x%x lineno_checksum 0x%x "
+ "ccfg_checksum 0x%x)"
+ " is NULL.", fn_ident, lineno_checksum,
+ cfg_checksum);
+ }
+ name = xstrdup (nm);
if (seen_summary)
{
/* We have already seen a summary, this means that this
@@ -257,6 +288,7 @@ read_counts_file (void)
unsigned ix;
elt.ident = fn_ident;
+ elt.name = name;
elt.ctr = GCOV_COUNTER_FOR_TAG (tag);
slot = (counts_entry_t **) htab_find_slot
@@ -265,17 +297,22 @@ read_counts_file (void)
if (!entry)
{
*slot = entry = XCNEW (counts_entry_t);
- entry->ident = elt.ident;
+ entry->ident = fn_ident;
+ entry->name = name;
entry->ctr = elt.ctr;
- entry->checksum = checksum;
+ entry->lineno_checksum = lineno_checksum;
+ entry->cfg_checksum = cfg_checksum;
entry->summary.num = n_counts;
entry->counts = XCNEWVEC (gcov_type, n_counts);
}
- else if (entry->checksum != checksum)
+ else if (entry->lineno_checksum != lineno_checksum
+ || entry->cfg_checksum != cfg_checksum)
{
error ("coverage mismatch for function %u while reading execution counters",
fn_ident);
- error ("checksum is %x instead of %x", entry->checksum, checksum);
+ error ("checksum is (%x,%x) instead of (%x,%x)",
+ entry->lineno_checksum, entry->cfg_checksum,
+ lineno_checksum, cfg_checksum);
htab_delete (counts_hash);
break;
}
@@ -324,10 +361,10 @@ read_counts_file (void)
gcov_type *
get_coverage_counts (unsigned counter, unsigned expected,
+ unsigned cfg_checksum, unsigned lineno_checksum,
const struct gcov_ctr_summary **summary)
{
counts_entry_t *entry, elt;
- gcov_unsigned_t checksum = -1;
/* No hash table, no counts. */
if (!counts_hash)
@@ -343,6 +380,8 @@ get_coverage_counts (unsigned counter, u
}
elt.ident = current_function_funcdef_no + 1;
+ elt.name = xstrdup_mask_random
+ (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (current_function_decl)));
elt.ctr = counter;
entry = (counts_entry_t *) htab_find (counts_hash, &elt);
if (!entry)
@@ -352,23 +391,22 @@ get_coverage_counts (unsigned counter, u
return NULL;
}
- checksum = compute_checksum ();
- if (entry->checksum != checksum
+ if (entry->cfg_checksum != cfg_checksum
|| entry->summary.num != expected)
{
static int warned = 0;
bool warning_printed = false;
tree id = DECL_ASSEMBLER_NAME (current_function_decl);
- warning_printed =
- warning_at (input_location, OPT_Wcoverage_mismatch,
+ warning_printed =
+ warning_at (input_location, OPT_Wcoverage_mismatch,
"coverage mismatch for function "
"%qE while reading counter %qs", id, ctr_names[counter]);
if (warning_printed)
{
- if (entry->checksum != checksum)
- inform (input_location, "checksum is %x instead of %x",
- entry->checksum, checksum);
+ if (entry->cfg_checksum != cfg_checksum)
+ inform (input_location, "cfg_checksum is %x instead of %x",
+ entry->cfg_checksum, cfg_checksum);
else
inform (input_location, "number of counters is %d instead of %d",
entry->summary.num, expected);
@@ -388,6 +426,12 @@ get_coverage_counts (unsigned counter, u
return NULL;
}
+ else if (entry->lineno_checksum != lineno_checksum)
+ {
+ warning (0, "Source location for function %qE have changed,"
+ " the profile data may be out of date",
+ DECL_ASSEMBLER_NAME (current_function_decl));
+ }
if (summary)
*summary = &entry->summary;
@@ -467,79 +511,125 @@ tree_coverage_counter_addr (unsigned cou
NULL, NULL));
}
\f
-/* Generate a checksum for a string. CHKSUM is the current
- checksum. */
-
-static unsigned
-coverage_checksum_string (unsigned chksum, const char *string)
+/* Mask out the random part of the identifier name.
+ Returns a pointer to the newly allocated string
+ that contains random part masked out to 0. */
+
+static char *
+xstrdup_mask_random (const char *string)
{
- int i;
- char *dup = NULL;
+ char *dup = xstrdup (string);
+ char *ptr = dup;
/* Look for everything that looks if it were produced by
get_file_function_name and zero out the second part
- that may result from flag_random_seed. This is not critical
- as the checksums are used only for sanity checking. */
- for (i = 0; string[i]; i++)
- {
- int offset = 0;
- if (!strncmp (string + i, "_GLOBAL__N_", 11))
- offset = 11;
- if (!strncmp (string + i, "_GLOBAL__", 9))
- offset = 9;
-
- /* C++ namespaces do have scheme:
- _GLOBAL__N_<filename>_<wrongmagicnumber>_<magicnumber>functionname
- since filename might contain extra underscores there seems
- to be no better chance then walk all possible offsets looking
- for magicnumber. */
- if (offset)
- {
- for (i = i + offset; string[i]; i++)
- if (string[i]=='_')
- {
- int y;
-
- for (y = 1; y < 9; y++)
- if (!(string[i + y] >= '0' && string[i + y] <= '9')
- && !(string[i + y] >= 'A' && string[i + y] <= 'F'))
- break;
- if (y != 9 || string[i + 9] != '_')
- continue;
- for (y = 10; y < 18; y++)
- if (!(string[i + y] >= '0' && string[i + y] <= '9')
- && !(string[i + y] >= 'A' && string[i + y] <= 'F'))
- break;
- if (y != 18)
- continue;
- if (!dup)
- string = dup = xstrdup (string);
- for (y = 10; y < 18; y++)
- dup[i + y] = '0';
- }
- break;
- }
+ that may result from flag_random_seed. This is critical
+ since we use the function name as the key for finding
+ the profile entry. */
+#define GLOBAL_PREFIX "_GLOBAL__"
+#define TRAILING_N "N_"
+#define ISCAPXDIGIT(a) (((a) >= '0' && (a) <= '9') || ((a) >= 'A' && (a) <= 'F'))
+ /* Thanks to anonymous namespace, we can have a function name
+ with multiple GLOBAL_PREFIX. */
+ while ((ptr = strstr (ptr, GLOBAL_PREFIX)))
+ {
+ char *temp_ptr;
+ /* Skip _GLOBAL__. */
+ ptr += strlen (GLOBAL_PREFIX);
+
+ /* Skip optional N_ (in case __GLOBAL_N__). */
+ if (!strncmp (ptr, TRAILING_N, strlen (TRAILING_N)))
+ ptr += strlen (TRAILING_N);
+ /* At this point, ptr should point after "_GLOBAL__N_" or "_GLOBAL__". */
+
+ while ((temp_ptr = strchr (ptr, '_')) != NULL)
+ {
+ int y;
+
+ ptr = temp_ptr;
+ /* For every "_" in the rest of the string,
+ try the follwing pattern matching */
+
+ /* Skip over '_'. */
+ ptr++;
+#define NDIGITS (8)
+ /* Try matching the pattern:
+ <8-digit hex>_<8-digit hex>
+ The second number is randomly generated
+ so we want to mask it out before computing the checksum. */
+ for (y = 0; *ptr != 0 && y < NDIGITS; y++, ptr++)
+ if (!ISCAPXDIGIT (*ptr))
+ break;
+ if (y != NDIGITS || *ptr != '_')
+ continue;
+ /* Skip over '_' again. */
+ ptr++;
+ for (y = 0; *ptr != 0 && y < NDIGITS; y++, ptr++)
+ if (!ISCAPXDIGIT (*ptr))
+ break;
+
+ if (y == NDIGITS)
+ {
+ /* We have a match.
+ Mask out the second 8-digit number. */
+ for(y = -NDIGITS - 1 ; y < 0; y++)
+ ptr[y] = '0';
+ break;
+ }
+ }
}
- chksum = crc32_string (chksum, string);
- if (dup)
- free (dup);
-
- return chksum;
+ return dup;
}
-/* Compute checksum for the current function. We generate a CRC32. */
-static unsigned
-compute_checksum (void)
+/* Compute checksum for the current function. We generate a CRC32.
+ TODO: This checksum can be made more stringent by computing
+ crc32 over the filename/lineno output in gcno. */
+
+unsigned
+coverage_compute_lineno_checksum (void)
{
expanded_location xloc
= expand_location (DECL_SOURCE_LOCATION (current_function_decl));
+ char *name = xstrdup_mask_random
+ (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (current_function_decl)));
unsigned chksum = xloc.line;
- chksum = coverage_checksum_string (chksum, xloc.file);
- chksum = coverage_checksum_string
- (chksum, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (current_function_decl)));
+ chksum = crc32_string (chksum, xloc.file);
+ chksum = crc32_string (chksum, name);
+
+ free (name);
+
+ return chksum;
+}
+
+
+/* Compute cfg checksum for the current function.
+ The checksum is calculated carefully so that
+ source code changes that doesn't affect the control flow graph
+ won't change the checksum.
+ This is to make the profile data useable across source code change.
+ The downside of this is that the compiler may use potentially
+ wrong profile data - that the source code change has non-trivial impact
+ on the validity of profile data (e.g. the reversed condition)
+ but the compiler won't detect the change and use the wrong profile data. */
+
+unsigned
+coverage_compute_cfg_checksum (void)
+{
+ basic_block bb;
+ unsigned chksum = n_basic_blocks;
+
+ FOR_EACH_BB (bb)
+ {
+ edge e;
+ edge_iterator ei;
+ FOR_EACH_EDGE (e, ei, bb->succs)
+ {
+ chksum = crc32_byte (chksum, e->dest->index);
+ }
+ }
return chksum;
}
@@ -550,7 +640,7 @@ compute_checksum (void)
should be output. */
int
-coverage_begin_output (void)
+coverage_begin_output (unsigned lineno_checksum, unsigned cfg_checksum)
{
/* We don't need to output .gcno file unless we're under -ftest-coverage
(e.g. -fprofile-arcs/generate/use don't need .gcno to work). */
@@ -562,6 +652,7 @@ coverage_begin_output (void)
expanded_location xloc
= expand_location (DECL_SOURCE_LOCATION (current_function_decl));
unsigned long offset;
+ char *name;
if (!bbg_file_opened)
{
@@ -576,17 +667,22 @@ coverage_begin_output (void)
bbg_file_opened = 1;
}
+ name = xstrdup_mask_random
+ (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (current_function_decl)));
+
/* Announce function */
offset = gcov_write_tag (GCOV_TAG_FUNCTION);
gcov_write_unsigned (current_function_funcdef_no + 1);
- gcov_write_unsigned (compute_checksum ());
- gcov_write_string (IDENTIFIER_POINTER
- (DECL_ASSEMBLER_NAME (current_function_decl)));
+ gcov_write_unsigned (lineno_checksum);
+ gcov_write_unsigned (cfg_checksum);
+ gcov_write_string (name);
gcov_write_string (xloc.file);
gcov_write_unsigned (xloc.line);
gcov_write_length (offset);
bbg_function_announced = 1;
+
+ free (name);
}
return !gcov_is_error ();
}
@@ -595,7 +691,7 @@ coverage_begin_output (void)
error has occurred. Save function coverage counts. */
void
-coverage_end_function (void)
+coverage_end_function (unsigned lineno_checksum, unsigned cfg_checksum)
{
unsigned i;
@@ -608,15 +704,22 @@ coverage_end_function (void)
if (fn_ctr_mask)
{
struct function_list *item;
+ const char *name;
item = XNEW (struct function_list);
*functions_tail = item;
functions_tail = &item->next;
+ name = xstrdup_mask_random
+ (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (current_function_decl)));
+
item->next = 0;
item->ident = current_function_funcdef_no + 1;
- item->checksum = compute_checksum ();
+ item->decl = current_function_decl;
+ item->name = name;
+ item->lineno_checksum = lineno_checksum;
+ item->cfg_checksum = cfg_checksum;
for (i = 0; i != GCOV_COUNTERS; i++)
{
item->n_ctrs[i] = fn_n_ctrs[i];
@@ -641,13 +744,24 @@ build_fn_info_type (unsigned int counter
/* ident */
fields = build_decl (BUILTINS_LOCATION,
FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
-
- /* checksum */
+ /* lineno_checksum */
field = build_decl (BUILTINS_LOCATION,
FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
DECL_CHAIN (field) = fields;
fields = field;
+ /* cfg checksum */
+ field = build_decl (BUILTINS_LOCATION,
+ FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
+ DECL_CHAIN (field) = fields;
+ fields = field;
+
+ /* name */
+ field = build_decl (BUILTINS_LOCATION,
+ FIELD_DECL, NULL_TREE, get_const_string_type ());
+ DECL_CHAIN (field) = fields;
+ fields = field;
+
array_type = build_int_cst (NULL_TREE, counters - 1);
array_type = build_index_type (array_type);
array_type = build_array_type (get_gcov_unsigned_t (), array_type);
@@ -681,10 +795,22 @@ build_fn_info_value (const struct functi
function->ident));
fields = DECL_CHAIN (fields);
- /* checksum */
+ /* lineno_checksum */
+ CONSTRUCTOR_APPEND_ELT (v1, fields,
+ build_int_cstu (get_gcov_unsigned_t (),
+ function->lineno_checksum));
+ fields = DECL_CHAIN (fields);
+
+ /* cfg_checksum */
CONSTRUCTOR_APPEND_ELT (v1, fields,
build_int_cstu (get_gcov_unsigned_t (),
- function->checksum));
+ function->cfg_checksum));
+ fields = DECL_CHAIN (fields);
+
+ /* name */
+ CONSTRUCTOR_APPEND_ELT (v1, fields,
+ build_string_literal (strlen (function->name) + 1,
+ function->name));
fields = DECL_CHAIN (fields);
/* counters */
Index: coverage.h
===================================================================
--- coverage.h (revision 172693)
+++ coverage.h (working copy)
@@ -28,11 +28,17 @@ extern void coverage_finish (void);
/* Complete the coverage information for the current function. Once
per function. */
-extern void coverage_end_function (void);
+extern void coverage_end_function (unsigned, unsigned);
/* Start outputting coverage information for the current
function. Repeatable per function. */
-extern int coverage_begin_output (void);
+extern int coverage_begin_output (unsigned, unsigned);
+
+/* Compute the control flow checksum for the current function. */
+extern unsigned coverage_compute_cfg_checksum (void);
+
+/* Compute the line number checksum for the current function. */
+extern unsigned coverage_compute_lineno_checksum (void);
/* Allocate some counters. Repeatable per function. */
extern int coverage_counter_alloc (unsigned /*counter*/, unsigned/*num*/);
@@ -44,6 +50,8 @@ extern tree tree_coverage_counter_addr (
/* Get all the counters for the current function. */
extern gcov_type *get_coverage_counts (unsigned /*counter*/,
unsigned /*expected*/,
+ unsigned /*cfg_checksum*/,
+ unsigned /*lineno_checksum*/,
const struct gcov_ctr_summary **);
extern tree get_gcov_type (void);
Index: libgcov.c
===================================================================
--- libgcov.c (revision 172693)
+++ libgcov.c (working copy)
@@ -372,9 +372,10 @@ gcov_exit (void)
/* Check function. */
if (tag != GCOV_TAG_FUNCTION
- || length != GCOV_TAG_FUNCTION_LENGTH
|| gcov_read_unsigned () != fi_ptr->ident
- || gcov_read_unsigned () != fi_ptr->checksum)
+ || gcov_read_unsigned () != fi_ptr->lineno_checksum
+ || gcov_read_unsigned () != fi_ptr->cfg_checksum
+ || strcmp (gcov_read_string (), fi_ptr->name))
{
read_mismatch:;
fprintf (stderr, "profiling:%s:Merge mismatch for %s\n",
@@ -515,9 +516,13 @@ gcov_exit (void)
((const char *) gi_ptr->functions + f_ix * fi_stride);
/* Announce function. */
- gcov_write_tag_length (GCOV_TAG_FUNCTION, GCOV_TAG_FUNCTION_LENGTH);
+ gcov_write_tag_length
+ (GCOV_TAG_FUNCTION,
+ GCOV_TAG_FUNCTION_LENGTH + gcov_string_length (fi_ptr->name));
gcov_write_unsigned (fi_ptr->ident);
- gcov_write_unsigned (fi_ptr->checksum);
+ gcov_write_unsigned (fi_ptr->lineno_checksum);
+ gcov_write_unsigned (fi_ptr->cfg_checksum);
+ gcov_write_string (fi_ptr->name);
c_ix = 0;
for (t_ix = 0; t_ix < GCOV_COUNTERS; t_ix++)
Index: gcov-dump.c
===================================================================
--- gcov-dump.c (revision 172693)
+++ gcov-dump.c (working copy)
@@ -265,21 +265,18 @@ tag_function (const char *filename ATTRI
unsigned tag ATTRIBUTE_UNUSED, unsigned length ATTRIBUTE_UNUSED)
{
unsigned long pos = gcov_position ();
+ const char *name;
printf (" ident=%u", gcov_read_unsigned ());
- printf (", checksum=0x%08x", gcov_read_unsigned ());
+ printf (", lineno_checksum=0x%08x", gcov_read_unsigned ());
+ printf (", cfg_checksum=0x%08x", gcov_read_unsigned ());
- if (gcov_position () - pos < length)
- {
- const char *name;
-
- name = gcov_read_string ();
- printf (", `%s'", name ? name : "NULL");
name = gcov_read_string ();
printf (" %s", name ? name : "NULL");
+
+ if (gcov_position () - pos < length)
printf (":%u", gcov_read_unsigned ());
}
-}
static void
tag_blocks (const char *filename ATTRIBUTE_UNUSED,
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-19 20:37 ` Xinliang David Li
@ 2011-04-19 22:36 ` Jan Hubicka
2011-04-21 8:30 ` Xinliang David Li
2011-04-21 18:23 ` Diego Novillo
1 sibling, 1 reply; 19+ messages in thread
From: Jan Hubicka @ 2011-04-19 22:36 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Jan Hubicka, GCC Patches
I can not review tree.c changes. I would probably suggest making crc_byte inline.
> +#if IN_LIBGCOV
> +
> +/* These functions are guarded by #if to avoid compile time warning. */
> +
> +/* Return the number of words STRING would need including the length
> + field in the output stream itself. This should be identical to
> + "alloc" calculation in gcov_write_string(). */
Hmm, probably better to make gcov_write_string to use gcov_string_length then.
> @@ -238,13 +265,15 @@ gcov_write_words (unsigned words)
>
> gcc_assert (gcov_var.mode < 0);
> #if IN_LIBGCOV
> - if (gcov_var.offset >= GCOV_BLOCK_SIZE)
> + if (gcov_var.offset + words >= GCOV_BLOCK_SIZE)
> {
> - gcov_write_block (GCOV_BLOCK_SIZE);
> + gcov_write_block (MIN (gcov_var.offset, GCOV_BLOCK_SIZE));
> if (gcov_var.offset)
> {
> - gcc_assert (gcov_var.offset == 1);
> - memcpy (gcov_var.buffer, gcov_var.buffer + GCOV_BLOCK_SIZE, 4);
> + gcc_assert (gcov_var.offset < GCOV_BLOCK_SIZE);
> + memcpy (gcov_var.buffer,
> + gcov_var.buffer + GCOV_BLOCK_SIZE,
> + gcov_var.offset << 2);
I don't really follow the logic here. buffer is allocated to be size of
block+4 and it is expected that gcov_write_words is not executed on size
greater than 4. Since gcov_write_string now seems to be expected to handle
strings of bigger size, I think you acually need to make write_string to write
in chunks when you reach block boundary?
As soon as you decide to not write out in the GCOV_BLOCK_SIZE chunks, the
MIN computation above seems unnecesary (and bogus, since we won't let
gcov_var.offset to grow past GCOV_BLOCK_SIZE.
What gets into libgcov is very problematic busyness for embedded targets, where you really want
libgcov to be small. Why do you need to store strings now?
> Index: gcov-io.h
> ===================================================================
> --- gcov-io.h (revision 172693)
> +++ gcov-io.h (working copy)
> @@ -101,9 +101,10 @@ see the files COPYING3 and COPYING.RUNTI
>
> The basic block graph file contains the following records
> note: unit function-graph*
> - unit: header int32:checksum string:source
> + unit: header int32:checksum int32: string:source
> function-graph: announce_function basic_blocks {arcs | lines}*
> - announce_function: header int32:ident int32:checksum
> + announce_function: header int32:ident
> + int32:lineno_checksum int32:cfg_checksum
> string:name string:source int32:lineno
> basic_block: header int32:flags*
> arcs: header int32:block_no arc*
> @@ -132,7 +133,9 @@ see the files COPYING3 and COPYING.RUNTI
> data: {unit function-data* summary:object summary:program*}*
> unit: header int32:checksum
> function-data: announce_function arc_counts
> - announce_function: header int32:ident int32:checksum
> + announce_function: header int32:ident
> + int32:lineno_checksum int32:cfg_checksum
> + string:name
> arc_counts: header int64:count*
> summary: int32:checksum {count-summary}GCOV_COUNTERS
> count-summary: int32:num int32:runs int64:sum
We also need to bump gcov version, right?
> @@ -411,11 +417,20 @@ struct gcov_summary
> /* Information about a single function. This uses the trailing array
> idiom. The number of counters is determined from the counter_mask
> in gcov_info. We hold an array of function info, so have to
> - explicitly calculate the correct array stride. */
> + explicitly calculate the correct array stride.
> +
> + "ident" is no longer used in hash computation. Additionally,
> + "name" is used in hash computation. This makes the profile data
> + not compatible across function name changes.
> + Also, the function pointer is stored for later use for indirect
> + function call name resolution.
Hmm, your patch is not adding indirect function call name resolution,
so independent change?
> Index: profile.c
> ===================================================================
> --- profile.c (revision 172693)
> +++ profile.c (working copy)
> @@ -102,13 +102,6 @@ static int total_num_branches;
>
> /* Forward declarations. */
> static void find_spanning_tree (struct edge_list *);
> -static unsigned instrument_edges (struct edge_list *);
> -static void instrument_values (histogram_values);
> -static void compute_branch_probabilities (void);
> -static void compute_value_histograms (histogram_values);
> -static gcov_type * get_exec_counts (void);
> -static basic_block find_group (basic_block);
> -static void union_groups (basic_block, basic_block);
This is also independent and OK for mainline.
>
> /* Add edge instrumentation code to the entire insn chain.
>
> @@ -233,10 +226,12 @@ instrument_values (histogram_values valu
> }
> \f
>
> -/* Computes hybrid profile for all matching entries in da_file. */
> +/* Computes hybrid profile for all matching entries in da_file.
> +
> + CFG_CHECKSUM is the precomputed checksum for the CFG. */
document LINENO_CHECKSUM, too.
> struct function_list *next; /* next function */
> unsigned ident; /* function ident */
> - unsigned checksum; /* function checksum */
> + unsigned lineno_checksum; /* function lineno checksum */
> + unsigned cfg_checksum; /* function cfg checksum */
> + const char *name; /* function name */
> + tree decl; /* function decl */
> unsigned n_ctrs[GCOV_COUNTERS];/* number of counters. */
> };
>
> @@ -67,9 +71,11 @@ typedef struct counts_entry
> /* We hash by */
> unsigned ident;
> unsigned ctr;
> + char *name;
also const, right?
> @@ -139,12 +144,21 @@ get_gcov_unsigned_t (void)
> return lang_hooks.types.type_for_size (32, true);
> }
> \f
> +/* Return the type node for const char *. */
> +
> +static tree
> +get_const_string_type (void)
This is not really coverage specific and already mudflap is doing the same. So probably it better
should go into tree.c.
> - if (entry->checksum != checksum)
> - inform (input_location, "checksum is %x instead of %x",
> - entry->checksum, checksum);
> + if (entry->cfg_checksum != cfg_checksum)
> + inform (input_location, "cfg_checksum is %x instead of %x",
> + entry->cfg_checksum, cfg_checksum);
Well, the message is already cryptic, perhaps just expanding cfg to be control flow graph
so users get some clue.
> @@ -641,13 +744,24 @@ build_fn_info_type (unsigned int counter
> /* ident */
> fields = build_decl (BUILTINS_LOCATION,
> FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
> -
> - /* checksum */
> + /* lineno_checksum */
> field = build_decl (BUILTINS_LOCATION,
> FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
> DECL_CHAIN (field) = fields;
> fields = field;
>
> + /* cfg checksum */
> + field = build_decl (BUILTINS_LOCATION,
> + FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
> + DECL_CHAIN (field) = fields;
> + fields = field;
Why do we need function names to end up there at runtime?
Honza
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-19 22:36 ` Jan Hubicka
@ 2011-04-21 8:30 ` Xinliang David Li
2011-04-21 22:21 ` Jan Hubicka
0 siblings, 1 reply; 19+ messages in thread
From: Xinliang David Li @ 2011-04-21 8:30 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, Diego Novillo
On Tue, Apr 19, 2011 at 3:20 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> I can not review tree.c changes. I would probably suggest making crc_byte inline.
Diego, can you review this change? This is just a simple refactoring.
>
>> +#if IN_LIBGCOV
>> +
>> +/* These functions are guarded by #if to avoid compile time warning. */
>> +
>> +/* Return the number of words STRING would need including the length
>> + field in the output stream itself. This should be identical to
>> + "alloc" calculation in gcov_write_string(). */
>
> Hmm, probably better to make gcov_write_string to use gcov_string_length then.
yes.
>> @@ -238,13 +265,15 @@ gcov_write_words (unsigned words)
>>
>> gcc_assert (gcov_var.mode < 0);
>> #if IN_LIBGCOV
>> - if (gcov_var.offset >= GCOV_BLOCK_SIZE)
>> + if (gcov_var.offset + words >= GCOV_BLOCK_SIZE)
>> {
>> - gcov_write_block (GCOV_BLOCK_SIZE);
>> + gcov_write_block (MIN (gcov_var.offset, GCOV_BLOCK_SIZE));
>> if (gcov_var.offset)
>> {
>> - gcc_assert (gcov_var.offset == 1);
>> - memcpy (gcov_var.buffer, gcov_var.buffer + GCOV_BLOCK_SIZE, 4);
>> + gcc_assert (gcov_var.offset < GCOV_BLOCK_SIZE);
>> + memcpy (gcov_var.buffer,
>> + gcov_var.buffer + GCOV_BLOCK_SIZE,
>> + gcov_var.offset << 2);
>
> I don't really follow the logic here. buffer is allocated to be size of
> block+4 and it is expected that gcov_write_words is not executed on size
> greater than 4. Since gcov_write_string now seems to be expected to handle
> strings of bigger size, I think you acually need to make write_string to write
> in chunks when you reach block boundary?
gcov_write_words is used to reserve words*4 bytes in buffer for data
write later. The the old logic is wrong -- if 'words' is large enough,
it will lead to out of bound access.
>
> As soon as you decide to not write out in the GCOV_BLOCK_SIZE chunks, the
> MIN computation above seems unnecesary (and bogus, since we won't let
> gcov_var.offset to grow past GCOV_BLOCK_SIZE.
Yes, using gcov_var.offset should be good enough.
>
> What gets into libgcov is very problematic busyness for embedded targets, where you really want
> libgcov to be small. Why do you need to store strings now?
It is needed to store the assembler name for functions. It allows
lookup of the profile data using assembler name as key instead of
using function ids. For gcc, the total size of gcda files is about 59k
bytes without storing the names, and about 65k with names -- about 10%
increase. For eon, the size changes from 27k to 35k bytes.
I can split the patches into two parts -- one with cfg checksum and
one with the name string.
>> Index: gcov-io.h
>> ===================================================================
>> --- gcov-io.h (revision 172693)
>> +++ gcov-io.h (working copy)
>> @@ -101,9 +101,10 @@ see the files COPYING3 and COPYING.RUNTI
>>
>> The basic block graph file contains the following records
>> note: unit function-graph*
>> - unit: header int32:checksum string:source
>> + unit: header int32:checksum int32: string:source
>> function-graph: announce_function basic_blocks {arcs | lines}*
>> - announce_function: header int32:ident int32:checksum
>> + announce_function: header int32:ident
>> + int32:lineno_checksum int32:cfg_checksum
>> string:name string:source int32:lineno
>> basic_block: header int32:flags*
>> arcs: header int32:block_no arc*
>> @@ -132,7 +133,9 @@ see the files COPYING3 and COPYING.RUNTI
>> data: {unit function-data* summary:object summary:program*}*
>> unit: header int32:checksum
>> function-data: announce_function arc_counts
>> - announce_function: header int32:ident int32:checksum
>> + announce_function: header int32:ident
>> + int32:lineno_checksum int32:cfg_checksum
>> + string:name
>> arc_counts: header int64:count*
>> summary: int32:checksum {count-summary}GCOV_COUNTERS
>> count-summary: int32:num int32:runs int64:sum
>
> We also need to bump gcov version, right?
Yes -- but the version is currently derived from gcc version number
and phase number --- this is wrong -- different version of gcc may
have compatible coverage data format. Any suggestions to change this?
--- probably just hard code the GCOV_VERSION string?
>
>> @@ -411,11 +417,20 @@ struct gcov_summary
>> /* Information about a single function. This uses the trailing array
>> idiom. The number of counters is determined from the counter_mask
>> in gcov_info. We hold an array of function info, so have to
>> - explicitly calculate the correct array stride. */
>> + explicitly calculate the correct array stride.
>> +
>> + "ident" is no longer used in hash computation. Additionally,
>> + "name" is used in hash computation. This makes the profile data
>> + not compatible across function name changes.
>> + Also, the function pointer is stored for later use for indirect
>> + function call name resolution.
>
> Hmm, your patch is not adding indirect function call name resolution,
> so independent change?
Yes -- the comment on indirect function call name is for SampleFDO change.
>> @@ -67,9 +71,11 @@ typedef struct counts_entry
>> /* We hash by */
>> unsigned ident;
>> unsigned ctr;
>> + char *name;
> also const, right?
yes.
>> @@ -139,12 +144,21 @@ get_gcov_unsigned_t (void)
>> return lang_hooks.types.type_for_size (32, true);
>> }
>>
>> +/* Return the type node for const char *. */
>> +
>> +static tree
>> +get_const_string_type (void)
>
> This is not really coverage specific and already mudflap is doing the same. So probably it better
> should go into tree.c.
Ok.
>> - if (entry->checksum != checksum)
>> - inform (input_location, "checksum is %x instead of %x",
>> - entry->checksum, checksum);
>> + if (entry->cfg_checksum != cfg_checksum)
>> + inform (input_location, "cfg_checksum is %x instead of %x",
>> + entry->cfg_checksum, cfg_checksum);
>
> Well, the message is already cryptic, perhaps just expanding cfg to be control flow graph
> so users get some clue.
Ok.
>> @@ -641,13 +744,24 @@ build_fn_info_type (unsigned int counter
>> /* ident */
>> fields = build_decl (BUILTINS_LOCATION,
>> FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
>> -
>> - /* checksum */
>> + /* lineno_checksum */
>> field = build_decl (BUILTINS_LOCATION,
>> FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
>> DECL_CHAIN (field) = fields;
>> fields = field;
>>
>> + /* cfg checksum */
>> + field = build_decl (BUILTINS_LOCATION,
>> + FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
>> + DECL_CHAIN (field) = fields;
>> + fields = field;
>
> Why do we need function names to end up there at runtime?
They need to be stored into gcda files at the end of the profile collection.
Thanks,
David
>
> Honza
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-19 20:37 ` Xinliang David Li
2011-04-19 22:36 ` Jan Hubicka
@ 2011-04-21 18:23 ` Diego Novillo
1 sibling, 0 replies; 19+ messages in thread
From: Diego Novillo @ 2011-04-21 18:23 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Jan Hubicka, GCC Patches
On Tue, Apr 19, 2011 at 15:47, Xinliang David Li <davidxl@google.com> wrote:
> The attached is the revised patch with a warning suggested for cases
> when CFG matches, but source locations change.
>
> Ok for trunk?
The tree.c changes are OK.
Diego.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-21 8:30 ` Xinliang David Li
@ 2011-04-21 22:21 ` Jan Hubicka
2011-04-21 22:42 ` Xinliang David Li
0 siblings, 1 reply; 19+ messages in thread
From: Jan Hubicka @ 2011-04-21 22:21 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Jan Hubicka, GCC Patches, Diego Novillo
> > I don't really follow the logic here. Â buffer is allocated to be size of
> > block+4 and it is expected that gcov_write_words is not executed on size
> > greater than 4. Â Since gcov_write_string now seems to be expected to handle
> > strings of bigger size, I think you acually need to make write_string to write
> > in chunks when you reach block boundary?
>
> gcov_write_words is used to reserve words*4 bytes in buffer for data
> write later. The the old logic is wrong -- if 'words' is large enough,
> it will lead to out of bound access.
The old logic assumes that writes are not bigger than 4 and it is documented
somewhere in gcov-io.h if I remember right (it is not my code).
I however still think your change won't solve it in general, since you might
want to write more than block size, so you need write_string update.
> >
> > What gets into libgcov is very problematic busyness for embedded targets, where you really want
> > libgcov to be small. Â Why do you need to store strings now?
>
> It is needed to store the assembler name for functions. It allows
> lookup of the profile data using assembler name as key instead of
> using function ids. For gcc, the total size of gcda files is about 59k
> bytes without storing the names, and about 65k with names -- about 10%
> increase. For eon, the size changes from 27k to 35k bytes.
>
> I can split the patches into two parts -- one with cfg checksum and
> one with the name string.
Well, lets make it incremental change then at time we will have use for it once
we agree it makes sense. So you want to do symbol name resolving at GCOV
runtime?
I duno about embedded targets, 10% increase is not too bad, but I don't know.
libgcov has quite inflexible coding style just to be small and fit there, so
I don't know what are the space constraints that are considered acceptable
in that world.
> > We also need to bump gcov version, right?
>
> Yes -- but the version is currently derived from gcc version number
> and phase number --- this is wrong -- different version of gcc may
> have compatible coverage data format. Any suggestions to change this?
> --- probably just hard code the GCOV_VERSION string?
Hmm, I am pretty sure we used to have minor/major numbering on the file format.
Perhaps it went away with Nathan's changes. For -fprofile-use the compatibility
across versions is pointless, at least now, but for gcov utility it makes sense.
So I would go for explicit versioning again.
Honza
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-21 22:21 ` Jan Hubicka
@ 2011-04-21 22:42 ` Xinliang David Li
2011-04-22 19:35 ` Xinliang David Li
0 siblings, 1 reply; 19+ messages in thread
From: Xinliang David Li @ 2011-04-21 22:42 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, Diego Novillo
On Thu, Apr 21, 2011 at 1:43 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > I don't really follow the logic here. buffer is allocated to be size of
>> > block+4 and it is expected that gcov_write_words is not executed on size
>> > greater than 4. Since gcov_write_string now seems to be expected to handle
>> > strings of bigger size, I think you acually need to make write_string to write
>> > in chunks when you reach block boundary?
>>
>> gcov_write_words is used to reserve words*4 bytes in buffer for data
>> write later. The the old logic is wrong -- if 'words' is large enough,
>> it will lead to out of bound access.
>
> The old logic assumes that writes are not bigger than 4 and it is documented
> somewhere in gcov-io.h if I remember right (it is not my code).
> I however still think your change won't solve it in general, since you might
> want to write more than block size, so you need write_string update.
Right, 'words; can be large for string. -- the memcpy code following
does not look right either.
>> >
>> > What gets into libgcov is very problematic busyness for embedded targets, where you really want
>> > libgcov to be small. Why do you need to store strings now?
>>
>> It is needed to store the assembler name for functions. It allows
>> lookup of the profile data using assembler name as key instead of
>> using function ids. For gcc, the total size of gcda files is about 59k
>> bytes without storing the names, and about 65k with names -- about 10%
>> increase. For eon, the size changes from 27k to 35k bytes.
>>
>> I can split the patches into two parts -- one with cfg checksum and
>> one with the name string.
>
> Well, lets make it incremental change then at time we will have use for it once
> we agree it makes sense.
Ok.
> So you want to do symbol name resolving at GCOV
> runtime?
For now, it is just passed through as a tag for the profile data for
each function.
> I duno about embedded targets, 10% increase is not too bad, but I don't know.
> libgcov has quite inflexible coding style just to be small and fit there, so
> I don't know what are the space constraints that are considered acceptable
> in that world.
I will delay the string writing patch for now.
>> > We also need to bump gcov version, right?
>>
>> Yes -- but the version is currently derived from gcc version number
>> and phase number --- this is wrong -- different version of gcc may
>> have compatible coverage data format. Any suggestions to change this?
>> --- probably just hard code the GCOV_VERSION string?
>
> Hmm, I am pretty sure we used to have minor/major numbering on the file format.
> Perhaps it went away with Nathan's changes. For -fprofile-use the compatibility
> across versions is pointless,
Right, but also equally unnecessary to force version mismatch between
compiler releases.
>at least now, but for gcov utility it makes sense.
Yes.
> So I would go for explicit versioning again.
>
Will dig through the revision history ..
David
> Honza
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-21 22:42 ` Xinliang David Li
@ 2011-04-22 19:35 ` Xinliang David Li
2011-04-22 20:11 ` Jan Hubicka
2011-04-27 16:27 ` Jan Hubicka
0 siblings, 2 replies; 19+ messages in thread
From: Xinliang David Li @ 2011-04-22 19:35 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, Diego Novillo
[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]
Please review the new patch which only implements cfg checksum.
The auto version generation was introduced in 2002 before FDO support
was added (so the old way never existed), so it might be better to
make the change independent of this one.
Thanks,
David
On Thu, Apr 21, 2011 at 2:15 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Thu, Apr 21, 2011 at 1:43 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> > I don't really follow the logic here. buffer is allocated to be size of
>>> > block+4 and it is expected that gcov_write_words is not executed on size
>>> > greater than 4. Since gcov_write_string now seems to be expected to handle
>>> > strings of bigger size, I think you acually need to make write_string to write
>>> > in chunks when you reach block boundary?
>>>
>>> gcov_write_words is used to reserve words*4 bytes in buffer for data
>>> write later. The the old logic is wrong -- if 'words' is large enough,
>>> it will lead to out of bound access.
>>
>> The old logic assumes that writes are not bigger than 4 and it is documented
>> somewhere in gcov-io.h if I remember right (it is not my code).
>> I however still think your change won't solve it in general, since you might
>> want to write more than block size, so you need write_string update.
>
> Right, 'words; can be large for string. -- the memcpy code following
> does not look right either.
>
>
>>> >
>>> > What gets into libgcov is very problematic busyness for embedded targets, where you really want
>>> > libgcov to be small. Why do you need to store strings now?
>>>
>>> It is needed to store the assembler name for functions. It allows
>>> lookup of the profile data using assembler name as key instead of
>>> using function ids. For gcc, the total size of gcda files is about 59k
>>> bytes without storing the names, and about 65k with names -- about 10%
>>> increase. For eon, the size changes from 27k to 35k bytes.
>>>
>>> I can split the patches into two parts -- one with cfg checksum and
>>> one with the name string.
>>
>> Well, lets make it incremental change then at time we will have use for it once
>> we agree it makes sense.
>
> Ok.
>
>> So you want to do symbol name resolving at GCOV
>> runtime?
>
> For now, it is just passed through as a tag for the profile data for
> each function.
>
>> I duno about embedded targets, 10% increase is not too bad, but I don't know.
>> libgcov has quite inflexible coding style just to be small and fit there, so
>> I don't know what are the space constraints that are considered acceptable
>> in that world.
>
> I will delay the string writing patch for now.
>
>>> > We also need to bump gcov version, right?
>>>
>>> Yes -- but the version is currently derived from gcc version number
>>> and phase number --- this is wrong -- different version of gcc may
>>> have compatible coverage data format. Any suggestions to change this?
>>> --- probably just hard code the GCOV_VERSION string?
>>
>> Hmm, I am pretty sure we used to have minor/major numbering on the file format.
>> Perhaps it went away with Nathan's changes. For -fprofile-use the compatibility
>> across versions is pointless,
>
> Right, but also equally unnecessary to force version mismatch between
> compiler releases.
>
>>at least now, but for gcov utility it makes sense.
>
> Yes.
>
>> So I would go for explicit versioning again.
>>
>
> Will dig through the revision history ..
>
> David
>
>> Honza
>>
>
[-- Attachment #2: cfg_chksum3.p --]
[-- Type: text/x-pascal, Size: 24892 bytes --]
Index: tree.c
===================================================================
--- tree.c (revision 172802)
+++ tree.c (working copy)
@@ -8513,14 +8513,12 @@ dump_tree_statistics (void)
\f
#define FILE_FUNCTION_FORMAT "_GLOBAL__%s_%s"
-/* Generate a crc32 of a string. */
+/* Generate a crc32 of a byte. */
unsigned
-crc32_string (unsigned chksum, const char *string)
+crc32_byte (unsigned chksum, char byte)
{
- do
- {
- unsigned value = *string << 24;
+ unsigned value = (unsigned) byte << 24;
unsigned ix;
for (ix = 8; ix--; value <<= 1)
@@ -8531,6 +8529,18 @@ crc32_string (unsigned chksum, const cha
chksum <<= 1;
chksum ^= feedback;
}
+ return chksum;
+}
+
+
+/* Generate a crc32 of a string. */
+
+unsigned
+crc32_string (unsigned chksum, const char *string)
+{
+ do
+ {
+ chksum = crc32_byte (chksum, *string);
}
while (*string++);
return chksum;
@@ -8554,8 +8564,10 @@ clean_symbol_name (char *p)
*p = '_';
}
-/* Generate a name for a special-purpose function function.
+/* Generate a name for a special-purpose function.
The generated name may need to be unique across the whole link.
+ Changes to this function may also require corresponding changes to
+ xstrdup_mask_random.
TYPE is some string to identify the purpose of this function to the
linker or collect2; it must start with an uppercase letter,
one of:
Index: tree.h
===================================================================
--- tree.h (revision 172802)
+++ tree.h (working copy)
@@ -4998,6 +4998,7 @@ inlined_function_outer_scope_p (const_tr
\f
/* In tree.c */
extern unsigned crc32_string (unsigned, const char *);
+extern unsigned crc32_byte (unsigned, char);
extern void clean_symbol_name (char *);
extern tree get_file_function_name (const char *);
extern tree get_callee_fndecl (const_tree);
Index: gcov.c
===================================================================
--- gcov.c (revision 172802)
+++ gcov.c (working copy)
@@ -54,6 +54,13 @@ along with Gcov; see the file COPYING3.
some places we make use of the knowledge of how profile.c works to
select particular algorithms here. */
+/* The code validates that the profile information read in corresponds
+ to the code currently being compiled. Rather than checking for
+ identical files, the code below computes a checksum on the CFG
+ (based on the order of basic blocks and the arcs in the CFG). If
+ the CFG checksum in the gcda file match the CFG checksum for the
+ code currently being compiled, the profile data will be used. */
+
/* This is the size of the buffer used to read in source file lines. */
#define STRING_SIZE 200
@@ -161,7 +168,8 @@ typedef struct function_info
/* Name of function. */
char *name;
unsigned ident;
- unsigned checksum;
+ unsigned lineno_checksum;
+ unsigned cfg_checksum;
/* Array of basic blocks. */
block_t *blocks;
@@ -807,12 +815,14 @@ read_graph_file (void)
if (tag == GCOV_TAG_FUNCTION)
{
char *function_name;
- unsigned ident, checksum, lineno;
+ unsigned ident, lineno;
+ unsigned lineno_checksum, cfg_checksum;
source_t *src;
function_t *probe, *prev;
ident = gcov_read_unsigned ();
- checksum = gcov_read_unsigned ();
+ lineno_checksum = gcov_read_unsigned ();
+ cfg_checksum = gcov_read_unsigned ();
function_name = xstrdup (gcov_read_string ());
src = find_source (gcov_read_string ());
lineno = gcov_read_unsigned ();
@@ -820,7 +830,8 @@ read_graph_file (void)
fn = XCNEW (function_t);
fn->name = function_name;
fn->ident = ident;
- fn->checksum = checksum;
+ fn->lineno_checksum = lineno_checksum;
+ fn->cfg_checksum = cfg_checksum;
fn->src = src;
fn->line = lineno;
@@ -1107,7 +1118,8 @@ read_count_file (void)
if (!fn)
;
- else if (gcov_read_unsigned () != fn->checksum)
+ else if (gcov_read_unsigned () != fn->lineno_checksum
+ || gcov_read_unsigned () != fn->cfg_checksum)
{
mismatch:;
fnotice (stderr, "%s:profile mismatch for '%s'\n",
Index: testsuite/gcc.dg/tree-prof/prof-robust-1.c
===================================================================
--- testsuite/gcc.dg/tree-prof/prof-robust-1.c (revision 0)
+++ testsuite/gcc.dg/tree-prof/prof-robust-1.c (revision 0)
@@ -0,0 +1,25 @@
+/* { dg-options "-O2 -w" } */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#ifdef _PROFILE_USE
+int foo(int x) {
+ return 3 * x;
+}
+#else
+int foo(int x) {
+ return 3 * x;
+}
+#endif
+
+int x = 1000;
+
+int main(int argc, char *argv[]) {
+ int i;
+ int sum = 0;
+ for (i = 0; i < x; i++)
+ sum += i;
+ printf("%d\n", sum);
+ return 0;
+}
Index: testsuite/g++.dg/prof-robust-1.C
===================================================================
--- testsuite/g++.dg/prof-robust-1.C (revision 0)
+++ testsuite/g++.dg/prof-robust-1.C (revision 0)
@@ -0,0 +1,24 @@
+/* { dg-options "-O2 -fno-weak" } */
+
+#include <stdio.h>
+
+namespace {
+ namespace {
+
+ class MyClass {
+ public:
+ void foo() const;
+ ~MyClass() { foo(); }
+ };
+
+ void MyClass::foo() const { printf("Goodbye World\n"); }
+
+ }
+
+ static MyClass variable;
+
+}
+
+int main() {
+ return 0;
+}
Index: gcov-io.h
===================================================================
--- gcov-io.h (revision 172802)
+++ gcov-io.h (working copy)
@@ -103,7 +103,8 @@ see the files COPYING3 and COPYING.RUNTI
note: unit function-graph*
unit: header int32:checksum string:source
function-graph: announce_function basic_blocks {arcs | lines}*
- announce_function: header int32:ident int32:checksum
+ announce_function: header int32:ident
+ int32:lineno_checksum int32:cfg_checksum
string:name string:source int32:lineno
basic_block: header int32:flags*
arcs: header int32:block_no arc*
@@ -132,7 +133,8 @@ see the files COPYING3 and COPYING.RUNTI
data: {unit function-data* summary:object summary:program*}*
unit: header int32:checksum
function-data: announce_function arc_counts
- announce_function: header int32:ident int32:checksum
+ announce_function: header int32:ident
+ int32:lineno_checksum int32:cfg_checksum
arc_counts: header int64:count*
summary: int32:checksum {count-summary}GCOV_COUNTERS
count-summary: int32:num int32:runs int64:sum
@@ -294,7 +296,7 @@ typedef HOST_WIDEST_INT gcov_type;
file marker -- it is not required to be present. */
#define GCOV_TAG_FUNCTION ((gcov_unsigned_t)0x01000000)
-#define GCOV_TAG_FUNCTION_LENGTH (2)
+#define GCOV_TAG_FUNCTION_LENGTH (3)
#define GCOV_TAG_BLOCKS ((gcov_unsigned_t)0x01410000)
#define GCOV_TAG_BLOCKS_LENGTH(NUM) (NUM)
#define GCOV_TAG_BLOCKS_NUM(LENGTH) (LENGTH)
@@ -412,10 +414,12 @@ struct gcov_summary
idiom. The number of counters is determined from the counter_mask
in gcov_info. We hold an array of function info, so have to
explicitly calculate the correct array stride. */
+
struct gcov_fn_info
{
gcov_unsigned_t ident; /* unique ident of function */
- gcov_unsigned_t checksum; /* function checksum */
+ gcov_unsigned_t lineno_checksum; /* function lineo_checksum */
+ gcov_unsigned_t cfg_checksum; /* function cfg checksum */
unsigned n_ctrs[0]; /* instrumented counters */
};
Index: profile.c
===================================================================
--- profile.c (revision 172802)
+++ profile.c (working copy)
@@ -102,13 +102,6 @@ static int total_num_branches;
/* Forward declarations. */
static void find_spanning_tree (struct edge_list *);
-static unsigned instrument_edges (struct edge_list *);
-static void instrument_values (histogram_values);
-static void compute_branch_probabilities (void);
-static void compute_value_histograms (histogram_values);
-static gcov_type * get_exec_counts (void);
-static basic_block find_group (basic_block);
-static void union_groups (basic_block, basic_block);
/* Add edge instrumentation code to the entire insn chain.
@@ -233,10 +226,12 @@ instrument_values (histogram_values valu
}
\f
-/* Computes hybrid profile for all matching entries in da_file. */
+/* Computes hybrid profile for all matching entries in da_file.
+
+ CFG_CHECKSUM is the precomputed checksum for the CFG. */
static gcov_type *
-get_exec_counts (void)
+get_exec_counts (unsigned cfg_checksum, unsigned lineno_checksum)
{
unsigned num_edges = 0;
basic_block bb;
@@ -253,7 +248,8 @@ get_exec_counts (void)
num_edges++;
}
- counts = get_coverage_counts (GCOV_COUNTER_ARCS, num_edges, &profile_info);
+ counts = get_coverage_counts (GCOV_COUNTER_ARCS, num_edges, cfg_checksum,
+ lineno_checksum, &profile_info);
if (!counts)
return NULL;
@@ -442,10 +438,12 @@ read_profile_edge_counts (gcov_type *exe
}
/* Compute the branch probabilities for the various branches.
- Annotate them accordingly. */
+ Annotate them accordingly.
+
+ CFG_CHECKSUM is the precomputed checksum for the CFG. */
static void
-compute_branch_probabilities (void)
+compute_branch_probabilities (unsigned cfg_checksum, unsigned lineno_checksum)
{
basic_block bb;
int i;
@@ -454,7 +452,7 @@ compute_branch_probabilities (void)
int passes;
int hist_br_prob[20];
int num_branches;
- gcov_type *exec_counts = get_exec_counts ();
+ gcov_type *exec_counts = get_exec_counts (cfg_checksum, lineno_checksum);
int inconsistent = 0;
/* Very simple sanity checks so we catch bugs in our profiling code. */
@@ -772,10 +770,13 @@ compute_branch_probabilities (void)
}
/* Load value histograms values whose description is stored in VALUES array
- from .gcda file. */
+ from .gcda file.
+
+ CFG_CHECKSUM is the precomputed checksum for the CFG. */
static void
-compute_value_histograms (histogram_values values)
+compute_value_histograms (histogram_values values, unsigned cfg_checksum,
+ unsigned lineno_checksum)
{
unsigned i, j, t, any;
unsigned n_histogram_counters[GCOV_N_VALUE_COUNTERS];
@@ -803,7 +804,8 @@ compute_value_histograms (histogram_valu
histogram_counts[t] =
get_coverage_counts (COUNTER_FOR_HIST_TYPE (t),
- n_histogram_counters[t], NULL);
+ n_histogram_counters[t], cfg_checksum,
+ lineno_checksum, NULL);
if (histogram_counts[t])
any = 1;
act_count[t] = histogram_counts[t];
@@ -905,6 +907,7 @@ branch_prob (void)
unsigned num_instrumented;
struct edge_list *el;
histogram_values values = NULL;
+ unsigned cfg_checksum, lineno_checksum;
total_num_times_called++;
@@ -1058,11 +1061,19 @@ branch_prob (void)
if (dump_file)
fprintf (dump_file, "%d ignored edges\n", ignored_edges);
+
+ /* Compute two different checksums. Note that we want to compute
+ the checksum in only once place, since it depends on the shape
+ of the control flow which can change during
+ various transformations. */
+ cfg_checksum = coverage_compute_cfg_checksum ();
+ lineno_checksum = coverage_compute_lineno_checksum ();
+
/* Write the data from which gcov can reconstruct the basic block
graph. */
/* Basic block flags */
- if (coverage_begin_output ())
+ if (coverage_begin_output (lineno_checksum, cfg_checksum))
{
gcov_position_t offset;
@@ -1079,7 +1090,7 @@ branch_prob (void)
EXIT_BLOCK_PTR->index = last_basic_block;
/* Arcs */
- if (coverage_begin_output ())
+ if (coverage_begin_output (lineno_checksum, cfg_checksum))
{
gcov_position_t offset;
@@ -1120,7 +1131,7 @@ branch_prob (void)
}
/* Line numbers. */
- if (coverage_begin_output ())
+ if (coverage_begin_output (lineno_checksum, cfg_checksum))
{
/* Initialize the output. */
output_location (NULL, 0, NULL, NULL);
@@ -1175,9 +1186,9 @@ branch_prob (void)
if (flag_branch_probabilities)
{
- compute_branch_probabilities ();
+ compute_branch_probabilities (cfg_checksum, lineno_checksum);
if (flag_profile_values)
- compute_value_histograms (values);
+ compute_value_histograms (values, cfg_checksum, lineno_checksum);
}
remove_fake_edges ();
@@ -1205,7 +1216,7 @@ branch_prob (void)
VEC_free (histogram_value, heap, values);
free_edge_list (el);
- coverage_end_function ();
+ coverage_end_function (lineno_checksum, cfg_checksum);
}
\f
/* Union find algorithm implementation for the basic blocks using
@@ -1372,4 +1383,3 @@ end_branch_prob (void)
}
}
}
-
Index: coverage.c
===================================================================
--- coverage.c (revision 172802)
+++ coverage.c (working copy)
@@ -51,13 +51,15 @@ along with GCC; see the file COPYING3.
#include "intl.h"
#include "filenames.h"
+#include "gcov-io.h"
#include "gcov-io.c"
struct function_list
{
struct function_list *next; /* next function */
unsigned ident; /* function ident */
- unsigned checksum; /* function checksum */
+ unsigned lineno_checksum; /* function lineno checksum */
+ unsigned cfg_checksum; /* function cfg checksum */
unsigned n_ctrs[GCOV_COUNTERS];/* number of counters. */
};
@@ -69,7 +71,8 @@ typedef struct counts_entry
unsigned ctr;
/* Store */
- unsigned checksum;
+ unsigned lineno_checksum;
+ unsigned cfg_checksum;
gcov_type *counts;
struct gcov_ctr_summary summary;
@@ -114,8 +117,6 @@ static hashval_t htab_counts_entry_hash
static int htab_counts_entry_eq (const void *, const void *);
static void htab_counts_entry_del (void *);
static void read_counts_file (void);
-static unsigned compute_checksum (void);
-static unsigned coverage_checksum_string (unsigned, const char *);
static tree build_fn_info_type (unsigned);
static tree build_fn_info_value (const struct function_list *, tree);
static tree build_ctr_info_type (void);
@@ -171,11 +172,12 @@ static void
read_counts_file (void)
{
gcov_unsigned_t fn_ident = 0;
- gcov_unsigned_t checksum = -1;
counts_entry_t *summaried = NULL;
unsigned seen_summary = 0;
gcov_unsigned_t tag;
int is_error = 0;
+ unsigned lineno_checksum = 0;
+ unsigned cfg_checksum = 0;
if (!gcov_open (da_file_name, 1))
return;
@@ -215,7 +217,8 @@ read_counts_file (void)
if (tag == GCOV_TAG_FUNCTION)
{
fn_ident = gcov_read_unsigned ();
- checksum = gcov_read_unsigned ();
+ lineno_checksum = gcov_read_unsigned ();
+ cfg_checksum = gcov_read_unsigned ();
if (seen_summary)
{
/* We have already seen a summary, this means that this
@@ -265,17 +268,21 @@ read_counts_file (void)
if (!entry)
{
*slot = entry = XCNEW (counts_entry_t);
- entry->ident = elt.ident;
+ entry->ident = fn_ident;
entry->ctr = elt.ctr;
- entry->checksum = checksum;
+ entry->lineno_checksum = lineno_checksum;
+ entry->cfg_checksum = cfg_checksum;
entry->summary.num = n_counts;
entry->counts = XCNEWVEC (gcov_type, n_counts);
}
- else if (entry->checksum != checksum)
+ else if (entry->lineno_checksum != lineno_checksum
+ || entry->cfg_checksum != cfg_checksum)
{
error ("coverage mismatch for function %u while reading execution counters",
fn_ident);
- error ("checksum is %x instead of %x", entry->checksum, checksum);
+ error ("checksum is (%x,%x) instead of (%x,%x)",
+ entry->lineno_checksum, entry->cfg_checksum,
+ lineno_checksum, cfg_checksum);
htab_delete (counts_hash);
break;
}
@@ -324,10 +331,10 @@ read_counts_file (void)
gcov_type *
get_coverage_counts (unsigned counter, unsigned expected,
+ unsigned cfg_checksum, unsigned lineno_checksum,
const struct gcov_ctr_summary **summary)
{
counts_entry_t *entry, elt;
- gcov_unsigned_t checksum = -1;
/* No hash table, no counts. */
if (!counts_hash)
@@ -352,23 +359,22 @@ get_coverage_counts (unsigned counter, u
return NULL;
}
- checksum = compute_checksum ();
- if (entry->checksum != checksum
+ if (entry->cfg_checksum != cfg_checksum
|| entry->summary.num != expected)
{
static int warned = 0;
bool warning_printed = false;
tree id = DECL_ASSEMBLER_NAME (current_function_decl);
- warning_printed =
- warning_at (input_location, OPT_Wcoverage_mismatch,
+ warning_printed =
+ warning_at (input_location, OPT_Wcoverage_mismatch,
"coverage mismatch for function "
"%qE while reading counter %qs", id, ctr_names[counter]);
if (warning_printed)
{
- if (entry->checksum != checksum)
- inform (input_location, "checksum is %x instead of %x",
- entry->checksum, checksum);
+ if (entry->cfg_checksum != cfg_checksum)
+ inform (input_location, "control flow checksum is %x instead of %x",
+ entry->cfg_checksum, cfg_checksum);
else
inform (input_location, "number of counters is %d instead of %d",
entry->summary.num, expected);
@@ -388,6 +394,12 @@ get_coverage_counts (unsigned counter, u
return NULL;
}
+ else if (entry->lineno_checksum != lineno_checksum)
+ {
+ warning (0, "Source location for function %qE have changed,"
+ " the profile data may be out of date",
+ DECL_ASSEMBLER_NAME (current_function_decl));
+ }
if (summary)
*summary = &entry->summary;
@@ -467,6 +479,7 @@ tree_coverage_counter_addr (unsigned cou
NULL, NULL));
}
\f
+
/* Generate a checksum for a string. CHKSUM is the current
checksum. */
@@ -529,8 +542,8 @@ coverage_checksum_string (unsigned chksu
/* Compute checksum for the current function. We generate a CRC32. */
-static unsigned
-compute_checksum (void)
+unsigned
+coverage_compute_lineno_checksum (void)
{
expanded_location xloc
= expand_location (DECL_SOURCE_LOCATION (current_function_decl));
@@ -542,6 +555,35 @@ compute_checksum (void)
return chksum;
}
+
+/* Compute cfg checksum for the current function.
+ The checksum is calculated carefully so that
+ source code changes that doesn't affect the control flow graph
+ won't change the checksum.
+ This is to make the profile data useable across source code change.
+ The downside of this is that the compiler may use potentially
+ wrong profile data - that the source code change has non-trivial impact
+ on the validity of profile data (e.g. the reversed condition)
+ but the compiler won't detect the change and use the wrong profile data. */
+
+unsigned
+coverage_compute_cfg_checksum (void)
+{
+ basic_block bb;
+ unsigned chksum = n_basic_blocks;
+
+ FOR_EACH_BB (bb)
+ {
+ edge e;
+ edge_iterator ei;
+ FOR_EACH_EDGE (e, ei, bb->succs)
+ {
+ chksum = crc32_byte (chksum, e->dest->index);
+ }
+ }
+
+ return chksum;
+}
\f
/* Begin output to the graph file for the current function.
Opens the output file, if not already done. Writes the
@@ -549,7 +591,7 @@ compute_checksum (void)
should be output. */
int
-coverage_begin_output (void)
+coverage_begin_output (unsigned lineno_checksum, unsigned cfg_checksum)
{
/* We don't need to output .gcno file unless we're under -ftest-coverage
(e.g. -fprofile-arcs/generate/use don't need .gcno to work). */
@@ -575,12 +617,14 @@ coverage_begin_output (void)
bbg_file_opened = 1;
}
+
/* Announce function */
offset = gcov_write_tag (GCOV_TAG_FUNCTION);
gcov_write_unsigned (current_function_funcdef_no + 1);
- gcov_write_unsigned (compute_checksum ());
+ gcov_write_unsigned (lineno_checksum);
+ gcov_write_unsigned (cfg_checksum);
gcov_write_string (IDENTIFIER_POINTER
- (DECL_ASSEMBLER_NAME (current_function_decl)));
+ (DECL_ASSEMBLER_NAME (current_function_decl)));
gcov_write_string (xloc.file);
gcov_write_unsigned (xloc.line);
gcov_write_length (offset);
@@ -594,7 +638,7 @@ coverage_begin_output (void)
error has occurred. Save function coverage counts. */
void
-coverage_end_function (void)
+coverage_end_function (unsigned lineno_checksum, unsigned cfg_checksum)
{
unsigned i;
@@ -613,9 +657,11 @@ coverage_end_function (void)
*functions_tail = item;
functions_tail = &item->next;
+
item->next = 0;
item->ident = current_function_funcdef_no + 1;
- item->checksum = compute_checksum ();
+ item->lineno_checksum = lineno_checksum;
+ item->cfg_checksum = cfg_checksum;
for (i = 0; i != GCOV_COUNTERS; i++)
{
item->n_ctrs[i] = fn_n_ctrs[i];
@@ -640,13 +686,18 @@ build_fn_info_type (unsigned int counter
/* ident */
fields = build_decl (BUILTINS_LOCATION,
FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
-
- /* checksum */
+ /* lineno_checksum */
field = build_decl (BUILTINS_LOCATION,
FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
DECL_CHAIN (field) = fields;
fields = field;
+ /* cfg checksum */
+ field = build_decl (BUILTINS_LOCATION,
+ FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
+ DECL_CHAIN (field) = fields;
+ fields = field;
+
array_type = build_int_cst (NULL_TREE, counters - 1);
array_type = build_index_type (array_type);
array_type = build_array_type (get_gcov_unsigned_t (), array_type);
@@ -680,10 +731,16 @@ build_fn_info_value (const struct functi
function->ident));
fields = DECL_CHAIN (fields);
- /* checksum */
+ /* lineno_checksum */
+ CONSTRUCTOR_APPEND_ELT (v1, fields,
+ build_int_cstu (get_gcov_unsigned_t (),
+ function->lineno_checksum));
+ fields = DECL_CHAIN (fields);
+
+ /* cfg_checksum */
CONSTRUCTOR_APPEND_ELT (v1, fields,
build_int_cstu (get_gcov_unsigned_t (),
- function->checksum));
+ function->cfg_checksum));
fields = DECL_CHAIN (fields);
/* counters */
Index: coverage.h
===================================================================
--- coverage.h (revision 172802)
+++ coverage.h (working copy)
@@ -28,11 +28,17 @@ extern void coverage_finish (void);
/* Complete the coverage information for the current function. Once
per function. */
-extern void coverage_end_function (void);
+extern void coverage_end_function (unsigned, unsigned);
/* Start outputting coverage information for the current
function. Repeatable per function. */
-extern int coverage_begin_output (void);
+extern int coverage_begin_output (unsigned, unsigned);
+
+/* Compute the control flow checksum for the current function. */
+extern unsigned coverage_compute_cfg_checksum (void);
+
+/* Compute the line number checksum for the current function. */
+extern unsigned coverage_compute_lineno_checksum (void);
/* Allocate some counters. Repeatable per function. */
extern int coverage_counter_alloc (unsigned /*counter*/, unsigned/*num*/);
@@ -44,6 +50,8 @@ extern tree tree_coverage_counter_addr (
/* Get all the counters for the current function. */
extern gcov_type *get_coverage_counts (unsigned /*counter*/,
unsigned /*expected*/,
+ unsigned /*cfg_checksum*/,
+ unsigned /*lineno_checksum*/,
const struct gcov_ctr_summary **);
extern tree get_gcov_type (void);
Index: libgcov.c
===================================================================
--- libgcov.c (revision 172802)
+++ libgcov.c (working copy)
@@ -372,9 +372,10 @@ gcov_exit (void)
/* Check function. */
if (tag != GCOV_TAG_FUNCTION
- || length != GCOV_TAG_FUNCTION_LENGTH
+ || length != GCOV_TAG_FUNCTION_LENGTH
|| gcov_read_unsigned () != fi_ptr->ident
- || gcov_read_unsigned () != fi_ptr->checksum)
+ || gcov_read_unsigned () != fi_ptr->lineno_checksum
+ || gcov_read_unsigned () != fi_ptr->cfg_checksum)
{
read_mismatch:;
fprintf (stderr, "profiling:%s:Merge mismatch for %s\n",
@@ -517,7 +518,8 @@ gcov_exit (void)
/* Announce function. */
gcov_write_tag_length (GCOV_TAG_FUNCTION, GCOV_TAG_FUNCTION_LENGTH);
gcov_write_unsigned (fi_ptr->ident);
- gcov_write_unsigned (fi_ptr->checksum);
+ gcov_write_unsigned (fi_ptr->lineno_checksum);
+ gcov_write_unsigned (fi_ptr->cfg_checksum);
c_ix = 0;
for (t_ix = 0; t_ix < GCOV_COUNTERS; t_ix++)
Index: gcov-dump.c
===================================================================
--- gcov-dump.c (revision 172802)
+++ gcov-dump.c (working copy)
@@ -267,7 +267,8 @@ tag_function (const char *filename ATTRI
unsigned long pos = gcov_position ();
printf (" ident=%u", gcov_read_unsigned ());
- printf (", checksum=0x%08x", gcov_read_unsigned ());
+ printf (", lineno_checksum=0x%08x", gcov_read_unsigned ());
+ printf (", cfg_checksum_checksum=0x%08x", gcov_read_unsigned ());
if (gcov_position () - pos < length)
{
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-22 19:35 ` Xinliang David Li
@ 2011-04-22 20:11 ` Jan Hubicka
2011-04-25 17:05 ` Xinliang David Li
2011-04-27 16:27 ` Jan Hubicka
1 sibling, 1 reply; 19+ messages in thread
From: Jan Hubicka @ 2011-04-22 20:11 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Jan Hubicka, GCC Patches, Diego Novillo
> Please review the new patch which only implements cfg checksum.
>
> The auto version generation was introduced in 2002 before FDO support
> was added (so the old way never existed), so it might be better to
> make the change independent of this one.
FDO support was there well before 2002, just with funny combination of flags.
(-fprofile-arcs and -fbranch-probabilities)
2002 was about the time of GCOV format rewrite, but yes, lets change it independently
I will look into the patch ASAP.
Honza
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-22 20:11 ` Jan Hubicka
@ 2011-04-25 17:05 ` Xinliang David Li
2011-04-27 16:22 ` Xinliang David Li
0 siblings, 1 reply; 19+ messages in thread
From: Xinliang David Li @ 2011-04-25 17:05 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, Diego Novillo
Is the patch ok?
Thanks,
David
On Fri, Apr 22, 2011 at 12:48 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Please review the new patch which only implements cfg checksum.
>>
>> The auto version generation was introduced in 2002 before FDO support
>> was added (so the old way never existed), so it might be better to
>> make the change independent of this one.
>
> FDO support was there well before 2002, just with funny combination of flags.
> (-fprofile-arcs and -fbranch-probabilities)
> 2002 was about the time of GCOV format rewrite, but yes, lets change it independently
> I will look into the patch ASAP.
>
> Honza
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-25 17:05 ` Xinliang David Li
@ 2011-04-27 16:22 ` Xinliang David Li
0 siblings, 0 replies; 19+ messages in thread
From: Xinliang David Li @ 2011-04-27 16:22 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, Diego Novillo
Honza, any more comments?
Thanks,
David
On Mon, Apr 25, 2011 at 9:35 AM, Xinliang David Li <davidxl@google.com> wrote:
> Is the patch ok?
>
> Thanks,
>
> David
>
> On Fri, Apr 22, 2011 at 12:48 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> Please review the new patch which only implements cfg checksum.
>>>
>>> The auto version generation was introduced in 2002 before FDO support
>>> was added (so the old way never existed), so it might be better to
>>> make the change independent of this one.
>>
>> FDO support was there well before 2002, just with funny combination of flags.
>> (-fprofile-arcs and -fbranch-probabilities)
>> 2002 was about the time of GCOV format rewrite, but yes, lets change it independently
>> I will look into the patch ASAP.
>>
>> Honza
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-22 19:35 ` Xinliang David Li
2011-04-22 20:11 ` Jan Hubicka
@ 2011-04-27 16:27 ` Jan Hubicka
2011-04-27 17:45 ` Xinliang David Li
1 sibling, 1 reply; 19+ messages in thread
From: Jan Hubicka @ 2011-04-27 16:27 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Jan Hubicka, GCC Patches, Diego Novillo
Please don't forget about changelogs..
> Index: tree.c
> ===================================================================
> --- tree.c (revision 172802)
> +++ tree.c (working copy)
> @@ -8513,14 +8513,12 @@ dump_tree_statistics (void)
The crc bits was already reviewed, right?
> - else if (entry->checksum != checksum)
> + else if (entry->lineno_checksum != lineno_checksum
> + || entry->cfg_checksum != cfg_checksum)
> {
> error ("coverage mismatch for function %u while reading execution counters",
> fn_ident);
> - error ("checksum is %x instead of %x", entry->checksum, checksum);
> + error ("checksum is (%x,%x) instead of (%x,%x)",
> + entry->lineno_checksum, entry->cfg_checksum,
> + lineno_checksum, cfg_checksum);
Can't we give more informative message whether code changes or it seems to be
optimization options disprepancy?
> +unsigned
> +coverage_compute_cfg_checksum (void)
> +{
> + basic_block bb;
> + unsigned chksum = n_basic_blocks;
> +
> + FOR_EACH_BB (bb)
> + {
> + edge e;
> + edge_iterator ei;
Perhaps also
chksum = crc32_byte (chksum, bb->index)
for cases where destination stays the same, but source of edge moves (i.e. with forwarder
blocks)
Patch is OK, thanks
Honza
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-27 16:27 ` Jan Hubicka
@ 2011-04-27 17:45 ` Xinliang David Li
2011-04-27 17:54 ` Jan Hubicka
0 siblings, 1 reply; 19+ messages in thread
From: Xinliang David Li @ 2011-04-27 17:45 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, Diego Novillo
On Wed, Apr 27, 2011 at 8:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Please don't forget about changelogs..
>> Index: tree.c
>> ===================================================================
>> --- tree.c (revision 172802)
>> +++ tree.c (working copy)
>> @@ -8513,14 +8513,12 @@ dump_tree_statistics (void)
>
> The crc bits was already reviewed, right?
Yes.
>> - else if (entry->checksum != checksum)
>> + else if (entry->lineno_checksum != lineno_checksum
>> + || entry->cfg_checksum != cfg_checksum)
>> {
>> error ("coverage mismatch for function %u while reading execution counters",
>> fn_ident);
>> - error ("checksum is %x instead of %x", entry->checksum, checksum);
>> + error ("checksum is (%x,%x) instead of (%x,%x)",
>> + entry->lineno_checksum, entry->cfg_checksum,
>> + lineno_checksum, cfg_checksum);
>
> Can't we give more informative message whether code changes or it seems to be
> optimization options disprepancy?
Good idea -- but to change the warning not the error here. For the
warning (which is promoted to error by default) currently it is:
error: coverage mismatch for function xxxx while reading counter yyy.
note: control flow checksum is aaa instead of bbb
Could be:
error: function xxx's control flow does match its profile data (counter yyy).
note: use -Wno-error=coverage-mismatch to tolerate the mismatch but
performance may drop if the function is hot.
>> +unsigned
>> +coverage_compute_cfg_checksum (void)
>> +{
>> + basic_block bb;
>> + unsigned chksum = n_basic_blocks;
>> +
>> + FOR_EACH_BB (bb)
>> + {
>> + edge e;
>> + edge_iterator ei;
> Perhaps also
> chksum = crc32_byte (chksum, bb->index)
> for cases where destination stays the same, but source of edge moves (i.e. with forwarder
> blocks)
Yes.
Thanks,
David
>
> Patch is OK, thanks
> Honza
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-27 17:45 ` Xinliang David Li
@ 2011-04-27 17:54 ` Jan Hubicka
2011-04-27 17:57 ` Xinliang David Li
0 siblings, 1 reply; 19+ messages in thread
From: Jan Hubicka @ 2011-04-27 17:54 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Jan Hubicka, GCC Patches, Diego Novillo
> >> - Â Â Â else if (entry->checksum != checksum)
> >> + Â Â Â else if (entry->lineno_checksum != lineno_checksum
> >> + Â Â Â Â Â Â Â Â || entry->cfg_checksum != cfg_checksum)
> >> Â Â Â Â Â {
> >> Â Â Â Â Â Â error ("coverage mismatch for function %u while reading execution counters",
> >> Â Â Â Â Â Â Â Â Â Â fn_ident);
> >> - Â Â Â Â Â error ("checksum is %x instead of %x", entry->checksum, checksum);
> >> + Â Â Â Â Â error ("checksum is (%x,%x) instead of (%x,%x)",
> >> + Â Â Â Â Â Â Â Â Â entry->lineno_checksum, entry->cfg_checksum,
> >> + Â Â Â Â Â Â Â Â Â lineno_checksum, cfg_checksum);
> >
> > Can't we give more informative message whether code changes or it seems to be
> > optimization options disprepancy?
>
> Good idea -- but to change the warning not the error here. For the
> warning (which is promoted to error by default) currently it is:
>
> error: coverage mismatch for function xxxx while reading counter yyy.
> note: control flow checksum is aaa instead of bbb
>
> Could be:
> error: function xxx's control flow does match its profile data (counter yyy).
> note: use -Wno-error=coverage-mismatch to tolerate the mismatch but
> performance may drop if the function is hot.
Well, I had more in mind giving diffent message for lineno checksum difference than cfg checksum.
But I agree that we probably don't want to output checksum numbers, those are useless even for debugging.
Probably could follow same warning/error rules as the main loading routine whatever you converged
to.
Honza
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: FDO usability patch -- cfg and lineno checksum
2011-04-27 17:54 ` Jan Hubicka
@ 2011-04-27 17:57 ` Xinliang David Li
0 siblings, 0 replies; 19+ messages in thread
From: Xinliang David Li @ 2011-04-27 17:57 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, Diego Novillo
On Wed, Apr 27, 2011 at 10:03 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> - else if (entry->checksum != checksum)
>> >> + else if (entry->lineno_checksum != lineno_checksum
>> >> + || entry->cfg_checksum != cfg_checksum)
>> >> {
>> >> error ("coverage mismatch for function %u while reading execution counters",
>> >> fn_ident);
>> >> - error ("checksum is %x instead of %x", entry->checksum, checksum);
>> >> + error ("checksum is (%x,%x) instead of (%x,%x)",
>> >> + entry->lineno_checksum, entry->cfg_checksum,
>> >> + lineno_checksum, cfg_checksum);
>> >
>> > Can't we give more informative message whether code changes or it seems to be
>> > optimization options disprepancy?
>>
>> Good idea -- but to change the warning not the error here. For the
>> warning (which is promoted to error by default) currently it is:
>>
>> error: coverage mismatch for function xxxx while reading counter yyy.
>> note: control flow checksum is aaa instead of bbb
>>
>> Could be:
>> error: function xxx's control flow does match its profile data (counter yyy).
>> note: use -Wno-error=coverage-mismatch to tolerate the mismatch but
>> performance may drop if the function is hot.
>
> Well, I had more in mind giving diffent message for lineno checksum difference than cfg checksum.
> But I agree that we probably don't want to output checksum numbers, those are useless even for debugging.
> Probably could follow same warning/error rules as the main loading routine whatever you converged
> to.
>
Sure.
Thanks,
David
> Honza
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2011-04-27 17:09 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-13 22:26 FDO usability patch -- cfg and lineno checksum Xinliang David Li
2011-04-13 22:34 ` Xinliang David Li
[not found] ` <BANLkTinVavyXCqy88TpqLzmexp2LQ1Pwqw@mail.gmail.com>
2011-04-15 16:54 ` Xinliang David Li
2011-04-17 17:15 ` Jan Hubicka
2011-04-17 19:22 ` Xinliang David Li
2011-04-19 20:37 ` Xinliang David Li
2011-04-19 22:36 ` Jan Hubicka
2011-04-21 8:30 ` Xinliang David Li
2011-04-21 22:21 ` Jan Hubicka
2011-04-21 22:42 ` Xinliang David Li
2011-04-22 19:35 ` Xinliang David Li
2011-04-22 20:11 ` Jan Hubicka
2011-04-25 17:05 ` Xinliang David Li
2011-04-27 16:22 ` Xinliang David Li
2011-04-27 16:27 ` Jan Hubicka
2011-04-27 17:45 ` Xinliang David Li
2011-04-27 17:54 ` Jan Hubicka
2011-04-27 17:57 ` Xinliang David Li
2011-04-21 18:23 ` Diego Novillo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).