On 10/1/21 12:17, Richard Biener wrote:
On Fri, Oct 1, 2021 at 11:55 AM Martin Liška <mli...@suse.cz> wrote:

Support merging of profiles that are built from a different .o files
but belong to the same source file. Moreover, a checksum is verified
during profile merging and so we can safely combine such profile.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
I'm going to install the patch if there are no comments about it.

Is the ->stamp field now unused?

Yes, it's still used for pairing of .gcda and .gcno files when --coverage is 
used.


I wonder whether crc32 is good enough or whether we need to enable

Dunno. We can alternatively use a stronger hashing function, maybe inchash?

-fprofile-correction by default for example?

Probably not.

But -fprofile-correction
is about -fprofile-use, not profile merging, right?  What does the latter
do upon mismatch?

It prints the 'libgcov profiling error'.


Alternatively would it be possible to keep multiple sets of data in the file,
one for each 'stamp'?

Yes, we can theoretically do it, but I'm not planning working on that now.

How does the profile-use step figure a mismatch
here, or does it not care whether it mixes input with 'different stamp'?

Based on function id, it does verification for cfg_checksum and lineno_checksum.


The current behavior is also somewhat odd:

./a.out
libgcov profiling error:/tmp/t.gcda:overwriting an existing profile
data with a different timestamp

but it should probably say 'warning' since it indeed simply overwrites data
instead of merging it.

Yes, I can prepare an incremental patch for it.


I wonder if we can simply drop the stamp check alltogether and make
the checks that match up the data against the number of counters behave
as to warning and overwriting the data instead of failing fatally?  The
actual merging of data would need to be delayed, but at least until
the first actual merge was done we could simply proceed?

Do you mean doing only merging of functions that have correct checksum, and 
bailing
out for the functions that don't?

Thanks for the ideas.

Martin


Richard.

Thanks,
Martin

         PR gcov-profile/90364

gcc/ChangeLog:

         * coverage.c (build_info): Emit checksum to the global variable.
         (build_info_type): Add new field for checksum.
         (coverage_obj_finish): Pass object_checksum.
         (coverage_init): Use 0 as checksum for .gcno files.
         * gcov-dump.c (dump_gcov_file): Dump also new checksum field.
         * gcov.c (read_graph_file): Read also checksum.

libgcc/ChangeLog:

         * libgcov-driver.c (merge_one_data): Skip timestamp and verify
         checksums.
         (write_one_data): Write also checksum.
         * libgcov-util.c (read_gcda_file): Read also checksum field.
         * libgcov.h (struct gcov_info): Add new field.
---
   gcc/coverage.c          | 50 ++++++++++++++++++++++++++++-------------
   gcc/gcov-dump.c         |  9 ++++----
   gcc/gcov.c              |  5 +++++
   libgcc/libgcov-driver.c |  8 +++++--
   libgcc/libgcov-util.c   |  3 +++
   libgcc/libgcov.h        |  1 +
   6 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/gcc/coverage.c b/gcc/coverage.c
index 10d7f8366cb..4467f1eaa5c 100644
--- a/gcc/coverage.c
+++ b/gcc/coverage.c
@@ -129,16 +129,7 @@ static const char *const ctr_names[GCOV_COUNTERS] = {
   #undef DEF_GCOV_COUNTER

   /* Forward declarations.  */
-static void read_counts_file (void);
   static tree build_var (tree, tree, int);
-static void build_fn_info_type (tree, unsigned, tree);
-static void build_info_type (tree, tree);
-static tree build_fn_info (const struct coverage_data *, tree, tree);
-static tree build_info (tree, tree);
-static bool coverage_obj_init (void);
-static vec<constructor_elt, va_gc> *coverage_obj_fn
-(vec<constructor_elt, va_gc> *, tree, struct coverage_data const *);
-static void coverage_obj_finish (vec<constructor_elt, va_gc> *);

   /* Return the type node for gcov_type.  */

@@ -218,6 +209,9 @@ read_counts_file (void)
     tag = gcov_read_unsigned ();
     bbg_file_stamp = crc32_unsigned (bbg_file_stamp, tag);

+  /* Read checksum.  */
+  gcov_read_unsigned ();
+
     counts_hash = new hash_table<counts_entry> (10);
     while ((tag = gcov_read_unsigned ()))
       {
@@ -935,6 +929,12 @@ build_info_type (tree type, tree fn_info_ptr_type)
     DECL_CHAIN (field) = fields;
     fields = field;

+  /* Checksum.  */
+  field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
+                     get_gcov_unsigned_t ());
+  DECL_CHAIN (field) = fields;
+  fields = field;
+
     /* Filename */
     field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
                       build_pointer_type (build_qualified_type
@@ -977,7 +977,7 @@ build_info_type (tree type, tree fn_info_ptr_type)
      function info objects.  */

   static tree
-build_info (tree info_type, tree fn_ary)
+build_info (tree info_type, tree fn_ary, unsigned object_checksum)
   {
     tree info_fields = TYPE_FIELDS (info_type);
     tree merge_fn_type, n_funcs;
@@ -996,13 +996,19 @@ build_info (tree info_type, tree fn_ary)
     /* next -- NULL */
     CONSTRUCTOR_APPEND_ELT (v1, info_fields, null_pointer_node);
     info_fields = DECL_CHAIN (info_fields);
-
+
     /* stamp */
     CONSTRUCTOR_APPEND_ELT (v1, info_fields,
                           build_int_cstu (TREE_TYPE (info_fields),
                                           bbg_file_stamp));
     info_fields = DECL_CHAIN (info_fields);

+  /* Checksum.  */
+  CONSTRUCTOR_APPEND_ELT (v1, info_fields,
+                         build_int_cstu (TREE_TYPE (info_fields),
+                                         object_checksum));
+  info_fields = DECL_CHAIN (info_fields);
+
     /* Filename */
     da_file_name_len = strlen (da_file_name);
     filename_string = build_string (da_file_name_len + 1, da_file_name);
@@ -1214,7 +1220,8 @@ coverage_obj_fn (vec<constructor_elt, va_gc> *ctor, tree 
fn,
      function objects from CTOR.  Generate the gcov_info initializer.  */

   static void
-coverage_obj_finish (vec<constructor_elt, va_gc> *ctor)
+coverage_obj_finish (vec<constructor_elt, va_gc> *ctor,
+                    unsigned object_checksum)
   {
     unsigned n_functions = vec_safe_length (ctor);
     tree fn_info_ary_type = build_array_type
@@ -1231,7 +1238,7 @@ coverage_obj_finish (vec<constructor_elt, va_gc> *ctor)
     varpool_node::finalize_decl (fn_info_ary);

     DECL_INITIAL (gcov_info_var)
-    = build_info (TREE_TYPE (gcov_info_var), fn_info_ary);
+    = build_info (TREE_TYPE (gcov_info_var), fn_info_ary, object_checksum);
     varpool_node::finalize_decl (gcov_info_var);
   }

@@ -1300,7 +1307,6 @@ coverage_init (const char *filename)
     strcpy (da_file_name + prefix_len + len, GCOV_DATA_SUFFIX);

     bbg_file_stamp = local_tick;
-
     if (flag_auto_profile)
       read_autofdo_file ();
     else if (flag_branch_probabilities)
@@ -1328,6 +1334,8 @@ coverage_init (const char *filename)
           gcov_write_unsigned (GCOV_NOTE_MAGIC);
           gcov_write_unsigned (GCOV_VERSION);
           gcov_write_unsigned (bbg_file_stamp);
+         /* Use an arbitrary checksum */
+         gcov_write_unsigned (0);
           gcov_write_string (getpwd ());

           /* Do not support has_unexecuted_blocks for Ada.  */
@@ -1353,14 +1361,24 @@ coverage_finish (void)
          cannot uniquely stamp it.  If we can stamp it, libgcov will DTRT.  */
       unlink (da_file_name);

+  /* Global GCDA checksum that aggregates all functions.  */
+  unsigned object_checksum = 0;
+
     if (coverage_obj_init ())
       {
         vec<constructor_elt, va_gc> *fn_ctor = NULL;
         struct coverage_data *fn;

         for (fn = functions_head; fn; fn = fn->next)
-       fn_ctor = coverage_obj_fn (fn_ctor, fn->fn_decl, fn);
-      coverage_obj_finish (fn_ctor);
+       {
+         fn_ctor = coverage_obj_fn (fn_ctor, fn->fn_decl, fn);
+
+         object_checksum = crc32_unsigned (object_checksum, fn->ident);
+         object_checksum = crc32_unsigned (object_checksum,
+                                           fn->lineno_checksum);
+         object_checksum = crc32_unsigned (object_checksum, fn->cfg_checksum);
+       }
+      coverage_obj_finish (fn_ctor, object_checksum);
       }

     XDELETEVEC (da_file_name);
diff --git a/gcc/gcov-dump.c b/gcc/gcov-dump.c
index f1489fe0214..bfaf735d2ff 100644
--- a/gcc/gcov-dump.c
+++ b/gcc/gcov-dump.c
@@ -206,11 +206,12 @@ dump_gcov_file (const char *filename)
     }

     /* stamp */
-  {
-    unsigned stamp = gcov_read_unsigned ();
+  unsigned stamp = gcov_read_unsigned ();
+  printf ("%s:stamp %lu\n", filename, (unsigned long)stamp);

-    printf ("%s:stamp %lu\n", filename, (unsigned long)stamp);
-  }
+  /* Checksum */
+  unsigned checksum = gcov_read_unsigned ();
+  printf ("%s:checksum %lu\n", filename, (unsigned long)checksum);

     if (!is_data_type)
       {
diff --git a/gcc/gcov.c b/gcc/gcov.c
index cf0a49d8c30..829e955a63b 100644
--- a/gcc/gcov.c
+++ b/gcc/gcov.c
@@ -1814,6 +1814,8 @@ read_graph_file (void)
                bbg_file_name, v, e);
       }
     bbg_stamp = gcov_read_unsigned ();
+  /* Read checksum.  */
+  gcov_read_unsigned ();
     bbg_cwd = xstrdup (gcov_read_string ());
     bbg_supports_has_unexecuted_blocks = gcov_read_unsigned ();

@@ -2031,6 +2033,9 @@ read_count_file (void)
         goto cleanup;
       }

+  /* Read checksum.  */
+  gcov_read_unsigned ();
+
     while ((tag = gcov_read_unsigned ()))
       {
         unsigned length = gcov_read_unsigned ();
diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c
index 087f71e0107..7aa97bbb06a 100644
--- a/libgcc/libgcov-driver.c
+++ b/libgcc/libgcov-driver.c
@@ -260,12 +260,15 @@ merge_one_data (const char *filename,
     if (!gcov_version (gi_ptr, length, filename))
       return -1;

+  /* Skip timestamp.  */
+  gcov_read_unsigned ();
+
     length = gcov_read_unsigned ();
-  if (length != gi_ptr->stamp)
+  if (length != gi_ptr->checksum)
       {
         /* Read from a different compilation.  Overwrite the file.  */
         gcov_error (GCOV_PROF_PREFIX "overwriting an existing profile data "
-                 "with a different timestamp\n", filename);
+                 "with a different checksum\n", filename);
         return 0;
       }

@@ -495,6 +498,7 @@ write_one_data (const struct gcov_info *gi_ptr,
     dump_unsigned (GCOV_DATA_MAGIC, dump_fn, arg);
     dump_unsigned (GCOV_VERSION, dump_fn, arg);
     dump_unsigned (gi_ptr->stamp, dump_fn, arg);
+  dump_unsigned (gi_ptr->checksum, dump_fn, arg);

   #ifdef NEED_L_GCOV
     /* Generate whole program statistics.  */
diff --git a/libgcc/libgcov-util.c b/libgcc/libgcov-util.c
index bc7416d99bf..766ca3559c4 100644
--- a/libgcc/libgcov-util.c
+++ b/libgcc/libgcov-util.c
@@ -310,6 +310,9 @@ read_gcda_file (const char *filename)
     /* Read stamp.  */
     obj_info->stamp = gcov_read_unsigned ();

+  /* Read checksum.  */
+  obj_info->checksum = gcov_read_unsigned ();
+
     while (1)
       {
         gcov_position_t base;
diff --git a/libgcc/libgcov.h b/libgcc/libgcov.h
index f6354a7a070..2a365c95759 100644
--- a/libgcc/libgcov.h
+++ b/libgcc/libgcov.h
@@ -230,6 +230,7 @@ struct gcov_info
     struct gcov_info *next;     /* link to next, used by libgcov */

     gcov_unsigned_t stamp;      /* uniquifying time stamp */
+  gcov_unsigned_t checksum;    /* unique object checksum */
     const char *filename;               /* output file name */

     gcov_merge_fn merge[GCOV_COUNTERS];  /* merge functions (null for
--
2.33.0


Reply via email to