Re: [Boost-users] [Boost.Test] valgrind complains about invalid reads
Thanks for the reply Gennadiy, see inline for comments: --- On Wed, 12/16/09, Gennadiy Rozental <rogeeff@gmail.com> wrote:
From: Gennadiy Rozental <rogeeff@gmail.com> Subject: Re: [Boost-users] [Boost.Test] valgrind complains about invalid reads To: boost-users@lists.boost.org Date: Wednesday, December 16, 2009, 9:32 PM Scott Banachowski <sbanacho <at> yahoo.com> writes:
I agree with Peter here, the destructor for test_unit
should be virtual.
'Should' is a strong statement. Do you have anything to based it on?
Ok, should is too strong. Change that to "it would be nice to", because the possibility that the base class destructor may be called (even if not by your code, possibly by others).
I find that it is being deleted when test_unit* is a stored in reference counted pointer,
Where?
My mistake, it's held in a map. See the next comment:
and at program exit it is being deleted by calling ~test_unit().
Where do I call destructor explicitly?
You don't. It's being called implicitly at program exit, by valgrind. I think what is happening is that valgrind replaces the delete operator with its own, which is "operator delete(void*)" The typeinfo is likely lost by this replacement of delete. Just because you don't call the destructor, it doesn't mean others won't. If a base class is derived from, best practice is to declare its destructor virtual. It is relatively common to replace allocators (e.g. tcmalloc).
If you use a derived class (test_suite), this is leading to an error.
Where?
As already mentioned, reported by valgrind.
Also, in the code I'm looking at (1.40.0), the test_suite class has a virtual destructor, but its base class test_unit does not. Not making the base class virtual seems to be defeating the purpose.
What purpose? The only reason that test_suite destructor is virtual is because this class in fact is being polymorphically. It has subclass master_test_suite.
By defeating the purpose, I mean it forces code like the following: if( ut_detail::test_id_2_unit_type( tu.second->p_id ) == tut_suite ) delete static_cast<test_suite const*>(tu.second); else delete static_cast<test_case const*>(tu.second); A virtual base case simplifies this code (especially if in the future other classes are added, or in the case of replacement allocator it does the right thing). thanks, Scott
Gennadiy
_______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
__________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
Scott Banachowski wrote:
By defeating the purpose, I mean it forces code like the following:
if( ut_detail::test_id_2_unit_type( tu.second->p_id ) == tut_suite ) delete static_cast<test_suite const*>(tu.second); else delete static_cast<test_case const*>(tu.second);
A virtual base case simplifies this code (especially if in the future other classes are added, or in the case of replacement allocator it does the right thing).
I'm jumping in the wagon but this very piece of code leads to a "incorrect free" on Snow Leopard using g++ 4.2.1 Apple. Maybe it is related to the virtual matter -- ___________________________________________ Joel Falcou - Assistant Professor PARALL Team - LRI - Universite Paris Sud XI Tel : (+33)1 69 15 66 35
AMDG joel falcou wrote:
Scott Banachowski wrote:
By defeating the purpose, I mean it forces code like the following:
if( ut_detail::test_id_2_unit_type( tu.second->p_id ) == tut_suite ) delete static_cast<test_suite const*>(tu.second); else delete static_cast<test_case const*>(tu.second);
A virtual base case simplifies this code (especially if in the future other classes are added, or in the case of replacement allocator it does the right thing).
I'm jumping in the wagon but this very piece of code leads to a "incorrect free" on Snow Leopard using g++ 4.2.1 Apple. Maybe it is related to the virtual matter
We've already discussed this many times. The code above is wrong for reasons unrelated to the use of a virtual destructor, although adding a virtual destructor masks the problem. This has been fixed in the trunk. In Christ, Steven Watanabe
Steven Watanabe wrote:
We've already discussed this many times. The code above is wrong for reasons unrelated to the use of a virtual destructor, although adding a virtual destructor masks the problem. This has been fixed in the trunk.
Thanks for the head up :)
participants (4)
-
joel falcou -
Joel Falcou
-
Scott Banachowski -
Steven Watanabe