This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Consolidate new/delete replacement in tests and disable it when using sanitizers.
ClosedPublic

Authored by EricWF on Dec 7 2014, 7:27 PM.

Details

Summary

MSAN and ASAN also replace new/delete which leads to a link error in these tests. Currently they are unsupported but I think it would be useful if these tests could run with sanitizers.

This patch creates a support header that consolidates the new/delete replacement functionality and checking.
When we are using sanitizers new and delete are no longer replaced and the checks always return true.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 17027.Dec 7 2014, 7:27 PM
EricWF retitled this revision from to [libcxx] Consolidate new/delete replacement in tests and disable it when using sanitizers..
EricWF updated this object.
EricWF edited the test plan for this revision. (Show Details)
EricWF added reviewers: mclow.lists, danalbert, jroelofs.
EricWF added a subscriber: Unknown Object (MLST).
EricWF added inline comments.Dec 7 2014, 8:45 PM
test/localization/locale.stdcvt/codecvt_utf16.pass.cpp
38–42

I'm not sure why it checks that p is not null. I added an assertion and it never fired. Should count_new.hpp also assert deleted pointers are non-null? Obviously passing a nullptr to delete is legal but I'm not sure why we would be doing that.

jroelofs added inline comments.Dec 8 2014, 6:40 AM
test/support/count_new.hpp
8

For avoidance of macros, how about something like this:

class MemCounter {
private:
  int new_called;
public:
  bool checkNewCallsEq(int N) {
#if !__has_feature(address_sanitizer) && !__has_feature(memory_sanitizer)
    return new_called == N;
#else
    return true;
#endif
  }
};

MemCounter globalMemCounter; 

#if !__has_feature(address_sanitizer) && !__has_feature(memory_sanitizer)
void* operator new(std::size_t s) throw(std::bad_alloc) {
    ...
}
#endif

And then in the individual tests:

assert(globalMemCounter.checkNewCallsEq(0));

And then if we want to be even more fancy later on, the MemCounter objects could be set up to count only within a scope by allowing them to register with each other to listen for these events. That seems like it'd be beneficial for at least utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp, and possibly others.

54

Do MSan and ASan have hooks that we could use to implement these checks when they're enabled?

EricWF added inline comments.Dec 9 2014, 12:21 PM
test/support/count_new.hpp
8

I like it! Much cleaner than what we have now. Thanks!

EricWF updated this revision to Diff 17092.Dec 9 2014, 1:14 PM

Mostly address @jroelofs comments.

jroelofs added inline comments.Dec 9 2014, 1:30 PM
test/support/count_new.hpp
49

Hmm... I just realized that these have really weird names. new_called doesn't count the number of times new was called, rather it counts the number of objects allocated by new that haven't been deleted yet (i.e. the tests don't care if there are extra allocations and deallocations between checks).

EricWF updated this revision to Diff 17096.Dec 9 2014, 1:54 PM

Address @jroelofs point that the variables are poorly named. I introduced outstanding_new to replace count_new and count_new now acts as a monotonic counter.

jroelofs added inline comments.Dec 9 2014, 2:04 PM
test/utilities/memory/default.allocator/allocator.members/allocate.pass.cpp
27

I think we're losing test coverage now that this assert is gone :(

EricWF added inline comments.Dec 9 2014, 2:43 PM
test/utilities/memory/default.allocator/allocator.members/allocate.pass.cpp
27

We shouldn't be. I added assertions on line 38 and line 47 that check the same thing.

EricWF updated this object.Dec 9 2014, 2:44 PM
jroelofs edited edge metadata.Dec 9 2014, 3:25 PM

Looks good to me.

test/utilities/memory/default.allocator/allocator.members/allocate.pass.cpp
27

Oh, I missed that.

EricWF updated this revision to Diff 17573.Dec 22 2014, 2:38 PM
EricWF edited edge metadata.

Updating patch against TOT.

EricWF accepted this revision.Dec 22 2014, 2:38 PM
EricWF added a reviewer: EricWF.

Accepting.

This revision is now accepted and ready to land.Dec 22 2014, 2:38 PM
EricWF closed this revision.Dec 22 2014, 2:39 PM