Re: [Boost-users] [Boost.Test] valgrind complains about invalid reads

Subject: Re: [Boost-users] [Boost.Test] valgrind complains about invalid reads
From: Peter Klotz (peter.klotz_at_[hidden])
Date: 2009-06-20 15:55:26

Gennadiy Rozental wrote:
> Peter Klotz wrote:
>> Gennadiy Rozental wrote:
>>> Peter Klotz <peter.klotz <at> aon.at> writes:
>>>
>>>> Why not just adding the virtual destructor to make valgrind happy?
>>>
>>> Did you try removing virtual from test_suite destructor instead?
>>>
>>
>> Yes, just making ~test_suite() non virtual worsens things.
>>
>> valgrind complains about twice as many "invalid read" errors.
> 
> [...]
> 
>> Making ~test_unit() virtual removes all errors. Then even a non 
>> virtual ~test_suite() is no longer a problem.
>>
>> So the final conclusion would be to remove virtual from ~test_suite() 
>> and add it to the base class destructor ~test_unit().
> 
> Peter,
> 
> I am not convinced there is a bug inside the Boost.Test code. As far as 
> I can tell all I am doing is legitimate. Maybe we should report this to 
> the valgrind developers instead?

Hello Gennadiy

I finally found the time to extract the code portion from Boost.Test 
that shows the valgrind error. Meanwhile I have switched to Boost 
1.39.0, gcc 4.3.3 and valgrind 3.4.1.

The attached sample is a standalone program and can be compiled like this:

g++ -g -W -Wall BoostTestValgrindError.cpp -o BoostTestValgrindError

Running it with valgrind (valgrind ./BoostTestValgrindError) shows the 
error.

I stripped everything that is not necessary to reproduce the problem. 
The internal map of test cases and test suites is replaced by a set.

The reason for the error seems to be the downcast in your clear() method 
in framework_impl. Assigning the downcasted pointer to a variable and 
then performing the delete resolves the problem. There seems to be a 
subtle difference between these two alternatives.

My suggestion that would also clean up the clear() method is the following:

* Add the virtual destructor to class test_unit

* Rewrite framework_impl::clear() this way:

     void clear()
     {
         while( !m_test_units.empty() )
             delete m_test_units.begin()->second; // the delete will 
erase this element from the map
     }

The virtual destructor ensures that you can safely destroy your derived 
classes through the base class pointer stored in m_test_units. The 
distinction between test suites and test cases in clear() is then no 
longer needed and the downcast is also gone.

Regards, Peter.