From 2902f23a4193d93c2e96daa45587a8c597c0a831 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 27 Jun 2019 10:57:34 -0400 Subject: DRTVWR-476: Remove special llcorehttp test memory manager. NickyD discovered that the substitute default allocator used for llcorehttp tests was returning badly-aligned storage, which caused access violations on alignment-sensitive data such as std::atomic. Thanks Nicky!! Moreover, the llcorehttp test assertions regarding memory usage, well- intentioned though they are, have been causing us trouble for years. Many have already been disabled. The problem is that use of test_allocator.h affected *everything* defined with that header file's declarations visible. That inevitably included specific functions in other subsystems. Those functions then (unintentionally) consumed the special allocator, throwing off the memory tracking and making certain memory-related assertions consistently fail. This is a particular, observable bad effect of One Definition Rule violations. Within a given program, C++ allows multiple definitions for the same entity, but requires that all such definitions be the same. Partial visibility of the global operator new() and operator delete() overrides meant that some definitions of certain entities used the default global allocator, some used llcorehttp's. There may have been other, more subtle bad effects of these ODR violations. If one wanted to reimplement verification of the memory consumption of llcorehttp classes: * Each llcorehttp class (for which memory tracking was desired) should declare class-specific operator new() and operator delete() methods. Naturally, these would all consume a central llcorehttp-specific allocator, but that allocator should *not* be named global operator new(). * Presumably that would require runtime indirection to allow using the default allocator in production while substituting the special allocator for tests. * Recording and verifying the memory consumption in each test should be performed in the test-object constructor and destructor, rather than being sprinkled throughout the test() methods. * With that mechanism in place, the test object should provide methods to adjust (or entirely disable) memory verification for a particular test. * The test object should also provide a "yes, we're still consuming llcorehttp memory" method to be used for spot checks in the middle of tests -- instead of sprinkling in explicit comparisons as before. * In fact, the llcorehttp test object in each test_*.hpp file should be derived from a central llcorehttp test-object base class providing those methods. --- indra/llcorehttp/tests/test_bufferstream.hpp | 52 ---------------------------- 1 file changed, 52 deletions(-) (limited to 'indra/llcorehttp/tests/test_bufferstream.hpp') diff --git a/indra/llcorehttp/tests/test_bufferstream.hpp b/indra/llcorehttp/tests/test_bufferstream.hpp index 831c901b9d..2739a6e38e 100644 --- a/indra/llcorehttp/tests/test_bufferstream.hpp +++ b/indra/llcorehttp/tests/test_bufferstream.hpp @@ -30,7 +30,6 @@ #include -#include "test_allocator.h" #include "llsd.h" #include "llsdserialize.h" @@ -45,7 +44,6 @@ struct BufferStreamTestData { // the test objects inherit from this so the member functions and variables // can be referenced directly inside of the test functions. - size_t mMemTotal; }; typedef test_group BufferStreamTestGroupType; @@ -59,12 +57,8 @@ void BufferStreamTestObjectType::test<1>() { set_test_name("BufferArrayStreamBuf construction with NULL BufferArray"); - // record the total amount of dynamically allocated memory - mMemTotal = GetMemTotal(); - // create a new ref counted object with an implicit reference BufferArrayStreamBuf * bsb = new BufferArrayStreamBuf(NULL); - ensure("Memory being used", mMemTotal < GetMemTotal()); // Not much will work with a NULL ensure("underflow() on NULL fails", tst_traits_t::eof() == bsb->underflow()); @@ -78,9 +72,6 @@ void BufferStreamTestObjectType::test<1>() // release the implicit reference, causing the object to be released delete bsb; bsb = NULL; - - // make sure we didn't leak any memory - ensure("Allocated memory returned", mMemTotal == GetMemTotal()); } @@ -89,12 +80,8 @@ void BufferStreamTestObjectType::test<2>() { set_test_name("BufferArrayStream construction with NULL BufferArray"); - // record the total amount of dynamically allocated memory - mMemTotal = GetMemTotal(); - // create a new ref counted object with an implicit reference BufferArrayStream * bas = new BufferArrayStream(NULL); - ensure("Memory being used", mMemTotal < GetMemTotal()); // Not much will work with a NULL here ensure("eof() is false on NULL", ! bas->eof()); @@ -104,9 +91,6 @@ void BufferStreamTestObjectType::test<2>() // release the implicit reference, causing the object to be released delete bas; bas = NULL; - - // make sure we didn't leak any memory - ensure("Allocated memory returned", mMemTotal == GetMemTotal()); } @@ -115,13 +99,9 @@ void BufferStreamTestObjectType::test<3>() { set_test_name("BufferArrayStreamBuf construction with empty BufferArray"); - // record the total amount of dynamically allocated memory - mMemTotal = GetMemTotal(); - // create a new ref counted BufferArray with implicit reference BufferArray * ba = new BufferArray; BufferArrayStreamBuf * bsb = new BufferArrayStreamBuf(ba); - ensure("Memory being used", mMemTotal < GetMemTotal()); // I can release my ref on the BA ba->release(); @@ -130,9 +110,6 @@ void BufferStreamTestObjectType::test<3>() // release the implicit reference, causing the object to be released delete bsb; bsb = NULL; - - // make sure we didn't leak any memory - ensure("Allocated memory returned", mMemTotal == GetMemTotal()); } @@ -141,24 +118,17 @@ void BufferStreamTestObjectType::test<4>() { set_test_name("BufferArrayStream construction with empty BufferArray"); - // record the total amount of dynamically allocated memory - mMemTotal = GetMemTotal(); - // create a new ref counted BufferArray with implicit reference BufferArray * ba = new BufferArray; { // create a new ref counted object with an implicit reference BufferArrayStream bas(ba); - ensure("Memory being used", mMemTotal < GetMemTotal()); } // release the implicit reference, causing the object to be released ba->release(); ba = NULL; - - // make sure we didn't leak any memory - ensure("Allocated memory returned", mMemTotal == GetMemTotal()); } @@ -167,9 +137,6 @@ void BufferStreamTestObjectType::test<5>() { set_test_name("BufferArrayStreamBuf construction with real BufferArray"); - // record the total amount of dynamically allocated memory - mMemTotal = GetMemTotal(); - // create a new ref counted BufferArray with implicit reference BufferArray * ba = new BufferArray; const char * content("This is a string. A fragment."); @@ -178,7 +145,6 @@ void BufferStreamTestObjectType::test<5>() // Creat an adapter for the BufferArray BufferArrayStreamBuf * bsb = new BufferArrayStreamBuf(ba); - ensure("Memory being used", mMemTotal < GetMemTotal()); // I can release my ref on the BA ba->release(); @@ -206,9 +172,6 @@ void BufferStreamTestObjectType::test<5>() // release the implicit reference, causing the object to be released delete bsb; bsb = NULL; - - // make sure we didn't leak any memory - ensure("Allocated memory returned", mMemTotal == GetMemTotal()); } @@ -217,9 +180,6 @@ void BufferStreamTestObjectType::test<6>() { set_test_name("BufferArrayStream construction with real BufferArray"); - // record the total amount of dynamically allocated memory - mMemTotal = GetMemTotal(); - // create a new ref counted BufferArray with implicit reference BufferArray * ba = new BufferArray; //const char * content("This is a string. A fragment."); @@ -229,7 +189,6 @@ void BufferStreamTestObjectType::test<6>() { // Creat an adapter for the BufferArray BufferArrayStream bas(ba); - ensure("Memory being used", mMemTotal < GetMemTotal()); // Basic operations bas << "Hello" << 27 << "."; @@ -243,10 +202,6 @@ void BufferStreamTestObjectType::test<6>() // release the implicit reference, causing the object to be released ba->release(); ba = NULL; - - // make sure we didn't leak any memory - // ensure("Allocated memory returned", mMemTotal == GetMemTotal()); - // static U64 mem = GetMemTotal(); } @@ -255,16 +210,12 @@ void BufferStreamTestObjectType::test<7>() { set_test_name("BufferArrayStream with LLSD serialization"); - // record the total amount of dynamically allocated memory - mMemTotal = GetMemTotal(); - // create a new ref counted BufferArray with implicit reference BufferArray * ba = new BufferArray; { // Creat an adapter for the BufferArray BufferArrayStream bas(ba); - ensure("Memory being used", mMemTotal < GetMemTotal()); // LLSD LLSD llsd = LLSD::emptyMap(); @@ -292,9 +243,6 @@ void BufferStreamTestObjectType::test<7>() // release the implicit reference, causing the object to be released ba->release(); ba = NULL; - - // make sure we didn't leak any memory - // ensure("Allocated memory returned", mMemTotal == GetMemTotal()); } -- cgit v1.2.3