[PATCH][google] Fix parallel make issue exposed by recent gcda change

[tejohnson, 2013-02-28 00:51:52]

Fix race with writing module infos in a parallel make exposed
by change that added primary module info to all gcda files. Instead
of saving eof_pos from the initial write, seek to the zero byte
written at the end of the file when reopening and writing the module
infos.

Tested with regression tests and internal test. Ok for google branches?

2013-02-27  Teresa Johnson  <tejohnson@google.com>

	* libgcc/libgcov.c (gcov_dump_module_info): Seek to zero
        word using new end of file relative seek.
	(gcov_merge_gcda_file): Make eof_pos local.
	(gcov_write_gcda_file): Pass new argument to gcov_seek,
        remove old eof_pos handling.
	* gcc/coverage.c (build_info_type): Remove eof_pos from gcov_info.
	(build_info): Ditto.
	* gcc/gcov-io.c (gcov_seek): Add new parameter from_end.
	* gcc/gcov-io.h (struct gcov_info): Remove eof_pos from gcov_info.

Index: libgcc/libgcov.c
===================================================================
--- libgcc/libgcov.c	(revision 196314)
+++ libgcc/libgcov.c	(working copy)
@@ -226,9 +226,6 @@ static struct gcov_summary program;
 /* Record the position of summary info.  */
 static gcov_position_t summary_pos = 0;
 
-/* Record the postion of eof.  */
-static gcov_position_t eof_pos = 0;
-
 /* Number of chars in prefix to be stripped.  */
 static int gcov_prefix_strip = 0;
 
@@ -580,7 +577,7 @@ gcov_dump_module_info (int do_lipo)
 
     /* Overwrite the zero word at the of the file.  */
     gcov_rewrite ();
-    gcov_seek (gi_ptr->eof_pos);
+    gcov_seek (1, 1);
 
     gcov_write_module_infos (gi_ptr, do_lipo);
     /* Write the end marker  */
@@ -947,8 +944,8 @@ gcov_merge_gcda_file (struct gcov_info *gi_ptr)
   const struct gcov_fn_info *gfi_ptr;
   int error = 0;
   gcov_unsigned_t tag, length, version, stamp;
+  gcov_position_t eof_pos = 0;
 
-  eof_pos = 0;
   summary_pos = 0;
   sum_buffer = 0;
   sum_tail = &sum_buffer;
@@ -1202,15 +1199,14 @@ gcov_write_gcda_file (struct gcov_info *gi_ptr)
   const struct gcov_ctr_info *ci_ptr;
   unsigned t_ix, f_ix, n_counts, length;
   int error = 0;
-  gcov_position_t eof_pos1 = 0;
 
   /* Write out the data.  */
-  gcov_seek (0);
+  gcov_seek (0, 0);
   gcov_write_tag_length (GCOV_DATA_MAGIC, GCOV_VERSION);
   gcov_write_unsigned (gi_ptr->stamp);
 
    if (summary_pos)
-     gcov_seek (summary_pos);
+     gcov_seek (summary_pos, 0);
   gcc_assert (!summary_pos || summary_pos == gcov_position ());
 
   /* Generate whole program statistics.  */
@@ -1259,14 +1255,10 @@ gcov_write_gcda_file (struct gcov_info *gi_ptr)
             gcov_write_counter (*c_ptr++);
           ci_ptr++;
         }
-      eof_pos1 = gcov_position ();
     }
-    eof_pos = eof_pos1;
     /* Write the end marker  */
     gcov_write_unsigned (0);
 
-    gi_ptr->eof_pos = eof_pos;
-
     if ((error = gcov_close ()))
       gcov_error (error  < 0 ?
                   "profiling:%s:Overflow writing\n" :
Index: gcc/coverage.c
===================================================================
--- gcc/coverage.c	(revision 196314)
+++ gcc/coverage.c	(working copy)
@@ -1961,12 +1961,6 @@ build_info_type (tree type, tree fn_info_ptr_type)
   DECL_CHAIN (field) = fields;
   fields = field;
 
-  /* eof_pos */
-  field = build_decl (BUILTINS_LOCATION, FIELD_DECL,
-                      NULL_TREE, get_gcov_unsigned_t ());
-  DECL_CHAIN (field) = fields;
-  fields = field;
-
   /* merge fn array */
   merge_fn_type
     = build_function_type_list (void_type_node,
@@ -2412,11 +2406,6 @@ build_info (tree info_type, tree fn_ary)
 				  filename_string));
   info_fields = DECL_CHAIN (info_fields);
 
-  /* eof_pos */
-  CONSTRUCTOR_APPEND_ELT (v1, info_fields,
-                          build_int_cstu (TREE_TYPE (info_fields), 0));
-  info_fields = DECL_CHAIN (info_fields);
-
   /* merge fn array -- NULL slots indicate unmeasured counters */
   merge_fn_type = TREE_TYPE (TREE_TYPE (info_fields));
   for (ix = 0; ix != GCOV_COUNTERS; ix++)
Index: gcc/gcov-io.c
===================================================================
--- gcc/gcov-io.c	(revision 196314)
+++ gcc/gcov-io.c	(working copy)
@@ -812,15 +812,19 @@ gcov_sync (gcov_position_t base, gcov_unsigned_t l
 #endif
 
 #if IN_LIBGCOV
-/* Move to a given position in a gcov file.  */
+/* Move to a given position in a gcov file. If FROM_END is true,
+   then seek to the given BASE relative to the end of the file.  */
 
 GCOV_LINKAGE void
-gcov_seek (gcov_position_t base)
+gcov_seek (gcov_position_t base, int from_end)
 {
+  int offset = base << 2;
   gcc_assert (gcov_var.mode < 0);
   if (gcov_var.offset)
     gcov_write_block (gcov_var.offset);
-  _GCOV_fseek (gcov_var.file, base << 2, SEEK_SET);
+  _GCOV_fseek (gcov_var.file,
+               from_end ? -offset : offset,
+               from_end ? SEEK_END : SEEK_SET);
   gcov_var.start = _GCOV_ftell (gcov_var.file) >> 2;
 }
 
Index: gcc/gcov-io.h
===================================================================
--- gcc/gcov-io.h	(revision 196314)
+++ gcc/gcov-io.h	(working copy)
@@ -802,7 +802,6 @@ struct gcov_info
 
   gcov_unsigned_t stamp;	/* uniquifying time stamp */
   const char *filename;		/* output file name */
-  gcov_unsigned_t eof_pos;      /* end position of profile data */
   gcov_merge_fn merge[GCOV_COUNTERS];  /* merge functions (null for
 					  unused) */
   
@@ -1011,7 +1010,8 @@ GCOV_LINKAGE const struct dyn_imp_mod **
 gcov_get_sorted_import_module_array (struct gcov_info *mod_info, unsigned
*len)
     ATTRIBUTE_HIDDEN;
 static void gcov_rewrite (void);
-GCOV_LINKAGE void gcov_seek (gcov_position_t /*position*/) ATTRIBUTE_HIDDEN;
+GCOV_LINKAGE void gcov_seek (gcov_position_t /*position*/, int /* from_end */)
+    ATTRIBUTE_HIDDEN;
 GCOV_LINKAGE void gcov_truncate (void) ATTRIBUTE_HIDDEN;
 GCOV_LINKAGE gcov_unsigned_t gcov_string_length (const char *)
ATTRIBUTE_HIDDEN;
 GCOV_LINKAGE unsigned gcov_gcda_file_size (struct gcov_info *);

--
This patch is available for review at http://codereview.appspot.com/7426043

[davidxl, 2013-02-28 01:10:38]

https://codereview.appspot.com/7426043/diff/1/gcc/gcov-io.h
File gcc/gcov-io.h (right):

https://codereview.appspot.com/7426043/diff/1/gcc/gcov-io.h#newcode1013
gcc/gcov-io.h:1013: GCOV_LINKAGE void gcov_seek (gcov_position_t /*position*/,
int /* from_end */)
I think it is better to create a new interface for seek_from_end instead of
changing the existing interface.

[tejohnson, 2013-02-28 06:08:45]

Patch updated based on comments. Added new gcov_seek_from_end method.
Passes regression tests and internal benchmark test.

Ok for google branches?
Thanks,
Teresa

2013-02-27  Teresa Johnson  <tejohnson@google.com>

	* gcc/coverage.c (build_info_type): Remove eof_pos from gcov_info.
	(build_info): Ditto.
	* gcc/gcov-io.c (gcov_seek_from_end): New function.
	* gcc/gcov-io.h (struct gcov_info): Remove eof_pos.
	* libgcc/libgcov.c (gcov_dump_module_info): Seek to end of
        file instead of relying on saved eof_pos.
	(gcov_merge_gcda_file): Remove saved eof_pos.
	(gcov_write_gcda_file): Ditto.

Index: gcc/coverage.c
===================================================================
--- gcc/coverage.c	(revision 196314)
+++ gcc/coverage.c	(working copy)
@@ -1961,12 +1961,6 @@ build_info_type (tree type, tree fn_info_ptr_type)
   DECL_CHAIN (field) = fields;
   fields = field;
 
-  /* eof_pos */
-  field = build_decl (BUILTINS_LOCATION, FIELD_DECL,
-                      NULL_TREE, get_gcov_unsigned_t ());
-  DECL_CHAIN (field) = fields;
-  fields = field;
-
   /* merge fn array */
   merge_fn_type
     = build_function_type_list (void_type_node,
@@ -2412,11 +2406,6 @@ build_info (tree info_type, tree fn_ary)
 				  filename_string));
   info_fields = DECL_CHAIN (info_fields);
 
-  /* eof_pos */
-  CONSTRUCTOR_APPEND_ELT (v1, info_fields,
-                          build_int_cstu (TREE_TYPE (info_fields), 0));
-  info_fields = DECL_CHAIN (info_fields);
-
   /* merge fn array -- NULL slots indicate unmeasured counters */
   merge_fn_type = TREE_TYPE (TREE_TYPE (info_fields));
   for (ix = 0; ix != GCOV_COUNTERS; ix++)
Index: gcc/gcov-io.c
===================================================================
--- gcc/gcov-io.c	(revision 196314)
+++ gcc/gcov-io.c	(working copy)
@@ -824,6 +824,19 @@ gcov_seek (gcov_position_t base)
   gcov_var.start = _GCOV_ftell (gcov_var.file) >> 2;
 }
 
+/* Move to the end minus the given number of 4-byte words.  */
+
+GCOV_LINKAGE void
+gcov_seek_from_end (gcov_position_t offset)
+{
+  int offset_in_bytes = offset << 2;
+  gcc_assert (gcov_var.mode < 0);
+  if (gcov_var.offset)
+    gcov_write_block (gcov_var.offset);
+  _GCOV_fseek (gcov_var.file, -offset_in_bytes, SEEK_END);
+  gcov_var.start = _GCOV_ftell (gcov_var.file) >> 2;
+}
+
 /* Truncate the gcov file at the current position.  */
 
 GCOV_LINKAGE void
Index: gcc/gcov-io.h
===================================================================
--- gcc/gcov-io.h	(revision 196314)
+++ gcc/gcov-io.h	(working copy)
@@ -391,6 +391,7 @@ typedef unsigned HOST_WIDEST_INT gcov_type_unsigne
 #define gcov_write_tag_length __gcov_write_tag_length
 #define gcov_position __gcov_position
 #define gcov_seek __gcov_seek
+#define gcov_seek_from_end __gcov_seek_from_end
 #define gcov_rewrite __gcov_rewrite
 #define gcov_truncate __gcov_truncate
 #define gcov_is_error __gcov_is_error
@@ -802,7 +803,6 @@ struct gcov_info
 
   gcov_unsigned_t stamp;	/* uniquifying time stamp */
   const char *filename;		/* output file name */
-  gcov_unsigned_t eof_pos;      /* end position of profile data */
   gcov_merge_fn merge[GCOV_COUNTERS];  /* merge functions (null for
 					  unused) */
   
@@ -925,10 +925,10 @@ extern struct gcov_var gcov_var;
    file either for reading or for writing. When reading a file you may
    use the gcov_read_* functions, gcov_sync, gcov_position, &
    gcov_error. When writing a file you may use the gcov_write
-   functions, gcov_seek & gcov_error. When a file is to be rewritten
-   you use the functions for reading, then gcov_rewrite then the
-   functions for writing.  Your file may become corrupted if you break
-   these invariants.  */
+   functions, gcov_seek, gcov_seek_from_end & gcov_error. When a file
+   is to be rewritten you use the functions for reading, then gcov_rewrite
+   then the functions for writing.  Your file may become corrupted if you
+   break these invariants.  */
 #if IN_LIBGCOV
 GCOV_LINKAGE int gcov_open (const char */*name*/) ATTRIBUTE_HIDDEN;
 #else
@@ -1012,6 +1012,8 @@ gcov_get_sorted_import_module_array (struct gcov_i
     ATTRIBUTE_HIDDEN;
 static void gcov_rewrite (void);
 GCOV_LINKAGE void gcov_seek (gcov_position_t /*position*/) ATTRIBUTE_HIDDEN;
+GCOV_LINKAGE void gcov_seek_from_end (gcov_position_t /*offset*/)
+    ATTRIBUTE_HIDDEN;
 GCOV_LINKAGE void gcov_truncate (void) ATTRIBUTE_HIDDEN;
 GCOV_LINKAGE gcov_unsigned_t gcov_string_length (const char *)
ATTRIBUTE_HIDDEN;
 GCOV_LINKAGE unsigned gcov_gcda_file_size (struct gcov_info *);
Index: libgcc/libgcov.c
===================================================================
--- libgcc/libgcov.c	(revision 196314)
+++ libgcc/libgcov.c	(working copy)
@@ -226,9 +226,6 @@ static struct gcov_summary program;
 /* Record the position of summary info.  */
 static gcov_position_t summary_pos = 0;
 
-/* Record the postion of eof.  */
-static gcov_position_t eof_pos = 0;
-
 /* Number of chars in prefix to be stripped.  */
 static int gcov_prefix_strip = 0;
 
@@ -580,7 +577,7 @@ gcov_dump_module_info (int do_lipo)
 
     /* Overwrite the zero word at the of the file.  */
     gcov_rewrite ();
-    gcov_seek (gi_ptr->eof_pos);
+    gcov_seek_from_end (1);
 
     gcov_write_module_infos (gi_ptr, do_lipo);
     /* Write the end marker  */
@@ -947,8 +944,8 @@ gcov_merge_gcda_file (struct gcov_info *gi_ptr)
   const struct gcov_fn_info *gfi_ptr;
   int error = 0;
   gcov_unsigned_t tag, length, version, stamp;
+  gcov_position_t eof_pos = 0;
 
-  eof_pos = 0;
   summary_pos = 0;
   sum_buffer = 0;
   sum_tail = &sum_buffer;
@@ -1202,7 +1199,6 @@ gcov_write_gcda_file (struct gcov_info *gi_ptr)
   const struct gcov_ctr_info *ci_ptr;
   unsigned t_ix, f_ix, n_counts, length;
   int error = 0;
-  gcov_position_t eof_pos1 = 0;
 
   /* Write out the data.  */
   gcov_seek (0);
@@ -1259,14 +1255,10 @@ gcov_write_gcda_file (struct gcov_info *gi_ptr)
             gcov_write_counter (*c_ptr++);
           ci_ptr++;
         }
-      eof_pos1 = gcov_position ();
     }
-    eof_pos = eof_pos1;
     /* Write the end marker  */
     gcov_write_unsigned (0);
 
-    gi_ptr->eof_pos = eof_pos;
-
     if ((error = gcov_close ()))
       gcov_error (error  < 0 ?
                   "profiling:%s:Overflow writing\n" :

--
This patch is available for review at http://codereview.appspot.com/7426043

[davidxl, 2013-02-28 06:33:08]

Looks good to me.