| Re: [eigen] Memory Leak When Constructor of User-Defined Type Throws Exception |
[ Thread Index | Date Index | More lists.tuxfamily.org/eigen Archives ]
Good evening,
thank you for looking at the patch and sorry for my late reply. I have
(finally) applied your suggestions now (see attached).
The unit test is now using a static object count as suggested. However,
this can only check that the member-wise destruction happens. I don't
know of a way to check from within C++ that a 'std::malloc()'ed pointer
was 'std::free()'ed again. (Eigen does not use operator 'new' /
'delete' to manage the storage array.)
While adding '#ifdef EIGEN_EXCEPTIONS' to the code, I found that there
is an existing try-catch in 'Memory.h' that is not surrounded by such
guards. I've left it unchanged since it has nothing to do with the
reason for this patch.
By the way, I find that
#ifdef EIGEN_EXCEPTIONS
try
#endif
{
do_stuff();
if (bad_things_happened())
#ifdef EIGEN_EXCEPTIONS
throw Ball("oh no!");
#else
std::abort();
#endif
}
#ifdef EIGEN_EXCEPTIONS
catch (const Ball&)
{
do_other_stuff();
}
#endif
is quite hard to read. In GCC source, I found the following macros:
#ifdef EIGEN_EXCEPTIONS
# define EIGEN_THROW(X) throw X
# define EIGEN_TRY try
# define EIGEN_CATCH(X) catch (x)
#else
# define EIGEN_THROW(X) std::abort()
# define EIGEN_TRY if (true)
# define EIGEN_CATCH(X) else
#endif
Then, we can write the above code as
EIGEN_TRY
{
do_stuff();
if (bad_things_happened())
EIGEN_THROW(Ball("oh no!"));
}
EIGEN_CATCH(const Ball&)
{
do_other_stuff();
}
which looks much cleaner to my eyes.
Maybe I have misunderstood the current infrastructure but if not: What
do you think about this style?
Moritz
Christoph Hertzberg <chtz@xxxxxxxxxxxxxxxxxxxxxxxx> writes:
> On 06.07.2014 07:47, Moritz Klammler wrote:
>> unfortunately, the Bugzilla server is not functional (I don't know
>> for how long this might be) so I could not check whether this issue
>> has already been reported.
>
> This appears to be related to a software update by
> http://tuxfamily.org/ Jitse is currently investigating.
>
>> Currently, if the constructor of a (user-defined) type throws an
>> exception during construction of a dense matrix with dynamic size,
>> the memory for the storage of the (not created) matrix is leaked.
>> This is not a documented [1] limitation for user-defined types so I
>> assume that it is not intended behavior. (The page also does not
>> mention that user-defined types must be default-constructible.
>> Although, that might go without saying.)
>
> Yes, we shall make Eigen leak-proof wherever possible. And we could
> add a note to [1] that types must be default-constructible.
>
> I'm confident that modern compilers are smart enough to optimize away
> the try-catch block if the Scalar constructor does not throw, however
> we need to #ifdef-guard it for platforms which do not support
> exceptions. For that we already have:
> #ifdef EIGEN_EXCEPTIONS
>
>
>> Attached is a unit test that demonstrates the leak if run through
>> Valgrind (or some other debugger) as in
>
> It would be nice to have a proper unit-test that actually checks if
> everything gets deconstucted properly, maybe by keeping track of
> allocations in a static variable. (Also, your unit test will
> wrongfully assert with a probability of (10/11)^1288 *g* )
>
>
> Christoph
>
>> [1] http://eigen.tuxfamily.org/dox/TopicCustomizingEigen.html#user_defined_scalars
--
OpenPGP:
Public Key: http://openpgp.klammler.eu
Fingerprint: 80C1 EC79 B554 3D84 0A35 A728 7057 B288 CE61 2235
# HG changeset patch
# User Moritz Klammler <moritz@xxxxxxxxxxx>
# Date 1404622693 -7200
# Sun Jul 06 06:58:13 2014 +0200
# Node ID 28d75bc35611b28c5021c0456d33b03b425a7cde
# Parent a7a7cf52fb22ecaec44495a784b834c760c53177
Avoid memory leak when constructor of user-defined type throws exception.
The added check `ctorleak.cpp` demonstrates how the leak can be reproduced.
The test appears to pass but it is leaking the storage of the (not created)
matrix. I don't know how to make this test fail in the existing test suite but
you can run it through Valgrind (or another debugger) to verify the leak.
$ ./check.sh ctorleak && valgrind --leak-check=full ./test/ctorleak
This patch fixes this leak by adding some try-catch-delete-rethrow blocks to
`Eigen/src/Core/util/Memory.h`.
diff --git a/Eigen/src/Core/util/Memory.h b/Eigen/src/Core/util/Memory.h
--- a/Eigen/src/Core/util/Memory.h
+++ b/Eigen/src/Core/util/Memory.h
@@ -333,35 +333,44 @@ template<> inline void* conditional_alig
{
return std::realloc(ptr, new_size);
}
/*****************************************************************************
*** Construction/destruction of array elements ***
*****************************************************************************/
-/** \internal Constructs the elements of an array.
- * The \a size parameter tells on how many objects to call the constructor of T.
- */
-template<typename T> inline T* construct_elements_of_array(T *ptr, size_t size)
-{
- for (size_t i=0; i < size; ++i) ::new (ptr + i) T;
- return ptr;
-}
-
/** \internal Destructs the elements of an array.
* The \a size parameters tells on how many objects to call the destructor of T.
*/
template<typename T> inline void destruct_elements_of_array(T *ptr, size_t size)
{
// always destruct an array starting from the end.
if(ptr)
while(size) ptr[--size].~T();
}
+/** \internal Constructs the elements of an array.
+ * The \a size parameter tells on how many objects to call the constructor of T.
+ */
+template<typename T> inline T* construct_elements_of_array(T *ptr, size_t size)
+{
+ size_t i;
+ try
+ {
+ for (i = 0; i < size; ++i) ::new (ptr + i) T;
+ return ptr;
+ }
+ catch (...)
+ {
+ destruct_elements_of_array(ptr, i);
+ throw;
+ }
+}
+
/*****************************************************************************
*** Implementation of aligned new/delete-like functions ***
*****************************************************************************/
template<typename T>
EIGEN_ALWAYS_INLINE void check_size_for_overflow(size_t size)
{
if(size > size_t(-1) / sizeof(T))
@@ -371,24 +380,40 @@ EIGEN_ALWAYS_INLINE void check_size_for_
/** \internal Allocates \a size objects of type T. The returned pointer is guaranteed to have 16 bytes alignment.
* On allocation error, the returned pointer is undefined, but a std::bad_alloc is thrown.
* The default constructor of T is called.
*/
template<typename T> inline T* aligned_new(size_t size)
{
check_size_for_overflow<T>(size);
T *result = reinterpret_cast<T*>(aligned_malloc(sizeof(T)*size));
- return construct_elements_of_array(result, size);
+ try
+ {
+ return construct_elements_of_array(result, size);
+ }
+ catch (...)
+ {
+ aligned_free(result);
+ throw;
+ }
}
template<typename T, bool Align> inline T* conditional_aligned_new(size_t size)
{
check_size_for_overflow<T>(size);
T *result = reinterpret_cast<T*>(conditional_aligned_malloc<Align>(sizeof(T)*size));
- return construct_elements_of_array(result, size);
+ try
+ {
+ return construct_elements_of_array(result, size);
+ }
+ catch (...)
+ {
+ conditional_aligned_free<Align>(result);
+ throw;
+ }
}
/** \internal Deletes objects constructed with aligned_new
* The \a size parameters tells on how many objects to call the destructor of T.
*/
template<typename T> inline void aligned_delete(T *ptr, size_t size)
{
destruct_elements_of_array<T>(ptr, size);
@@ -407,39 +432,69 @@ template<typename T, bool Align> inline
template<typename T, bool Align> inline T* conditional_aligned_realloc_new(T* pts, size_t new_size, size_t old_size)
{
check_size_for_overflow<T>(new_size);
check_size_for_overflow<T>(old_size);
if(new_size < old_size)
destruct_elements_of_array(pts+new_size, old_size-new_size);
T *result = reinterpret_cast<T*>(conditional_aligned_realloc<Align>(reinterpret_cast<void*>(pts), sizeof(T)*new_size, sizeof(T)*old_size));
if(new_size > old_size)
- construct_elements_of_array(result+old_size, new_size-old_size);
+ {
+ try
+ {
+ construct_elements_of_array(result+old_size, new_size-old_size);
+ }
+ catch (...)
+ {
+ conditional_aligned_free<Align>(result);
+ throw;
+ }
+ }
return result;
}
template<typename T, bool Align> inline T* conditional_aligned_new_auto(size_t size)
{
check_size_for_overflow<T>(size);
T *result = reinterpret_cast<T*>(conditional_aligned_malloc<Align>(sizeof(T)*size));
if(NumTraits<T>::RequireInitialization)
- construct_elements_of_array(result, size);
+ {
+ try
+ {
+ construct_elements_of_array(result, size);
+ }
+ catch (...)
+ {
+ conditional_aligned_free<Align>(result);
+ throw;
+ }
+ }
return result;
}
template<typename T, bool Align> inline T* conditional_aligned_realloc_new_auto(T* pts, size_t new_size, size_t old_size)
{
check_size_for_overflow<T>(new_size);
check_size_for_overflow<T>(old_size);
if(NumTraits<T>::RequireInitialization && (new_size < old_size))
destruct_elements_of_array(pts+new_size, old_size-new_size);
T *result = reinterpret_cast<T*>(conditional_aligned_realloc<Align>(reinterpret_cast<void*>(pts), sizeof(T)*new_size, sizeof(T)*old_size));
if(NumTraits<T>::RequireInitialization && (new_size > old_size))
- construct_elements_of_array(result+old_size, new_size-old_size);
+ {
+ try
+ {
+ construct_elements_of_array(result+old_size, new_size-old_size);
+ }
+ catch (...)
+ {
+ conditional_aligned_free<Align>(result);
+ throw;
+ }
+ }
return result;
}
template<typename T, bool Align> inline void conditional_aligned_delete_auto(T *ptr, size_t size)
{
if(NumTraits<T>::RequireInitialization)
destruct_elements_of_array<T>(ptr, size);
conditional_aligned_free<Align>(ptr);
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -246,16 +246,18 @@ ei_add_test(rvalue_types)
ei_add_test(dense_storage)
ei_add_test(simplicial_cholesky)
ei_add_test(conjugate_gradient)
ei_add_test(bicgstab)
ei_add_test(sparselu)
ei_add_test(sparseqr)
+ei_add_test(ctorleak)
+
# ei_add_test(denseLM)
if(QT4_FOUND)
ei_add_test(qtvector "" "${QT_QTCORE_LIBRARY}")
endif(QT4_FOUND)
if(UMFPACK_FOUND)
ei_add_test(umfpack_support "" "${UMFPACK_ALL_LIBS}")
diff --git a/test/ctorleak.cpp b/test/ctorleak.cpp
new file mode 100644
--- /dev/null
+++ b/test/ctorleak.cpp
@@ -0,0 +1,43 @@
+#include "main.h"
+
+#include <exception> // std::exception
+
+struct Foo
+{
+ int dummy;
+ Foo() { if (!internal::random(0, 10)) throw Foo::Fail(); }
+ class Fail : public std::exception {};
+};
+
+namespace Eigen
+{
+ template<>
+ struct NumTraits<Foo>
+ {
+ typedef double Real;
+ typedef double NonInteger;
+ typedef double Nested;
+ enum
+ {
+ IsComplex = 0,
+ IsInteger = 1,
+ ReadCost = -1,
+ AddCost = -1,
+ MulCost = -1,
+ IsSigned = 1,
+ RequireInitialization = 1
+ };
+ static inline Real epsilon() { return 1.0; }
+ static inline Real dummy_epsilon() { return 0.0; }
+ };
+}
+
+void test_ctorleak()
+{
+ try
+ {
+ Matrix<Foo, Dynamic, Dynamic> m(14, 92);
+ eigen_assert(false); // not reached
+ }
+ catch (const Foo::Fail&) { /* ignore */ }
+}
# HG changeset patch
# User Moritz Klammler <moritz@xxxxxxxxxxx>
# Date 1405718396 -7200
# Fri Jul 18 23:19:56 2014 +0200
# Node ID 2737292b0292f4c08b72531e44805b75a99df62b
# Parent 28d75bc35611b28c5021c0456d33b03b425a7cde
Applied changes suggested by Christoph Hertzberg to c'tor leak fix.
- Enclose exception handling in '#ifdef EIGEN_EXCEPTIONS'.
- Use an object counter to demonstrate the bug more readily.
diff --git a/Eigen/src/Core/util/Memory.h b/Eigen/src/Core/util/Memory.h
--- a/Eigen/src/Core/util/Memory.h
+++ b/Eigen/src/Core/util/Memory.h
@@ -349,26 +349,30 @@ template<typename T> inline void destruc
}
/** \internal Constructs the elements of an array.
* The \a size parameter tells on how many objects to call the constructor of T.
*/
template<typename T> inline T* construct_elements_of_array(T *ptr, size_t size)
{
size_t i;
+#ifdef EIGEN_EXCEPTIONS
try
+#endif
{
for (i = 0; i < size; ++i) ::new (ptr + i) T;
return ptr;
}
+#ifdef EIGEN_EXCEPTIONS
catch (...)
{
destruct_elements_of_array(ptr, i);
throw;
}
+#endif
}
/*****************************************************************************
*** Implementation of aligned new/delete-like functions ***
*****************************************************************************/
template<typename T>
EIGEN_ALWAYS_INLINE void check_size_for_overflow(size_t size)
@@ -380,40 +384,48 @@ EIGEN_ALWAYS_INLINE void check_size_for_
/** \internal Allocates \a size objects of type T. The returned pointer is guaranteed to have 16 bytes alignment.
* On allocation error, the returned pointer is undefined, but a std::bad_alloc is thrown.
* The default constructor of T is called.
*/
template<typename T> inline T* aligned_new(size_t size)
{
check_size_for_overflow<T>(size);
T *result = reinterpret_cast<T*>(aligned_malloc(sizeof(T)*size));
+#ifdef EIGEN_EXCEPTIONS
try
+#endif
{
return construct_elements_of_array(result, size);
}
+#ifdef EIGEN_EXCEPTIONS
catch (...)
{
aligned_free(result);
throw;
}
+#endif
}
template<typename T, bool Align> inline T* conditional_aligned_new(size_t size)
{
check_size_for_overflow<T>(size);
T *result = reinterpret_cast<T*>(conditional_aligned_malloc<Align>(sizeof(T)*size));
+#ifdef EIGEN_EXCEPTIONS
try
+#endif
{
return construct_elements_of_array(result, size);
}
+#ifdef EIGEN_EXCEPTIONS
catch (...)
{
conditional_aligned_free<Align>(result);
throw;
}
+#endif
}
/** \internal Deletes objects constructed with aligned_new
* The \a size parameters tells on how many objects to call the destructor of T.
*/
template<typename T> inline void aligned_delete(T *ptr, size_t size)
{
destruct_elements_of_array<T>(ptr, size);
@@ -433,67 +445,79 @@ template<typename T, bool Align> inline
{
check_size_for_overflow<T>(new_size);
check_size_for_overflow<T>(old_size);
if(new_size < old_size)
destruct_elements_of_array(pts+new_size, old_size-new_size);
T *result = reinterpret_cast<T*>(conditional_aligned_realloc<Align>(reinterpret_cast<void*>(pts), sizeof(T)*new_size, sizeof(T)*old_size));
if(new_size > old_size)
{
+#ifdef EIGEN_EXCEPTIONS
try
+#endif
{
construct_elements_of_array(result+old_size, new_size-old_size);
}
+#ifdef EIGEN_EXCEPTIONS
catch (...)
{
conditional_aligned_free<Align>(result);
throw;
}
+#endif
}
return result;
}
template<typename T, bool Align> inline T* conditional_aligned_new_auto(size_t size)
{
check_size_for_overflow<T>(size);
T *result = reinterpret_cast<T*>(conditional_aligned_malloc<Align>(sizeof(T)*size));
if(NumTraits<T>::RequireInitialization)
{
+#ifdef EIGEN_EXCEPTIONS
try
+#endif
{
construct_elements_of_array(result, size);
}
+#ifdef EIGEN_EXCEPTIONS
catch (...)
{
conditional_aligned_free<Align>(result);
throw;
}
+#endif
}
return result;
}
template<typename T, bool Align> inline T* conditional_aligned_realloc_new_auto(T* pts, size_t new_size, size_t old_size)
{
check_size_for_overflow<T>(new_size);
check_size_for_overflow<T>(old_size);
if(NumTraits<T>::RequireInitialization && (new_size < old_size))
destruct_elements_of_array(pts+new_size, old_size-new_size);
T *result = reinterpret_cast<T*>(conditional_aligned_realloc<Align>(reinterpret_cast<void*>(pts), sizeof(T)*new_size, sizeof(T)*old_size));
if(NumTraits<T>::RequireInitialization && (new_size > old_size))
{
+#ifdef EIGEN_EXCEPTIONS
try
+#endif
{
construct_elements_of_array(result+old_size, new_size-old_size);
}
+#ifdef EIGEN_EXCEPTIONS
catch (...)
{
conditional_aligned_free<Align>(result);
throw;
}
+#endif
}
return result;
}
template<typename T, bool Align> inline void conditional_aligned_delete_auto(T *ptr, size_t size)
{
if(NumTraits<T>::RequireInitialization)
destruct_elements_of_array<T>(ptr, size);
diff --git a/test/ctorleak.cpp b/test/ctorleak.cpp
--- a/test/ctorleak.cpp
+++ b/test/ctorleak.cpp
@@ -1,19 +1,38 @@
#include "main.h"
#include <exception> // std::exception
struct Foo
{
+ static unsigned object_count;
+ static unsigned object_limit;
int dummy;
- Foo() { if (!internal::random(0, 10)) throw Foo::Fail(); }
+
+ Foo()
+ {
+#ifdef EIGEN_EXCEPTIONS
+ // TODO: Is this the correct way to handle this?
+ if (Foo::object_count > Foo::object_limit) { throw Foo::Fail(); }
+#endif
+ ++Foo::object_count;
+ }
+
+ ~Foo()
+ {
+ --Foo::object_count;
+ }
+
class Fail : public std::exception {};
};
+unsigned Foo::object_count = 0;
+unsigned Foo::object_limit = 0;
+
namespace Eigen
{
template<>
struct NumTraits<Foo>
{
typedef double Real;
typedef double NonInteger;
typedef double Nested;
@@ -29,15 +48,22 @@ namespace Eigen
};
static inline Real epsilon() { return 1.0; }
static inline Real dummy_epsilon() { return 0.0; }
};
}
void test_ctorleak()
{
+ Foo::object_count = 0;
+ Foo::object_limit = internal::random(0, 14 * 92 - 2);
+#ifdef EIGEN_EXCEPTIONS
try
+#endif
{
Matrix<Foo, Dynamic, Dynamic> m(14, 92);
eigen_assert(false); // not reached
}
+#ifdef EIGEN_EXCEPTIONS
catch (const Foo::Fail&) { /* ignore */ }
+#endif
+ VERIFY_IS_EQUAL(static_cast<unsigned>(0), Foo::object_count);
}
Attachment:
pgpPqyUIH59v7.pgp
Description: PGP signature
| Mail converted by MHonArc 2.6.19+ | http://listengine.tuxfamily.org/ |