[Bug target/113401] New: Memory (resource) leak in -ftrampoline-impl=heap

[Bug target/113401] New: Memory (resource) leak in -ftrampoline-impl=heap

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113401

            Bug ID: 113401
           Summary: Memory (resource) leak in -ftrampoline-impl=heap
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: fw at gcc dot gnu.org
  Target Milestone: ---
            Target: x86_64-*, aarch64-*

Consider this test program:

#include <err.h>
#include <errno.h>
#include <pthread.h>

static void *volatile compiler_barrier;

void *
thread_routine (void *ignore)
{
  int local_variable;
  auto int *local_function (void)
  {
    return &local_variable;
  }
  compiler_barrier = local_function;
  return NULL;
}

int
main (void)
{
  for (int i = 0; i < 10000; ++i)
    {
      pthread_t thread;
      int ret = pthread_create (&thread, NULL, thread_routine, NULL);
      if (ret != 0)
        {
          errno = ret;
          err (1, "pthread_create");
        }
      ret = pthread_join (thread, NULL);
      if (ret != 0)
        {
          errno = ret;
          err (1, "pthread_join");
        }
    }
}

With -ftrampoline-impl=stack, I get:

$ \time ./a.out 
0.01user 0.16system 0:00.18elapsed 102%CPU (0avgtext+0avgdata 3008maxresident)k
0inputs+0outputs (0major+68minor)pagefaults 0swaps

With -ftrampoline-impl=heap:

$ \time ./a.out 
0.03user 0.17system 0:00.20elapsed 101%CPU (0avgtext+0avgdata
41796maxresident)k
0inputs+0outputs (0major+10148minor)pagefaults 0swaps

The reason for the maxresident change is that __builtin_nested_func_ptr_created
and __builtin_nested_func_ptr_deleted cache the last trampoline page even if it
is completely unused. This is required for performance reasons. The fix is to
register a TLS destructor to deallocate that page, too. On glibc, that also
fixes another memory leak for -fno-exceptions compilations (the default for C)
if pthread_exit is called with an active trampoline.

[Bug libgcc/113401] Memory (resource) leak in -ftrampoline-impl=heap

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113401

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |iains at gcc dot gnu.org

--- Comment #1 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #0)

> The fix is to register a TLS destructor to
> deallocate that page, too. On glibc, that also fixes another memory leak for
> -fno-exceptions compilations (the default for C) if pthread_exit is called
> with an active trampoline.

Does this mean you have a proposed patch already? (before I start
investigation)

[Bug libgcc/113401] Memory (resource) leak in -ftrampoline-impl=heap

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113401

--- Comment #2 from Florian Weimer <fw at gcc dot gnu.org> ---
(In reply to Iain Sandoe from comment #1)
> (In reply to Florian Weimer from comment #0)
> 
> > The fix is to register a TLS destructor to
> > deallocate that page, too. On glibc, that also fixes another memory leak for
> > -fno-exceptions compilations (the default for C) if pthread_exit is called
> > with an active trampoline.
> 
> Does this mean you have a proposed patch already? (before I start
> investigation)

No, this was a reference to __cxa_thread_atexit, which is unfortunately in
libstdc++ (but is a thin shim around __cxa_thread_atexit_impl for current
glibc). The pthread_key_create fallback probably needs to be duplicated into
libgcc_s.

[Bug libgcc/113401] Memory (resource) leak in -ftrampoline-impl=heap

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113401

--- Comment #3 from Iain Sandoe <iains at gcc dot gnu.org> ---
for platforms using pthreads as the underlying resource, then perhaps we can do
this without thread_atexit (which I do not see in many places) by using
pthread_cleanup_push ()

not sure if gthr.h abstracts that in any way (I do not have a patch at the
moment).

[Bug libgcc/113401] Memory (resource) leak in -ftrampoline-impl=heap

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113401

--- Comment #4 from Florian Weimer <fw at gcc dot gnu.org> ---
(In reply to Iain Sandoe from comment #3)
> for platforms using pthreads as the underlying resource, then perhaps we can
> do this without thread_atexit (which I do not see in many places) by using
> pthread_cleanup_push ()

The current implementation already uses the same underlying mechanism as
pthread_cleanup_push if building with -fexceptions. It does not solve the leak
because the outermost handler deliberately does not perform a full deallocation
of the thread state.

[Bug libgcc/113401] Memory (resource) leak in -ftrampoline-impl=heap

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113401

--- Comment #5 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #4)
> (In reply to Iain Sandoe from comment #3)
> > for platforms using pthreads as the underlying resource, then perhaps we can
> > do this without thread_atexit (which I do not see in many places) by using
> > pthread_cleanup_push ()
> 
> The current implementation already uses the same underlying mechanism as
> pthread_cleanup_push if building with -fexceptions. It does not solve the
> leak because the outermost handler deliberately does not perform a full
> deallocation of the thread state.

hmm.. I'm slightly confused here.

We certainly make the __gcc_nested_func_ptr_deleted () call a cleanup attached
to scope exits and certainly the last page of trampolines is not deallocated
(as you note for the sake of avoiding churn in m-mapping).

However, in the current code the only pthread-specific stuff I see (in, say
config/i386/heap-trampoline.c) is specific to changing protections on the
mapped pages.

What I was thinking of is attaching a thread exit cleanup using
pthread_cleanup_push() for platforms with pthreads but without Libc support for
_thread_atexit - I guess I'm missing something :)

[Bug libgcc/113401] Memory (resource) leak in -ftrampoline-impl=heap

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113401

--- Comment #6 from Florian Weimer <fw at gcc dot gnu.org> ---
Sorry, pthread_cleanup_push is purely scope-based, like the existing handler.
It cannot be used to push a handler to some unscoped cleanup function list that
persists even after the current function returns. It's also implemented as a
macro, so it's not possible to emit it from builtin expansion.

[Bug libgcc/113401] Memory (resource) leak in -ftrampoline-impl=heap

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113401

--- Comment #7 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #6)
> Sorry, pthread_cleanup_push is purely scope-based, like the existing
> handler. It cannot be used to push a handler to some unscoped cleanup
> function list that persists even after the current function returns. It's
> also implemented as a macro, so it's not possible to emit it from builtin
> expansion.

Ah, then we have a documentation issue, because man pthread_cleanup_push(3)
describes running the registered functions on thread cancellation or on
thread_exit() [but not, unfortunately if the thread exits by returning - so
still not ideal].

[Bug libgcc/113401] Memory (resource) leak in -ftrampoline-impl=heap

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113401

--- Comment #8 from Florian Weimer <fw at gcc dot gnu.org> ---
Which version of the manual page are you looking at?

https://man7.org/linux/man-pages/man3/pthread_cleanup_push.3.html seems pretty
clear about the scope-based nature (search for discussion of
break/return/goto).

[Bug libgcc/113401] Memory (resource) leak in -ftrampoline-impl=heap

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113401

--- Comment #9 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #8)
> Which version of the manual page are you looking at?
> 
> https://man7.org/linux/man-pages/man3/pthread_cleanup_push.3.html seems
> pretty clear about the scope-based nature (search for discussion of
> break/return/goto).

yeah, got it; one needs to read the union of the sections (the page I was
reading was slightly different but the same basic info).

I suppose if we were able to create a wrapper around the thread routine and the
cleanup was NOP for cases without nested fns.

Otherwise, it looks a bit tricky for platforms without thread_atexit support.

Have to think some more.