[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.

63

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

230–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

230–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

 }
 
 
 struct vBase {};
 struct vDerived : virtual public vBase {};
 
 void test8 ()
 {
+    vDerived derived;
     try
     {
-        throw new vDerived;
+        throw &derived;
         assert(false);
     }
     catch (vBase *p) {
         assert(p != 0);
     }
     catch (...)
     {
         assert (false);
     }
 }
 
 void test9 ()

test/inherited_exception.cpp

void f2() {
   Child child;
   child.b1 = 10;
   child.b2 = 11;
   child.c = 12;
   throw static_cast<Base2&>(child);
 }
 
 void f3() {
-  Child* child  = new Child;
-  child->b1 = 10;
-  child->b2 = 11;
-  child->c = 12;
-  throw static_cast<Base2*>(child);
+  static Child child;
+  child.b1 = 10;
+  child.b2 = 11;
+  child.c = 12;
+  throw static_cast<Base2*>(&child);
 }
 
 int main()
 {
     try
     {
         f1();
         assert(false);
int main()
     }
     catch (const Child* b)
     {
         assert(false);
     }
     catch (const Base2* c)
     {
         assert(true);
     }
     catch (...)
     {
         assert(false);
     }
 }

test/test_vector1.cpp

 //===---------------------------- test_vector.cpp -------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "cxxabi.h"
 
 #include <iostream>
 #include <cstdlib>
+#include <cassert>
 
 //  Wrapper routines
 void *my_alloc2 ( size_t sz ) {
     void *p = std::malloc ( sz );
 //  std::printf ( "Allocated %ld bytes at %lx\n", sz, (unsigned long) p );  
     return p;
     }
     
//      the destructor from the partially constructed array.
 
     return retVal;
     }
 
 //  Make sure the constructors and destructors are matched
 int test_exception_in_destructor ( ) {
     int retVal = 0;
     void *one, *two, *three;
+    one = two = three = NULL;
 
 //  Throw from within a destructor
     gConstructorCounter = gDestructorCounter = 0;
     gConstructorThrowTarget = -1;
     gDestructorThrowTarget  = 15;
     try {
-        one = two = three = NULL;
+        one = two = NULL;
         one     = __cxxabiv1::__cxa_vec_new ( 10, 40, 8, throw_construct, throw_destruct );
         two     = __cxxabiv1::__cxa_vec_new2( 10, 40, 8, throw_construct, throw_destruct, my_alloc2, my_dealloc2 );
-        three   = __cxxabiv1::__cxa_vec_new3( 10, 40, 8, throw_construct, throw_destruct, my_alloc2, my_dealloc3 );
         }
     catch ( int i ) {}
     
     try {
         __cxxabiv1::__cxa_vec_delete ( one,       40, 8, throw_destruct );
         __cxxabiv1::__cxa_vec_delete2( two,       40, 8, throw_destruct, my_dealloc2 );
-        __cxxabiv1::__cxa_vec_delete3( three,     40, 8, throw_destruct, my_dealloc3 );
+        assert(false);
         }
     catch ( int i ) {}
     
 //  We should have thrown in the middle of cleaning up "two", which means that
-//  there should be 20 calls to the destructor, and "three" was not cleaned up.
-    if ( gConstructorCounter != 30 || gDestructorCounter != 20 ) {
+//  there should be 20 calls to the destructor and the try block should exit
+//  before the assertion.
+    if ( gConstructorCounter != 20 || gDestructorCounter != 20 ) {
         std::cerr << "Unexpected Constructor/Destructor calls (1D)" << std::endl;
-        std::cerr << "Expected (30, 20), but got (" << gConstructorCounter << ", " << 
+        std::cerr << "Expected (20, 20), but got (" << gConstructorCounter << ", " <<
                 gDestructorCounter << ")" << std::endl;
         retVal = 1;
         }
 
 //  Try throwing from a destructor - should be fine.
     gConstructorCounter = gDestructorCounter = 0;
     gConstructorThrowTarget = -1;
     gDestructorThrowTarget  = 5;
     try { vec_on_stack v; }
     catch ( int i ) {}