fstrcmp: memory is not reclaimed on exit

fstrcmp: memory is not reclaimed on exit

[Akim Demaille]

Hi!

There was a recent bug report about Bison not reclaiming all its
memory on exit (https://lists.gnu.org/r/bug-bison/2020-01/msg00006.html).
This is a not a leak, the memory is still reachable, yet we try to
stay clean on exit.  The traces from Valgrind are:

> 10. input.at:331: testing Symbol declarations ...
> ./input.at:369: COLUMNS=1000; export COLUMNS;  bison --color=no -fno-caret -Wno-other -S./dump-symbols.m4 input.y
> --- /dev/null	2019-11-30 17:07:08.851263387 +0000
> +++ /home/tkloczko/rpmbuild/BUILD/bison-3.5/tests/testsuite.dir/at-groups/10/stderr	2020-01-08 19:14:17.284197849 +0000
> @@ -0,0 +1,29 @@
> +==2837387== 320 bytes in 1 blocks are still reachable in loss record 1 of 1
> +==2837387==    at 0x483A80B: malloc (vg_replace_malloc.c:309)
> +==2837387==    by 0x117CFC: xmalloc (xmalloc.c:41)
> +==2837387==    by 0x11B743: UnknownInlinedFun (xalloc.h:103)
> +==2837387==    by 0x11B743: fstrcmp_bounded (fstrcmp.c:208)
> +==2837387==    by 0x142DED: UnknownInlinedFun (symtab.c:350)
> +==2837387==    by 0x142DED: UnknownInlinedFun (symtab.c:365)
> +==2837387==    by 0x142DED: UnknownInlinedFun (symtab.c:608)
> +==2837387==    by 0x142DED: UnknownInlinedFun (symtab.c:1017)
> +==2837387==    by 0x142DED: check_and_convert_grammar (reader.c:792)
> +==2837387==    by 0x1113E5: UnknownInlinedFun (reader.c:722)
> +==2837387==    by 0x1113E5: main (main.c:104)
> +==2837387== 
> +{
> +   <insert_a_suppression_name_here>
> +   Memcheck:Leak
> +   match-leak-kinds: reachable
> +   fun:malloc
> +   fun:xmalloc
> +   fun:UnknownInlinedFun
> +   fun:fstrcmp_bounded
> +   fun:UnknownInlinedFun
> +   fun:UnknownInlinedFun
> +   fun:UnknownInlinedFun
> +   fun:UnknownInlinedFun
> +   fun:check_and_convert_grammar
> +   fun:UnknownInlinedFun
> +   fun:main
> +}
> 10. input.at:331: 10. Symbol declarations (input.at:331): FAILED (input.at:369)

which points to fstrcmp's allocation of its per-thread internal buffer.

AFAICT, glthread takes care of reclaiming the memory when the thread
exits, that's the whole point of the "free" in the first line of the
body here:

> static void
> keys_init (void)
> {
>   gl_tls_key_init (buffer_key, free);
>   gl_tls_key_init (bufmax_key, NULL);
>   /* The per-thread initial values are NULL and 0, respectively.  */
> }

glthread/tls.h reads

>      Initialization:            gl_tls_key_init (name, destructor);
> 
>    A destructor is a function pointer of type 'void (*) (void *)', called
>    when a thread exits, and taking the last per-thread value as argument.  It
>    is unspecified whether the destructor function is called when the last
>    per-thread value is NULL.  On some platforms, the destructor function is
>    not called at all.

I can see that it's not expected to work on some platforms, but in the
case of the user the platform is fairly mainstream:

> ## --------- ##
> ## Platform. ##
> ## --------- ##
> 
> hostname = barrel
> uname -m = x86_64
> uname -r = 5.3.12-300.fc31.x86_64
> uname -s = Linux
> uname -v = #1 SMP Thu Nov 21 22:52:07 UTC 2019

So I don't know what to do.  Is this a red herring related to Valgrind
as a platform?  Is this something else?  I found nothing interesting
about "glthread valgrind" etc. on web, sorry if I missed something.

Thanks!

Re: fstrcmp: memory is not reclaimed on exit

[Bruno Haible]

Hi Akim,

> > +==2837387== 320 bytes in 1 blocks are still reachable in loss record 1 of 1
> > +==2837387==    at 0x483A80B: malloc (vg_replace_malloc.c:309)
> > +==2837387==    by 0x117CFC: xmalloc (xmalloc.c:41)
> > +==2837387==    by 0x11B743: UnknownInlinedFun (xalloc.h:103)
> > +==2837387==    by 0x11B743: fstrcmp_bounded (fstrcmp.c:208)
> 
> which points to fstrcmp's allocation of its per-thread internal buffer.

Correct. This is the per-thread buffer.

> AFAICT, glthread takes care of reclaiming the memory when the thread
> exits, that's the whole point of the "free" in the first line of the
> body here:
> 
> > static void
> > keys_init (void)
> > {
> >   gl_tls_key_init (buffer_key, free);
> >   gl_tls_key_init (bufmax_key, NULL);
> >   /* The per-thread initial values are NULL and 0, respectively.  */
> > }
> 
> glthread/tls.h reads
> 
> >      Initialization:            gl_tls_key_init (name, destructor);
> > 
> >    A destructor is a function pointer of type 'void (*) (void *)', called
> >    when a thread exits, and taking the last per-thread value as argument.  It
> >    is unspecified whether the destructor function is called when the last
> >    per-thread value is NULL.  On some platforms, the destructor function is
> >    not called at all.
> 
> I can see that it's not expected to work on some platforms, but in the
> case of the user the platform is fairly mainstream:

Meanwhile this destructor stuff even works on native Windows. The list of
platforms where it does not work is very small (something like mingw with
winpthreads, IIRC).

> So I don't know what to do.  Is this a red herring related to Valgrind
> as a platform?

Do the threads still exist at the moment valgrind does its inventory of left-
over memory? In particular:
  - Did you create threads, in which fstrcmp is run? If yes, are they still
    running?
  - 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).

Bruno

Re: fstrcmp: memory is not reclaimed on exit

[Akim Demaille]

Hi Bruno,

> Le 20 janv. 2020 à 22:57, Bruno Haible <bruno@clisp.org> a écrit :
> 
>>>     Initialization:            gl_tls_key_init (name, destructor);
>>> 
>>>   A destructor is a function pointer of type 'void (*) (void *)', called
>>>   when a thread exits, and taking the last per-thread value as argument.  It
>>>   is unspecified whether the destructor function is called when the last
>>>   per-thread value is NULL.  On some platforms, the destructor function is
>>>   not called at all.
>> 
>> I can see that it's not expected to work on some platforms, but in the
>> case of the user the platform is fairly mainstream:
> 
> Meanwhile this destructor stuff even works on native Windows. The list of
> platforms where it does not work is very small (something like mingw with
> winpthreads, IIRC).

Great!

>> So I don't know what to do.  Is this a red herring related to Valgrind
>> as a platform?
> 
> 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.

>  - 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?  I feel
stupid to initialize the memory right before releasing, but I didn't
find a means to check whether the tls memory was initialized.

Thanks!

commit eee1a395a841f7d1ae4388710c88c5dd3e047cc0
Author: Akim Demaille <akim.demaille@gmail.com>
Date:   Wed Jan 22 07:46:45 2020 +0100

    fstrcmp: provide a means to explictly release resources
    
    * lib/fstrcmp.h, lib/fstrcmp.c (fstrcmp_free): New.

diff --git a/ChangeLog b/ChangeLog
index 53a1fb435..10626e535 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-01-22  Akim Demaille  <akim@lrde.epita.fr>
+
+	fstrcmp: provide a means to explictly release resources.
+	* lib/fstrcmp.h, lib/fstrcmp.c (fstrcmp_free): New.
+
 2020-01-09  Bruno Haible  <bruno@clisp.org>
 
 	c32srtombs: Add tests.
diff --git a/lib/fstrcmp.c b/lib/fstrcmp.c
index c6a66389e..e8e02e856 100644
--- a/lib/fstrcmp.c
+++ b/lib/fstrcmp.c
@@ -73,6 +73,13 @@ keys_init (void)
 /* Ensure that keys_init is called once only.  */
 gl_once_define(static, keys_init_once)
 
+void
+fstrcmp_free (void)
+{
+  gl_once (keys_init_once, keys_init);
+  gl_tls_key_destroy (buffer_key);
+  gl_tls_key_destroy (bufmax_key);
+}
 
 /* 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 92b67e34a..f3a57ecb6 100644
--- a/lib/fstrcmp.h
+++ b/lib/fstrcmp.h
@@ -38,6 +38,13 @@ 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)
 
+/* Explicitly release resources acquired in this thread.  Regular
+   thread termination invokes it automatically, so it is typically not
+   needed to call it.  However, in the case of the main thread, tools
+   such as Valgrind might report "leaks" if the memory is not
+   explicitly reclaimed first.  */
+extern void fstrcmp_free (void);
+
 #ifdef __cplusplus
 }
 #endif

Re: fstrcmp: memory is not reclaimed on exit

[Akim Demaille]

Hi all,

> Le 22 janv. 2020 à 07:50, Akim Demaille <akim@lrde.epita.fr> a écrit :
> 
> 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?  I feel
> stupid to initialize the memory right before releasing, but I didn't
> find a means to check whether the tls memory was initialized.
> 
> Thanks!
> 
> commit eee1a395a841f7d1ae4388710c88c5dd3e047cc0
> Author: Akim Demaille <akim.demaille@gmail.com>
> Date:   Wed Jan 22 07:46:45 2020 +0100
> 
>    fstrcmp: provide a means to explictly release resources
> 
>    * lib/fstrcmp.h, lib/fstrcmp.c (fstrcmp_free): New.

Do we have a better alternative to address this issue?  Is it ok to install?

Cheers!

Re: fstrcmp: memory is not reclaimed on exit

[Bruno Haible]

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

Re: fstrcmp: memory is not reclaimed on exit

[Jeffrey Walton]

On Sun, Feb 16, 2020 at 7:53 AM Bruno Haible <bruno@clisp.org> wrote:
>
> ...
> > 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.

The coding standard is wrong in this area. The practice breaks testing.

Every time someone brings up failed acceptance tests or audits someone
quotes the coding standard saying its OK.

What more evidence do you need to realize the coding standard is broken?

Jeff

Re: fstrcmp: memory is not reclaimed on exit

[Paul Eggert]

On 2/16/20 12:44 PM, Jeffrey Walton wrote:
> The coding standard is wrong in this area. The practice breaks testing.

Not really. It breaks some tests, which are arguably testing the wrong thing. 
Any tests that check for whether all memory is freed just before exit, are 
testing for a property that the GNU Coding Standards say is not a desirable 
property.

The underlying issue is whether we should complicate code to simplify tests, or 
vice versa. The GCS say that in this area we should keep the code simple, 
possibly at the expense of the tests. This is not inherently the wrong decision; 
it's merely choosing a point in a design space that involves simplicity, 
efficiency, correctness, testability, etc. and where there are several 
reasonable choices.

Re: fstrcmp: memory is not reclaimed on exit

[Bruno Haible]

Paul Eggert wrote:
> it's merely choosing a point in a design space that involves simplicity, 
> efficiency, correctness, testability, etc. and where there are several 
> reasonable choices.

Right. In the case of fstrcmp, the bison tests use 'valgrind -q'. Akim could
use 'valgrind -q --leak-check=no'; then there would be no need to add a
cleanup to fstrcmp.c. However, I think that 'valgrind -q --leak-check=no'
would be a lot less useful for the day-to-day maintenance of bison than
'valgrind -q'. Therefore I think Akim made a reasonable choice here -
although it differs from the equally reasonable broader guideline in the GCS.

Ideally, valgrind would have a way to distinguish static memory allocations
from leaks (where by "leak" I mean an allocation that would accumulate when
the code is run repeatedly). How could such a distinction be implemented,
without multiplying the time needed to run the test by a factor of 100 or 1000?
Possibly define a function 'static_malloc' that is equivalent to 'malloc'
in glibc, but handled differently inside valgrind?

Bruno

Re: static allocations vs. memory leaks

[Bruno Haible]

> Ideally, valgrind would have a way to distinguish static memory allocations
> from leaks (where by "leak" I mean an allocation that would accumulate when
> the code is run repeatedly). How could such a distinction be implemented,
> without multiplying the time needed to run the test by a factor of 100 or 1000?
> Possibly define a function 'static_malloc' that is equivalent to 'malloc'
> in glibc, but handled differently inside valgrind?

Thinking more about it: How about a statically allocated hash table (or other
complex data structure)? We don't want to complicate the code of the hash
table with the option to choose a different allocator.

We would need primitive functions begin/end_static_allocation() that tell
valgrind to ignore the allocations between begin_static_allocation() and
end_static_allocation().

And it would need to nest. Example:

   malloc (10);
   begin_static_allocation ();
   malloc (100);
   begin_static_allocation ();
   malloc (1000);
   end_static_allocation ();
   malloc (100);
   end_static_allocation ();
   malloc (1000000);

valgrind would report two memory allocations: 10 and 1000000.

Bruno

Re: fstrcmp: memory is not reclaimed on exit

[Akim Demaille]

Hi Bruno,

> Le 16 févr. 2020 à 13:52, Bruno Haible <bruno@clisp.org> a écrit :
> 
> Hi Akim,
> 
> Sorry for the delay.

You look busy :)


>> +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.

Thanks for the details.  The main thread is really not like the others.

> 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.

It looks good to me, thanks!