Re: fstrcmp: memory is not reclaimed on exit

[Bruno Haible <bruno@clisp.org>]

Hi Akim,

Sorry for the delay.

> > Do the threads still exist at the moment valgrind does its inventory of left-
> > over memory?
> 
> I don't know when it runs its stuff, but I expect, given where it stands,
> that it does it as the latest possible instant.
> 
> > In particular:
> >  - Did you create threads, in which fstrcmp is run? If yes, are they still
> >    running?
> 
> No, Bison does not use any threads.

OK, then valgrind is really complaining about the memory allocations done in the
main thread.

> >  - Or did you run fstrcmp in the main thread? Most likely valgrind does its
> >    inventory in the main thread, during exit(). This means that at this point
> >    the fstrcmp buffer for the main thread still exists. In other words, you
> >    should treat thread-local memory allocations for the main thread like
> >    static memory allocations (e.g. like in uniqstr.c).
> 
> I agree, I would like to be able to explicitly release the memory.  But
> I can't see any API to do that in fstrcmp.c.  Is this one ok?

The GNU Coding Standards [1] tell us to not worry about this situation. However,
the alternative - to check for a memory leak - would be to run the test in a
loop (e.g. 1000 times), which is not practical since bison tests take some
noticeable time to execute already when executed once. Therefore, I'm OK to
look at a workaround.

> +void
> +fstrcmp_free (void)
> +{
> +  gl_once (keys_init_once, keys_init);
> +  gl_tls_key_destroy (buffer_key);
> +  gl_tls_key_destroy (bufmax_key);
> +}

This workaround is insufficient, since POSIX [2] says that
   "It is the responsibility of the application to free any application
    storage or perform any cleanup actions for data structures related
    to the deleted key or associated thread-specific data in any threads"

In other words, pthread_key_delete is not guaranteed to call the destructor
of 'buffer_key'. The gnulib test (tests/test-tls.c functions test_tls_dtorcheck1
and test_tls_dtorcheck2) verifies that the destructor gets called, but only
for threads != main thread, and you are interested in the main thread
particularly. Most likely, in this test, the destructor gets called when the
thread exits [3], not when pthread_key_delete gets called.

[1] https://www.gnu.org/prep/standards/html_node/Memory-Usage.html
[2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_key_delete.html
[3] https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_key_create.html

This patch, however, should work.


2020-02-16  Bruno Haible  <bruno@clisp.org>

	fstrcmp: Add API to clean up resources.
	Reported by Akim Demaille <akim@lrde.epita.fr> in
	<https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00080.html>.
	* lib/fstrcmp.h (fstrcmp_free_resources): New declaration.
	* lib/fstrcmp.c (fstrcmp_free_resources): New function.

diff --git a/lib/fstrcmp.c b/lib/fstrcmp.c
index c6a6638..1a4fbfd 100644
--- a/lib/fstrcmp.c
+++ b/lib/fstrcmp.c
@@ -73,6 +73,21 @@ keys_init (void)
 /* Ensure that keys_init is called once only.  */
 gl_once_define(static, keys_init_once)
 
+void
+fstrcmp_free_resources (void)
+{
+  ptrdiff_t *buffer;
+
+  gl_once (keys_init_once, keys_init);
+  buffer = gl_tls_get (buffer_key);
+  if (buffer != NULL)
+    {
+      gl_tls_set (buffer_key, NULL);
+      gl_tls_set (bufmax_key, (void *) (uintptr_t) 0);
+      free (buffer);
+    }
+}
+
 
 /* In the code below, branch probabilities were measured by Ralf Wildenhues,
    by running "msgmerge LL.po coreutils.pot" with msgmerge 0.18 for many
diff --git a/lib/fstrcmp.h b/lib/fstrcmp.h
index 92b67e3..37df588 100644
--- a/lib/fstrcmp.h
+++ b/lib/fstrcmp.h
@@ -38,6 +38,15 @@ extern double fstrcmp_bounded (const char *s1, const char *s2,
 /* A shortcut for fstrcmp.  Avoids a function call.  */
 #define fstrcmp(s1,s2) fstrcmp_bounded (s1, s2, 0.0)
 
+/* Frees the per-thread resources allocated by this module for the current
+   thread.
+   You don't need to call this function in threads other than the main thread,
+   because per-thread resources are reclaimed automatically when the thread
+   exits.  However, per-thread resources allocated by the main thread are
+   comparable to static allocations; calling this function can be useful to
+   avoid an error report from valgrind.  */
+extern void fstrcmp_free_resources (void);
+
 #ifdef __cplusplus
 }
 #endif