| Message ID | 20160224183558.GF4664@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Feb 24, 2016 at 7:35 PM, Jonathan Wakely <jwakely@redhat.com> wrote: > This adds a new function to libsupc++ which will free the memory still > in use by the pool used for allocating exceptions when malloc fails. > > This is similar to glibc's __libc_freeres, which valgrind (and other > tools?) use to tell glibc to deallocate everything before exiting. > > I initially called it __gnu_cxx::__free_eh_pool() but I figured we > might have other memory in use at some later date, and we wouldn't > want valgrind to have to start calling a second function, nor make a > function called __free_eh_pool() actually free other things. > > I intentionally *didn't* lock the pool's mutex before freeing it, > because this should never be called in a context where multiple > threads are still active and accessing the pool. > > Thoughts? > > Is the new test a good idea, or will exposing it like that in the > testsuite just give users the idea that they can/should be doing the > same themselves? The goal is to lookup the function via dlsym and only call it before glibcs freeres (which will probably make free() in-operatable?) > > [ Aside: should the actual pool be created with placement-new to > [ ensure it doesn't ever get destroyed? > [ > [ alignas(pool) unsigned char poolbuf[sizeof(pool)]; > [ pool& emergency_pool = *::new(poolbuf) pool; Hmm, for pedantic correctness yes I guess. But it hasn't any destructor, it's global (so no CLOBBER / memory re-use issue) and so the destruction won't do anything. Of course it might simply save a global dtor (does it even register one? I don't see one). There is still the missing feature of allowing to size the emergency pool using an environment variable (properly securely implemented, of course). Richard. > >
On 25/02/16 10:36 +0100, Richard Biener wrote: >On Wed, Feb 24, 2016 at 7:35 PM, Jonathan Wakely <jwakely@redhat.com> wrote: >> This adds a new function to libsupc++ which will free the memory still >> in use by the pool used for allocating exceptions when malloc fails. >> >> This is similar to glibc's __libc_freeres, which valgrind (and other >> tools?) use to tell glibc to deallocate everything before exiting. >> >> I initially called it __gnu_cxx::__free_eh_pool() but I figured we >> might have other memory in use at some later date, and we wouldn't >> want valgrind to have to start calling a second function, nor make a >> function called __free_eh_pool() actually free other things. >> >> I intentionally *didn't* lock the pool's mutex before freeing it, >> because this should never be called in a context where multiple >> threads are still active and accessing the pool. >> >> Thoughts? >> >> Is the new test a good idea, or will exposing it like that in the >> testsuite just give users the idea that they can/should be doing the >> same themselves? > >The goal is to lookup the function via dlsym and only call it before >glibcs freeres (which will probably make free() in-operatable?) Yes, I believe so (Mark? Ivo?) Another way to test it would be to use dg-final to run gdb in the global's destructor and try to call the function from GDB. >> >> [ Aside: should the actual pool be created with placement-new to >> [ ensure it doesn't ever get destroyed? >> [ >> [ alignas(pool) unsigned char poolbuf[sizeof(pool)]; >> [ pool& emergency_pool = *::new(poolbuf) pool; > >Hmm, for pedantic correctness yes I guess. But it hasn't any destructor, >it's global (so no CLOBBER / memory re-use issue) and so the destruction >won't do anything. Of course it might simply save a global dtor (does it >even register one? I don't see one). It was the CLOBBER that I was thinking about, but I agree we don't (currently) do that for trivial destructors. >There is still the missing feature of allowing to size the emergency >pool using an environment variable (properly securely implemented, of course). Yes.
On Wed, 2016-02-24 at 18:35 +0000, Jonathan Wakely wrote: > This adds a new function to libsupc++ which will free the memory still > in use by the pool used for allocating exceptions when malloc fails. > > This is similar to glibc's __libc_freeres, which valgrind (and other > tools?) use to tell glibc to deallocate everything before exiting. > > I initially called it __gnu_cxx::__free_eh_pool() but I figured we > might have other memory in use at some later date, and we wouldn't > want valgrind to have to start calling a second function, nor make a > function called __free_eh_pool() actually free other things. I tested this on x86_64-pc-linux-gnu with Ivo's valgrind patch from https://bugs.kde.org/show_bug.cgi?id=345307 and it works pretty nicely. No more spurious still reachable memory issues with memcheck. Is there any possibility to get this backported for 5.4? Thanks, Mark
On Thu, 2016-03-03 at 16:34 +0100, Mark Wielaard wrote: > On Wed, 2016-02-24 at 18:35 +0000, Jonathan Wakely wrote: > > This adds a new function to libsupc++ which will free the memory still > > in use by the pool used for allocating exceptions when malloc fails. > > > > This is similar to glibc's __libc_freeres, which valgrind (and other > > tools?) use to tell glibc to deallocate everything before exiting. > > > > I initially called it __gnu_cxx::__free_eh_pool() but I figured we > > might have other memory in use at some later date, and we wouldn't > > want valgrind to have to start calling a second function, nor make a > > function called __free_eh_pool() actually free other things. > > I tested this on x86_64-pc-linux-gnu with Ivo's valgrind patch from > https://bugs.kde.org/show_bug.cgi?id=345307 and it works pretty nicely. > No more spurious still reachable memory issues with memcheck. > > Is there any possibility to get this backported for 5.4? If there is anything I can do to help move this patch forward, please let me know. Thanks, Mark
On 16/03/16 16:29 +0100, Mark Wielaard wrote: >On Thu, 2016-03-03 at 16:34 +0100, Mark Wielaard wrote: >> On Wed, 2016-02-24 at 18:35 +0000, Jonathan Wakely wrote: >> > This adds a new function to libsupc++ which will free the memory still >> > in use by the pool used for allocating exceptions when malloc fails. >> > >> > This is similar to glibc's __libc_freeres, which valgrind (and other >> > tools?) use to tell glibc to deallocate everything before exiting. >> > >> > I initially called it __gnu_cxx::__free_eh_pool() but I figured we >> > might have other memory in use at some later date, and we wouldn't >> > want valgrind to have to start calling a second function, nor make a >> > function called __free_eh_pool() actually free other things. >> >> I tested this on x86_64-pc-linux-gnu with Ivo's valgrind patch from >> https://bugs.kde.org/show_bug.cgi?id=345307 and it works pretty nicely. >> No more spurious still reachable memory issues with memcheck. >> >> Is there any possibility to get this backported for 5.4? > >If there is anything I can do to help move this patch forward, please >let me know. Sorry for stalling, other things distracted me. It's committed to trunk now though.
commit 40fed1db72b0a4852d5890c2c464a0baabb02b74
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Wed Feb 24 18:22:30 2016 +0000
libstdc++/69945 Add __gnu_cxx::__freeres hook
* config/abi/pre/gnu.ver: Add new symbol.
* libsupc++/eh_alloc.cc (__gnu_cxx::__freeres): Define.
* testsuite/18_support/free_eh_pool.cc: New test.
diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 41069d1..5c6b0fe 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -2148,6 +2148,8 @@ CXXABI_1.3.10 {
_ZGTtNKSt13bad_exceptionD1Ev;
_ZGTtNKSt13bad_exception4whatEv;
+ _ZN9__gnu_cxx9__freeresEv;
+
} CXXABI_1.3.9;
# Symbols in the support library (libsupc++) supporting transactional memory.
diff --git a/libstdc++-v3/libsupc++/eh_alloc.cc b/libstdc++-v3/libsupc++/eh_alloc.cc
index 6973af3..d362e40 100644
--- a/libstdc++-v3/libsupc++/eh_alloc.cc
+++ b/libstdc++-v3/libsupc++/eh_alloc.cc
@@ -73,6 +73,10 @@ using namespace __cxxabiv1;
# define EMERGENCY_OBJ_COUNT 4
#endif
+namespace __gnu_cxx
+{
+ void __freeres();
+}
namespace
{
@@ -106,6 +110,8 @@ namespace
// to implement in_pool.
char *arena;
std::size_t arena_size;
+
+ friend void __gnu_cxx::__freeres();
};
pool::pool()
@@ -244,6 +250,19 @@ namespace
pool emergency_pool;
}
+namespace __gnu_cxx
+{
+ void
+ __freeres()
+ {
+ if (emergency_pool.arena)
+ {
+ ::free(emergency_pool.arena);
+ emergency_pool.arena = 0;
+ }
+ }
+}
+
extern "C" void *
__cxxabiv1::__cxa_allocate_exception(std::size_t thrown_size) _GLIBCXX_NOTHROW
{
diff --git a/libstdc++-v3/testsuite/18_support/free_eh_pool.cc b/libstdc++-v3/testsuite/18_support/free_eh_pool.cc
new file mode 100644
index 0000000..9712d3d
--- /dev/null
+++ b/libstdc++-v3/testsuite/18_support/free_eh_pool.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2016 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run }
+
+namespace __gnu_cxx {
+ void __freeres();
+}
+
+struct X {
+ ~X() {
+ __gnu_cxx::__freeres();
+ }
+};
+
+X x;
+
+int
+main()
+{
+}