[libcxxabi] Cleanup memory in tests to placate ASAN.

[EricWF]

Summary

ASAN fires on these tests because they don't clean up their memory.

[dblaikie]

test/catch_ptr_02.cpp

145

Could just rephrase this:

vDerived d;
try
{
    throw &d;

I assume that'd be sufficiently equivalent without having to pass owning pointers through the throw/catch?

test/inherited_exception.cpp

160

This one could use a global variable & throwing the address of that.

test/test_vector1.cpp

238

& I don't understand this one well enough to figure out why 'three' needs to be handled here but 'one' and 'two' are fine... all the deletes/news seem to appear in triples, so why is this one weird?

[EricWF]

test/catch_ptr_02.cpp

145

That's essentially what the functions test1, test2, test3, and test4 are doing. That is why I was hesitant to change the form.

test/inherited_exception.cpp

160

I don't understand enough about libc++abi to say those would both be equivalent but I really hope they would be.

test/test_vector1.cpp

238

There is a counter that throws after 15 objects have been destroyed. There are 10 objects in each of the vectors. That means the exception is throws in the call to __cxa_vec_delete2. I'm assuming the test checks that two's memory gets freed even when there is an exception thrown during __cxa_vec_delete2.

[jroelofs]

Can you re-generate the diff with more context please?

[dblaikie]

test/catch_ptr_02.cpp

145

test 1-4 are testing that the constness matches correctly, this is testing that catching by base from a derived throw works correctly - I think that's orthogonal to the way the memory is allocated.

test/test_vector1.cpp

238

I think the better way to phrase this part of the test might be to remove 'three' entirely (its allocations aren't part of the test, the test is about throwing from the dtor) and add an "assert(false)" where the delete3 is currently (and change the expectations to match the new behavior - 20 constructions, 20 destructions I guess). Yes/no/maybe?

[EricWF]

Thanks for the analysis. I'll make the changes.

[EricWF]

Remove all dynamic allocation as suggested by @dblaikie. Added more context in diffs for @jroelofs.

[dblaikie]

Looks good to me - but I don't know anything about libcxxabi, so I'll leave it to someone in there to actually sign off on the patch.

[jroelofs]

test/catch_ptr_02.cpp

141

This change looks good to me.

test/inherited_exception.cpp

59

Extra whitespace.

65

This change looks good to me.

test/test_vector1.cpp

218

I may have missed the conversation about this one, but why are you deleting this part of the test?

I don't fee comfortable signing off on the changes in this file as I'm not familiar with __cxa_vec_new*.

[EricWF]

test/test_vector1.cpp

218

The test seems to check that __cxa_vec_delete deallocates the vector even if one of the destructors throws. The call to __cxa_vec_delete3 is not called because __cxa_vec_delete2 is expected to throw. To check this assert(false) is now used instead of the side effects of __cxa_vec_delete3.

@mclow seems to have written the test so I'll ask him to sign off on it.

[EricWF]

Remove extra whitespace. Caught by @jroelofs.

[jroelofs]

test/test_vector1.cpp

231

This comment lies if you end up going though with the change.

Is there a way to tell asan that we expect a leak here? Based on this comment, I think leaking is part of the expected behavior of the test. @mclow.lists what do you think?

[EricWF]

test/test_vector1.cpp

231

I'll change the comment. Leaking is *currently* part of the expected behavior since the call to cleanup three should never be called. Changing it to an assert should test the same thing (that control flow exits the block before the next statement).

[EricWF]

Fix incorrect comment.

[jroelofs]

Ok, now I'm convinced.... The whole patch LGTM.

Jon

[mclow.lists]

Meh. I'm not thrilled with these changes, but they're ok.

[EricWF]

@mclow.lists What would thrill you?

test/catch_ptr_02.cpp

     }
     catch (vBase *p) {
         assert(p != 0);
+        // NOTE: delete needed to placate ASAN.
+        delete p;
     }
     catch (...)
     {

test/inherited_exception.cpp

     catch (const Base2* c)
     {
         assert(true);
+        // NOTE: delete needed to placate ASAN.
+        delete static_cast<Child*>(const_cast<Base2*>(c));
     }
     catch (...)
     {

test/test_vector1.cpp

                 gDestructorCounter << ")" << std::endl;
         retVal = 1;
         }
+// Delete "three" to placate ASAN.
+    __cxxabiv1::__cxa_vec_delete3(three, 40, 8, my_destruct, my_dealloc3);
 
 //  Try throwing from a destructor - should be fine.
     gConstructorCounter = gDestructorCounter = 0;