Page MenuHomePhabricator

[libcxxabi] Cleanup memory in tests to placate ASAN.
ClosedPublic

Authored by EricWF on Nov 14 2014, 3:43 PM.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 16253.Nov 14 2014, 3:43 PM
EricWF retitled this revision from to [libcxxabi] Cleanup memory in tests to placate ASAN..
EricWF updated this object.
EricWF edited the test plan for this revision. (Show Details)
EricWF added reviewers: danalbert, mclow.lists, jroelofs.
EricWF added a subscriber: Unknown Object (MLST).
dblaikie added inline comments.
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 added inline comments.Nov 14 2014, 11:32 PM
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 edited edge metadata.Nov 16 2014, 7:44 AM

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

dblaikie added inline comments.Nov 17 2014, 11:03 AM
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?

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

EricWF updated this revision to Diff 16353.Nov 18 2014, 3:20 PM
EricWF edited edge metadata.

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

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 added inline comments.Nov 18 2014, 3:48 PM
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 added inline comments.Nov 18 2014, 4:02 PM
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 updated this revision to Diff 16357.Nov 18 2014, 4:03 PM

Remove extra whitespace. Caught by @jroelofs.

jroelofs added inline comments.Nov 18 2014, 4:23 PM
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 added inline comments.Nov 18 2014, 5:35 PM
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 updated this revision to Diff 16361.Nov 18 2014, 5:36 PM

Fix incorrect comment.

jroelofs accepted this revision.Nov 20 2014, 4:36 PM
jroelofs edited edge metadata.

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

Jon

This revision is now accepted and ready to land.Nov 20 2014, 4:36 PM
mclow.lists edited edge metadata.Nov 20 2014, 4:52 PM

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

@mclow.lists What would thrill you?

EricWF closed this revision.Nov 20 2014, 5:54 PM