[stage1][PATCH 0/3] __gcov_dump improvements

[stage1][PATCH 0/3] __gcov_dump improvements

[Martin Liska]

Hi.

The following mini patch series improves:
- gcov run-time locking is used for user accessible __gcov_dump and __gcov_reset functions
- do not run __gcov_flush in fork, only __gcov_reset is called in child process
- gcov exec* wrappers dump counters and reset only if execv* fails

Patch can bootstrap on x86_64-linux-gnu and survives regression tests. 
I'll install the patch set in next stage1 if there are no objections.

Thanks,
Martin

Martin Liska (3):
  Do locking for __gcov_dump and __gcov_reset as well.
  Use __gcov_dump and __gcov_reset in execv and fork context.
  Remove __gcov_flush.

 gcc/tree-cfg.c             |   4 +-
 libgcc/Makefile.in         |   5 +-
 libgcc/gcov.h              |   5 --
 libgcc/libgcov-interface.c | 126 +++++++++++++++++++++++--------------
 libgcc/libgcov.h           |   6 ++
 5 files changed, 91 insertions(+), 55 deletions(-)

-- 
2.26.0

[PATCH 1/3] Do locking for __gcov_dump and __gcov_reset as well.

[Martin Liska]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]


libgcc/ChangeLog:

2020-04-03  Martin Liska  <mliska@suse.cz>

	PR gcov-profile/93623
	* Makefile.in: Add _gcov_lock_unlock to LIBGCOV_INTERFACE.
	* libgcov-interface.c (ALIAS_void_fn): Remove.
	(__gcov_lock): New.
	(__gcov_unlock): New.
	(__gcov_flush): Use __gcov_lock and __gcov_unlock.
	(__gcov_reset): Likewise.
	(__gcov_dump): Likewise.
	* libgcov.h (__gcov_lock): New declaration.
	(__gcov_unlock): Likewise.
---
 libgcc/Makefile.in         |  3 +-
 libgcc/libgcov-interface.c | 59 ++++++++++++++++++++++++++++----------
 libgcc/libgcov.h           |  6 ++++
 3 files changed, 52 insertions(+), 16 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-locking-for-__gcov_dump-and-__gcov_reset-as-well.patch --]
[-- Type: text/x-patch; name="0001-Do-locking-for-__gcov_dump-and-__gcov_reset-as-well.patch", Size: 3572 bytes --]

diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
index 851e7657d07..e6ed153abbc 100644
--- a/libgcc/Makefile.in
+++ b/libgcc/Makefile.in
@@ -906,7 +906,8 @@ LIBGCOV_PROFILER = _gcov_interval_profiler				\
 	_gcov_time_profiler
 LIBGCOV_INTERFACE = _gcov_dump _gcov_flush _gcov_fork			\
 	_gcov_execl _gcov_execlp					\
-	_gcov_execle _gcov_execv _gcov_execvp _gcov_execve _gcov_reset
+	_gcov_execle _gcov_execv _gcov_execvp _gcov_execve _gcov_reset  \
+	_gcov_lock_unlock
 LIBGCOV_DRIVER = _gcov
 
 libgcov-merge-objects = $(patsubst %,%$(objext),$(LIBGCOV_MERGE))
diff --git a/libgcc/libgcov-interface.c b/libgcc/libgcov-interface.c
index 048b9029ff3..a8054edba57 100644
--- a/libgcc/libgcov-interface.c
+++ b/libgcc/libgcov-interface.c
@@ -42,18 +42,9 @@ void __gcov_dump (void) {}
 
 #else
 
-/* Some functions we want to bind in this dynamic object, but have an
-   overridable global alias.  Unfortunately not all targets support
-   aliases, so we just have a forwarding function.  That'll be tail
-   called, so the cost is a single jump instruction.*/
-
-#define ALIAS_void_fn(src,dst) \
-  void dst (void)	    \
-  { src (); }
-
 extern __gthread_mutex_t __gcov_flush_mx ATTRIBUTE_HIDDEN;
 
-#ifdef L_gcov_flush
+#ifdef L_gcov_lock_unlock
 #ifdef __GTHREAD_MUTEX_INIT
 __gthread_mutex_t __gcov_flush_mx = __GTHREAD_MUTEX_INIT;
 #define init_mx_once()
@@ -74,6 +65,25 @@ init_mx_once (void)
 }
 #endif
 
+/* Lock critical section for __gcov_dump and __gcov_reset functions.  */
+
+void
+__gcov_lock (void)
+{
+  init_mx_once ();
+  __gthread_mutex_lock (&__gcov_flush_mx);
+}
+
+/* Unlock critical section for __gcov_dump and __gcov_reset functions.  */
+
+void
+__gcov_unlock (void)
+{
+  __gthread_mutex_unlock (&__gcov_flush_mx);
+}
+#endif
+
+#ifdef L_gcov_flush
 /* Called before fork or exec - write out profile information gathered so
    far and reset it to zero.  This avoids duplication or loss of the
    profile information gathered so far.  */
@@ -81,13 +91,12 @@ init_mx_once (void)
 void
 __gcov_flush (void)
 {
-  init_mx_once ();
-  __gthread_mutex_lock (&__gcov_flush_mx);
+  __gcov_lock ();
 
   __gcov_dump_int ();
   __gcov_reset_int ();
 
-  __gthread_mutex_unlock (&__gcov_flush_mx);
+  __gcov_unlock ();
 }
 
 #endif /* L_gcov_flush */
@@ -143,7 +152,17 @@ __gcov_reset_int (void)
     }
 }
 
-ALIAS_void_fn (__gcov_reset_int, __gcov_reset);
+/* Exported function __gcov_reset.  */
+
+void
+__gcov_reset (void)
+{
+  __gcov_lock ();
+
+  __gcov_reset_int ();
+
+  __gcov_unlock ();
+}
 
 #endif /* L_gcov_reset */
 
@@ -163,7 +182,17 @@ __gcov_dump_int (void)
     __gcov_dump_one (root);
 }
 
-ALIAS_void_fn (__gcov_dump_int, __gcov_dump);
+/* Exported function __gcov_dump.  */
+
+void
+__gcov_dump (void)
+{
+  __gcov_lock ();
+
+  __gcov_dump_int ();
+
+  __gcov_unlock ();
+}
 
 #endif /* L_gcov_dump */
 
diff --git a/libgcc/libgcov.h b/libgcc/libgcov.h
index 023293e05ec..104b80bdcbb 100644
--- a/libgcc/libgcov.h
+++ b/libgcc/libgcov.h
@@ -253,6 +253,12 @@ extern void __gcov_reset_int (void) ATTRIBUTE_HIDDEN;
 /* User function to enable early write of profile information so far.  */
 extern void __gcov_dump_int (void) ATTRIBUTE_HIDDEN;
 
+/* Lock critical section for __gcov_dump and __gcov_reset functions.  */
+extern void __gcov_lock (void) ATTRIBUTE_HIDDEN;
+
+/* Unlock critical section for __gcov_dump and __gcov_reset functions.  */
+extern void __gcov_unlock (void) ATTRIBUTE_HIDDEN;
+
 /* The merge function that just sums the counters.  */
 extern void __gcov_merge_add (gcov_type *, unsigned) ATTRIBUTE_HIDDEN;
 

[PATCH 2/3] Use __gcov_dump and __gcov_reset in execv and fork context.

[Martin Liska]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]


libgcc/ChangeLog:

2020-04-03  Martin Liska  <mliska@suse.cz>

	PR gcov-profile/93623
	* libgcov-interface.c (__gcov_fork): Do not flush
	and reset only in child process.
	(__gcov_execl): Dump counters only and reset them
	only if exec* fails.
	(__gcov_execlp): Likewise.
	(__gcov_execle): Likewise.
	(__gcov_execv): Likewise.
	(__gcov_execvp): Likewise.
	(__gcov_execve): Likewise.
---
 libgcc/libgcov-interface.c | 59 +++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 16 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Use-__gcov_dump-and-__gcov_reset-in-execv-and-fork-c.patch --]
[-- Type: text/x-patch; name="0002-Use-__gcov_dump-and-__gcov_reset-in-execv-and-fork-c.patch", Size: 3883 bytes --]

diff --git a/libgcc/libgcov-interface.c b/libgcc/libgcov-interface.c
index a8054edba57..855e8612018 100644
--- a/libgcc/libgcov-interface.c
+++ b/libgcc/libgcov-interface.c
@@ -197,17 +197,20 @@ __gcov_dump (void)
 #endif /* L_gcov_dump */
 
 #ifdef L_gcov_fork
-/* A wrapper for the fork function.  Flushes the accumulated profiling data, so
-   that they are not counted twice.  */
+/* A wrapper for the fork function.  We reset counters in the child
+   so that they are not counted twice.  */
 
 pid_t
 __gcov_fork (void)
 {
   pid_t pid;
-  __gcov_flush ();
   pid = fork ();
   if (pid == 0)
-    __GTHREAD_MUTEX_INIT_FUNCTION (&__gcov_flush_mx);
+    {
+      __GTHREAD_MUTEX_INIT_FUNCTION (&__gcov_flush_mx);
+      /* We do not need locking as we are the only thread in the child.  */
+      __gcov_reset_int ();
+    }
   return pid;
 }
 #endif
@@ -223,7 +226,8 @@ __gcov_execl (const char *path, char *arg, ...)
   unsigned i, length;
   char **args;
 
-  __gcov_flush ();
+  /* Dump counters only, they will be lost after exec.  */
+  __gcov_dump ();
 
   va_start (ap, arg);
   va_copy (aq, ap);
@@ -239,7 +243,10 @@ __gcov_execl (const char *path, char *arg, ...)
     args[i] = va_arg (aq, char *);
   va_end (aq);
 
-  return execv (path, args);
+  int ret = execv (path, args);
+  /* We reach this code only when execv fails, reset counter then here.  */
+  __gcov_reset ();
+  return ret;
 }
 #endif
 
@@ -254,7 +261,8 @@ __gcov_execlp (const char *path, char *arg, ...)
   unsigned i, length;
   char **args;
 
-  __gcov_flush ();
+  /* Dump counters only, they will be lost after exec.  */
+  __gcov_dump ();
 
   va_start (ap, arg);
   va_copy (aq, ap);
@@ -270,7 +278,10 @@ __gcov_execlp (const char *path, char *arg, ...)
     args[i] = va_arg (aq, char *);
   va_end (aq);
 
-  return execvp (path, args);
+  int ret = execvp (path, args);
+  /* We reach this code only when execv fails, reset counter then here.  */
+  __gcov_reset ();
+  return ret;
 }
 #endif
 
@@ -286,7 +297,8 @@ __gcov_execle (const char *path, char *arg, ...)
   char **args;
   char **envp;
 
-  __gcov_flush ();
+  /* Dump counters only, they will be lost after exec.  */
+  __gcov_dump ();
 
   va_start (ap, arg);
   va_copy (aq, ap);
@@ -303,7 +315,10 @@ __gcov_execle (const char *path, char *arg, ...)
   envp = va_arg (aq, char **);
   va_end (aq);
 
-  return execve (path, args, envp);
+  int ret = execve (path, args, envp);
+  /* We reach this code only when execv fails, reset counter then here.  */
+  __gcov_reset ();
+  return ret;
 }
 #endif
 
@@ -314,8 +329,12 @@ __gcov_execle (const char *path, char *arg, ...)
 int
 __gcov_execv (const char *path, char *const argv[])
 {
-  __gcov_flush ();
-  return execv (path, argv);
+  /* Dump counters only, they will be lost after exec.  */
+  __gcov_dump ();
+  int ret = execv (path, argv);
+  /* We reach this code only when execv fails, reset counter then here.  */
+  __gcov_reset ();
+  return ret;
 }
 #endif
 
@@ -326,8 +345,12 @@ __gcov_execv (const char *path, char *const argv[])
 int
 __gcov_execvp (const char *path, char *const argv[])
 {
-  __gcov_flush ();
-  return execvp (path, argv);
+  /* Dump counters only, they will be lost after exec.  */
+  __gcov_dump ();
+  int ret = execvp (path, argv);
+  /* We reach this code only when execv fails, reset counter then here.  */
+  __gcov_reset ();
+  return ret;
 }
 #endif
 
@@ -338,8 +361,12 @@ __gcov_execvp (const char *path, char *const argv[])
 int
 __gcov_execve (const char *path, char *const argv[], char *const envp[])
 {
-  __gcov_flush ();
-  return execve (path, argv, envp);
+  /* Dump counters only, they will be lost after exec.  */
+  __gcov_dump ();
+  int ret = execve (path, argv, envp);
+  /* We reach this code only when execv fails, reset counter then here.  */
+  __gcov_reset ();
+  return ret;
 }
 #endif
 #endif /* inhibit_libc */

[PATCH 3/3] Remove __gcov_flush.

[Martin Liska]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]


gcc/ChangeLog:

2020-04-03  Martin Liska  <mliska@suse.cz>

	PR gcov-profile/93623
	* tree-cfg.c (stmt_can_terminate_bb_p): Update comment to reflect
	reality.

libgcc/ChangeLog:

2020-04-03  Martin Liska  <mliska@suse.cz>

	PR gcov-profile/93623
	* Makefile.in: Remove __gcov_flush.
	* gcov.h (__gcov_flush): Remove.
	* libgcov-interface.c (__gcov_flush): Remove.
	(init_mx): Use renamed mutex.
	(__gcov_lock): Likewise.
	(__gcov_unlock): Likewise.
	(__gcov_fork): Likewise.
	(__gcov_flush): Remove.
---
 gcc/tree-cfg.c             |  4 ++--
 libgcc/Makefile.in         |  2 +-
 libgcc/gcov.h              |  5 -----
 libgcc/libgcov-interface.c | 36 +++++++-----------------------------
 4 files changed, 10 insertions(+), 37 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-Remove-__gcov_flush.patch --]
[-- Type: text/x-patch; name="0003-Remove-__gcov_flush.patch", Size: 3761 bytes --]

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index e99fb9ff5d1..b21ef0eee37 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -8439,8 +8439,8 @@ stmt_can_terminate_bb_p (gimple *t)
       && (call_flags & ECF_NOTHROW)
       && !(call_flags & ECF_RETURNS_TWICE)
       /* fork() doesn't really return twice, but the effect of
-         wrapping it in __gcov_fork() which calls __gcov_flush()
-	 and clears the counters before forking has the same
+	 wrapping it in __gcov_fork() which calls __gcov_dump() and
+	 __gcov_reset() and clears the counters before forking has the same
 	 effect as returning twice.  Force a fake edge.  */
       && !fndecl_built_in_p (fndecl, BUILT_IN_FORK))
     return false;
diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
index e6ed153abbc..5c50f9fe4df 100644
--- a/libgcc/Makefile.in
+++ b/libgcc/Makefile.in
@@ -904,7 +904,7 @@ LIBGCOV_PROFILER = _gcov_interval_profiler				\
 	_gcov_ior_profiler_atomic					\
 	_gcov_indirect_call_profiler_v4					\
 	_gcov_time_profiler
-LIBGCOV_INTERFACE = _gcov_dump _gcov_flush _gcov_fork			\
+LIBGCOV_INTERFACE = _gcov_dump _gcov_fork				\
 	_gcov_execl _gcov_execlp					\
 	_gcov_execle _gcov_execv _gcov_execvp _gcov_execve _gcov_reset  \
 	_gcov_lock_unlock
diff --git a/libgcc/gcov.h b/libgcc/gcov.h
index f1581914dde..0e3eed31032 100644
--- a/libgcc/gcov.h
+++ b/libgcc/gcov.h
@@ -33,9 +33,4 @@ extern void __gcov_reset (void);
 
 extern void __gcov_dump (void);
 
-/* Write profile information to a file and reset counters to zero.
-   The function does operations under a mutex.  */
-
-extern void __gcov_flush (void);
-
 #endif /* GCC_GCOV_H */
diff --git a/libgcc/libgcov-interface.c b/libgcc/libgcov-interface.c
index 855e8612018..3a8a5bf44b8 100644
--- a/libgcc/libgcov-interface.c
+++ b/libgcc/libgcov-interface.c
@@ -28,10 +28,6 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 #if defined(inhibit_libc)
 
-#ifdef L_gcov_flush
-void __gcov_flush (void) {}
-#endif
-
 #ifdef L_gcov_reset
 void __gcov_reset (void) {}
 #endif
@@ -42,19 +38,19 @@ void __gcov_dump (void) {}
 
 #else
 
-extern __gthread_mutex_t __gcov_flush_mx ATTRIBUTE_HIDDEN;
+extern __gthread_mutex_t __gcov_mx ATTRIBUTE_HIDDEN;
 
 #ifdef L_gcov_lock_unlock
 #ifdef __GTHREAD_MUTEX_INIT
-__gthread_mutex_t __gcov_flush_mx = __GTHREAD_MUTEX_INIT;
+__gthread_mutex_t __gcov_mx = __GTHREAD_MUTEX_INIT;
 #define init_mx_once()
 #else
-__gthread_mutex_t __gcov_flush_mx;
+__gthread_mutex_t __gcov_mx;
 
 static void
 init_mx (void)
 {
-  __GTHREAD_MUTEX_INIT_FUNCTION (&__gcov_flush_mx);
+  __GTHREAD_MUTEX_INIT_FUNCTION (&__gcov_mx);
 }
 
 static void
@@ -71,7 +67,7 @@ void
 __gcov_lock (void)
 {
   init_mx_once ();
-  __gthread_mutex_lock (&__gcov_flush_mx);
+  __gthread_mutex_lock (&__gcov_mx);
 }
 
 /* Unlock critical section for __gcov_dump and __gcov_reset functions.  */
@@ -79,28 +75,10 @@ __gcov_lock (void)
 void
 __gcov_unlock (void)
 {
-  __gthread_mutex_unlock (&__gcov_flush_mx);
+  __gthread_mutex_unlock (&__gcov_mx);
 }
 #endif
 
-#ifdef L_gcov_flush
-/* Called before fork or exec - write out profile information gathered so
-   far and reset it to zero.  This avoids duplication or loss of the
-   profile information gathered so far.  */
-
-void
-__gcov_flush (void)
-{
-  __gcov_lock ();
-
-  __gcov_dump_int ();
-  __gcov_reset_int ();
-
-  __gcov_unlock ();
-}
-
-#endif /* L_gcov_flush */
-
 #ifdef L_gcov_reset
 
 /* Reset all counters to zero.  */
@@ -207,7 +185,7 @@ __gcov_fork (void)
   pid = fork ();
   if (pid == 0)
     {
-      __GTHREAD_MUTEX_INIT_FUNCTION (&__gcov_flush_mx);
+      __GTHREAD_MUTEX_INIT_FUNCTION (&__gcov_mx);
       /* We do not need locking as we are the only thread in the child.  */
       __gcov_reset_int ();
     }