Sign in to follow this  
Woodsman

Malloc failing only in release

Recommended Posts

Alright, I'm having yet another weird problem, which I hadn't even noticed until now. I have a class that is little more than a list. It is defined as follows:
class KSEList {
public:
    KSEList();
    ~KSEList();
    KSE *current;
    KSEList *next;

    void operator delete (void *);
    void* operator new(size_t size);

    int getLength();

    float r;
    float s;
    int h;
};

New and delete are:
void* KSEList::operator new(size_t size) {
    KSEList *tmp = (KSEList *) malloc(size);
    tmp->current = 0;
    tmp->next = 0;
    return tmp;
}
void KSEList::operator delete(void *s) {
    if(s==0)
        return;
    if(((KSEList *)s)->next!=0) {
        delete ((KSEList *)s)->next;
    }
    if(((KSEList *)s)->current!=0)
        delete ((KSEList *)s)->current;
    free(s);
    s = 0;
}

Now, my problem is that when I run the code in Release mode (by going into the release directory and running the application) I eventually get the XP "GAT.exe has encountered a problem and needs to close. We are sorry for the inconvenience." I have narrowed this down to the 14th call to new, specifically, where tmp is being malloc'd. I don't get this problem in debug mode from the actual app or within the IDE, nor do I get it when running in release within the IDE. Is there a problem with my code?

Share this post


Link to post
Share on other sites
I don't think your operator new and delete are allowed to assume they're pointing to the class, and play with the data. For example, some compilers may store an array size when you do a new[], and this would be the flow of a new operation

x = new Blah[5];
which calls
void *Blah::operator new(sizeof(Blah)*5 + sizeof(size_t))
which does
return malloc (size)
then the compiler does:
*ptr = 5;
ptr += sizeof(size_t);
return ptr

so... x != the return from the malloc. Assuming the pointer is untouched may work, on some versions of some compilers, but relying on that is asking for trouble.

Any initilization of member data should be done in the constructor or initilization list. Any cleanup in the destructor. This will allow subclasses to work correctly. This will allow array allocations to work correctly. This won't get in the way of the compiler's housekeeping data. It's the way it was designed to work.

edit: Also, why does deleting an object, cause it to delete other objects? Can you not delete a single list element, without destroying the entire list (or remainder of the list)?

Share this post


Link to post
Share on other sites
Ok, I'm not really familiar with new/delete to be honest.
Quote:
Original post by Namethatnobodyelsetook
edit: Also, why does deleting an object, cause it to delete other objects? Can you not delete a single list element, without destroying the entire list (or remainder of the list)?

It's a list of sub-expressions, and if I delete one element, I'm always going to be deleting all of those it is connected to. As far as I could tell, it was working fine, but then with this release issue, maybe I'm wrong.

Share this post


Link to post
Share on other sites
Replacing everything in new with:
return malloc(size);

Ends with the same problem (in new)...


As a side question, what is the purpose of overloading new/delete if you can't do that? The only purpose I can think of is a counter to keep track of how many news/deletes have been called (memory leaks).

Share this post


Link to post
Share on other sites
first of all why don't you just use a standard library list? and if you mention anything about efficiency its going to be a redundant argument because you can always write/use a custom allocator, also if you only need a singlely linked-list then you can always use slist yes its not part of the c++ standard but its better than writting your own hand-written one.

Next don't mix C-style memory operations with c++, i mean stuff like calling new then using free, c-style memory operations deal with unintialized memory, its fine to use them with-in your version of operator new/delete but don't mix & match allocation with-out any thought.

Next your new/delete pair don't match, in your new operator don't allocate any memory for next & current but in your delete operator your deleting them and also probably destroying the list maybe even calling delete more than once.

Lastly if your going to write your own operator new & delete for your user-defined type aleast provide array versions of them aswell.


Quote:
Original post by Namethatnobodyelsetook
x = new Blah[5];
which calls
void *Blah::operator new(sizeof(Blah)*5 + sizeof(size_t))


Well if he does that on his KSEList its going to call the global array new, and the calculation is more likely to be:

void* ::operator new[](sizeof(Blah) * 5 + delta bytes)

where delta bytes is some minimal implementation defined over-head.

Share this post


Link to post
Share on other sites
Quote:
Original post by Woodsman
As a side question, what is the purpose of overloading new/delete if you can't do that? The only purpose I can think of is a counter to keep track of how many news/deletes have been called (memory leaks).


The main purpose is to take-over memory management, stuff like using garabage collected heap memory, or object pool memory scheme instead of using the general heap memory basically to be more efficent. So when a clients call new/new[] & delete/delete[] on your user-defined type it will use your version of new/new[] & delete/delete[] and not the general puspose global new/new[] & delete/delete[].

[Edited by - snk_kid on September 22, 2004 10:33:50 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by snk_kid
Next don't mix C-style memory operations with c++, i mean stuff like calling new then using free, c-style memory operations deal with unintialized memory, its fine to use them with-in your version of operator new/delete but don't mix & match allocation with-out any thought.

Am I? Everything I'm allocating/deallocating is done in the above fashion.
Quote:
Original post by snk_kid
Next your new/delete pair don't match, in your new operator don't allocate any memory for next & current but in your delete operator your deleting them and also probably destroying the list maybe even calling delete more than once.

But new is setting new and current to 0, and delete just returns if they are 0. Is that valid?
Quote:
Original post by snk_kid
Lastly if your going to write your own operator new & delete for your user-defined type aleast provide array versions of them aswell.

I'll never need an array of them. Anyway, again, I'm new to this so your input is greatly appreciated.

Share this post


Link to post
Share on other sites
Quote:
Original post by Woodsman
But new is setting new and current to 0, and delete just returns if they are 0. Is that valid?


yeah but if there are not null which they most likely will eventually be.

Actually i just remembered that that user-defined types of operator new & delete are implicitly static so when you call delete on KSEList it calls delete on next & current which is probably giving you the problems as your most likely messing up the list.

I would just ust STL's slist to be honest.

[Edited by - snk_kid on September 22, 2004 11:10:13 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Woodsman
As a side question, what is the purpose of overloading new/delete if you can't do that? The only purpose I can think of is a counter to keep track of how many news/deletes have been called (memory leaks).


Yes, but new and delete shouldn't attempt to write to class member variables. What I do is to wrap the allocations. I allocate extra space, put in my tracking info, and return the pointer following my extra space. On delete, I subtract from the pointer to get back to my tracking structure and the actual malloc'd pointer.


// User Information API
unsigned long MemSys_BeginTrackingSection(void);
void MemSys_DumpAllocationsSince(unsigned long nTrackingID, bool bSingleAlloc, bool bLine, bool bFile, bool bTotal);
unsigned long MemSys_MemoryAllocatedSince(unsigned long nTrackingID);

// Internal Memory API, advanced user operations API
void *MemSys_TrackedAllocate(size_t s, char *pFile, unsigned long nLine, unsigned long nType);
void *MemSys_TrackedReAllocate(void *pMem, size_t s);
void MemSys_TrackedFree(void *pMem, unsigned long nType);
void MemSys_PatchOwnerInfo(void *pMem, char *pFile, unsigned long nLine);

// Types for TrackedAllocate and TrackedFree.
// Enforces using correct free for each allocation type
#define MemSys_AllocStyleC 0x4D454D01
#define MemSys_AllocStyleCPP 0x4D454D02
#define MemSys_AllocStyleCPPArray 0x4D454D03

// Global new
void *operator new(size_t);
void *operator new[](size_t);
void operator delete(void *pMem);
void operator delete[](void *pMem);

void *operator new(size_t, char *pFile, unsigned long nLine);
void *operator new[](size_t, char *pFile, unsigned long nLine);
void operator delete(void *pMem, char *pFile, unsigned long nLine);
void operator delete[](void *pMem, char *pFile, unsigned long nLine);
#define GNew new(__FILE__, __LINE__)

// Placement new
void *operator new(size_t, void *pAddr);
void *operator new[](size_t, void *pAddr);
void operator delete(void *pMem, void *pAddr);
void operator delete[](void *pMem, void *pAddr);
#define PNew(a) new(a)

// Take over C allocations
void *MemSys_Malloc(size_t n, char *pFile, unsigned long nLine);
void *MemSys_Calloc(size_t n, size_t m, char *pFile, unsigned long nLine);
void *MemSys_ReAlloc(void *ptr, size_t n, char *pFile, unsigned long nLine);
void MemSys_Free(void *ptr);

#define malloc(n) MemSys_Malloc(n, __FILE__, __LINE__)
#define calloc(n,m) MemSys_Calloc(n,m, __FILE__, __LINE__)
#define realloc(ptr,n) MemSys_ReAlloc(ptr, n, __FILE__, __LINE__)
#define free(ptr) MemSys_Free(ptr)





#include "stdafx.h"


#undef malloc
#undef calloc
#undef free
#undef realloc

#include "stdio.h"

unsigned long g_MemSys_DebugID = 0;

class MemSys_Node
{
public:
MemSys_Node *m_pNext;
MemSys_Node *m_pPrev;

unsigned long m_nType;
unsigned long m_nSize;
char * m_pFile;
unsigned long m_nLine;
unsigned long m_nDebugID;
};

class MemSys_List
{
public:
MemSys_Node m_oHead;
MemSys_Node m_oTail;
EMutex m_oMutex;

MemSys_List();
~MemSys_List();

void Add(MemSys_Node *);
void Rem(MemSys_Node *);
void Sort();
};

void MemSys_List::Sort(void)
{
// Return on empty list;
if (m_oHead.m_pNext == &m_oTail)
return;

// based on http://www.chiark.greenend.org.uk/~sgtatham/algorithms/listsort.html
// Apparently O(n log n), which is good.
// Merge sort has issues on arrays (extra storage requirements) which are
// not an issue with list based implementations. This is supposed to be
// a really good list sort algorithm.
// This could be made faster by only maintaining the next pointers, and
// going back and filling in the prev pointers when the sort is complete.
MemSys_List sortedList;
long k = 1;
long numMerges;
MemSys_Node *p, *q, *tmp;
long psize, qsize;

do
{
p = m_oHead.m_pNext;
numMerges = 0;
while (p->m_pNext)
{
numMerges++;
q = p;
psize = 0;
while (psize < k)
{
psize++;
q = q->m_pNext;
if (!q->m_pNext)
break;
}
qsize = k;
// Merge phase
if (q->m_pNext) // Since q doesn't change every loop iteration, I've broken this out of the main loop condition
{
while (qsize && psize)
{
int strres;

// test case
if (!p->m_pFile)
{
if (!q->m_pFile)
strres = 0;
else
strres = -1;
}
else
{
if (!q->m_pFile)
strres = 1;
else
strres = strcmp(p->m_pFile, q->m_pFile);
}

if ( (strres < 0) || (strres == 0 && p->m_nLine <= q->m_nLine) )
// if ( *((long *)(&((char *)p)[offset])) <= *((long *)(&((char *)q)[offset])) )
{
tmp = p->m_pNext;
sortedList.Add(p);
psize--;
p = tmp;
}
else
{
tmp = q->m_pNext;
sortedList.Add(q);
qsize--;
q = tmp;
if (!q->m_pNext) // Since q doesn't change every loop iteration, I've broken this out of the main loop condition
break;
}
}
}
while (psize--)
{
tmp = p->m_pNext;
sortedList.Add(p);
p = tmp;
}
while (qsize-- && q->m_pNext)
{
tmp = q->m_pNext;
sortedList.Add(q);
q = tmp;
}
p = q;
}
k <<= 1;
m_oHead.m_pNext = sortedList.m_oHead.m_pNext;
m_oTail.m_pPrev = sortedList.m_oTail.m_pPrev;
m_oHead.m_pNext->m_pPrev = &m_oHead;
m_oTail.m_pPrev->m_pNext = &m_oTail;
sortedList.m_oHead.m_pNext = &sortedList.m_oTail;
sortedList.m_oTail.m_pPrev = &sortedList.m_oHead;
} while (numMerges > 1);
}

MemSys_List::MemSys_List()
{
m_oHead.m_pNext = &m_oTail;
m_oHead.m_pPrev = 0;
m_oTail.m_pPrev = &m_oHead;
m_oTail.m_pNext = 0;
}

MemSys_List::~MemSys_List()
{
}


void MemSys_List::Add(MemSys_Node *pNode)
{
pNode->m_pNext = &m_oTail;
pNode->m_pPrev = m_oTail.m_pPrev;
m_oTail.m_pPrev->m_pNext = pNode;
m_oTail.m_pPrev = pNode;
}

void MemSys_List::Rem(MemSys_Node *pNode)
{
pNode->m_pPrev->m_pNext = pNode->m_pNext;
pNode->m_pNext->m_pPrev = pNode->m_pPrev;
}

MemSys_List g_MemSys_MemList;

#define OBTAINMUT g_MemSys_MemList.m_oMutex.Obtain()
#define RELEASEMUT g_MemSys_MemList.m_oMutex.Release()


void *MemSys_TrackedAllocate(size_t s, char *pFile, unsigned long nLine, unsigned long nType)
{
MemSys_Node *pMem = (MemSys_Node *)malloc(s + sizeof(MemSys_Node));
if (pMem)
{
pMem->m_nDebugID = g_MemSys_DebugID;
pMem->m_pFile = pFile;
pMem->m_nLine = nLine;
pMem->m_nSize = s;
pMem->m_nType = nType;
OBTAINMUT;
g_MemSys_MemList.Add(pMem);
RELEASEMUT;
pMem++;
}
#if MEMSYSTEM_THROW
else
{
if (nType != MemSys_AllocStyleC)
{
throw EException_NoMemory();
}
}
#endif
return pMem;
}

void *MemSys_TrackedReAllocate(void *pMem, size_t s)
{
MemSys_Node *pMemory = (MemSys_Node *)pMem;
MemSys_Node *pNewMemory;
char *pFile;
unsigned long nLine;
unsigned long nID;

pMemory--;
EASSERT(pMemory->m_nType == MemSys_AllocStyleC);
EASSERT(pMemory->m_pNext);
EASSERT(pMemory->m_pNext->m_pPrev == pMemory);
EASSERT(pMemory->m_pPrev);
EASSERT(pMemory->m_pPrev->m_pNext == pMemory);

pFile = pMemory->m_pFile;
nLine = pMemory->m_nLine;
nID = pMemory->m_nDebugID;
OBTAINMUT;
g_MemSys_MemList.Rem(pMemory);
pNewMemory = (MemSys_Node *)realloc(pMemory, s + sizeof(MemSys_Node));
if (!pNewMemory)
{
g_MemSys_MemList.Add(pMemory);
RELEASEMUT;
return 0;
}

pNewMemory->m_nDebugID = nID;
pNewMemory->m_nLine = nLine;
pNewMemory->m_pFile = pFile;
pNewMemory->m_nSize = s;
pNewMemory->m_nType = MemSys_AllocStyleC;
g_MemSys_MemList.Add(pNewMemory);
RELEASEMUT;
pNewMemory++;
return pNewMemory;
}

void MemSys_TrackedFree(void *pMem, unsigned long nType)
{
// delete 0, or free(0) are acceptable.
if (pMem == 0)
return;

MemSys_Node *pMemory = (MemSys_Node *)pMem;

pMemory--;
EASSERT(pMemory->m_nType == nType);
EASSERT(pMemory->m_pNext);
EASSERT(pMemory->m_pNext->m_pPrev == pMemory);
EASSERT(pMemory->m_pPrev);
EASSERT(pMemory->m_pPrev->m_pNext == pMemory);
OBTAINMUT;
g_MemSys_MemList.Rem(pMemory);
RELEASEMUT;
free (pMemory);
}

void MemSys_PatchOwnerInfo(void *pMem, char *pFile, unsigned long nLine)
{
MemSys_Node *pMemory = (MemSys_Node *)pMem;

pMemory--;
EASSERT(pMemory->m_pNext);
EASSERT(pMemory->m_pPrev);
EASSERT(pMemory->m_pNext->m_pPrev == pMemory);
EASSERT(pMemory->m_pPrev->m_pNext == pMemory);
pMemory->m_pFile = pFile;
pMemory->m_nLine = nLine;
}




#if MEMSYSTEM_ENABLE_GLOBAL == 1
void *operator new(size_t s)
{
return MemSys_TrackedAllocate(s, 0, 0, MemSys_AllocStyleCPP);
}

void *operator new[](size_t s)
{
return MemSys_TrackedAllocate(s, 0, 0, MemSys_AllocStyleCPPArray);
}

void operator delete(void *pMem)
{
MemSys_TrackedFree(pMem, MemSys_AllocStyleCPP);
}

void operator delete[](void *pMem)
{
MemSys_TrackedFree(pMem, MemSys_AllocStyleCPPArray);
}


void *operator new(size_t s, char *pFile, unsigned long nLine)
{
return MemSys_TrackedAllocate(s, pFile, nLine, MemSys_AllocStyleCPP);
}

void *operator new[](size_t s, char *pFile, unsigned long nLine)
{
return MemSys_TrackedAllocate(s, pFile, nLine, MemSys_AllocStyleCPPArray);
}

void operator delete(void *pMem, char *pFile, unsigned long nLine)
{
MemSys_TrackedFree(pMem, MemSys_AllocStyleCPP);
}

void operator delete[](void *pMem, char *pFile, unsigned long nLine)
{
MemSys_TrackedFree(pMem, MemSys_AllocStyleCPPArray);
}
#endif





#if MEMSYSTEM_ENABLE_PLACEMENT == 1
void *operator new(size_t s, void *pAddr)
{
return pAddr;
}

void *operator new[](size_t s, void *pAddr)
{
return pAddr;
}

void operator delete(void *pMem, void *pAddr)
{
}

void operator delete[](void *pMem, void *pAddr)
{
}
#endif





#if MEMSYSTEM_ENABLE_CFUNC == 1
void *MemSys_Malloc(size_t n, char *pFile, unsigned long nLine)
{
return MemSys_TrackedAllocate(n, pFile, nLine, MemSys_AllocStyleC);
}

void *MemSys_Calloc(size_t n, size_t m, char *pFile, unsigned long nLine)
{
void *pMem;

pMem = MemSys_TrackedAllocate(n*m, pFile, nLine, MemSys_AllocStyleC);
memset(pMem, 0, n*m);
return pMem;
}

void *MemSys_ReAlloc(void *ptr, size_t n, char *pFile, unsigned long nLine)
{
if (n == 0)
{
if (ptr)
MemSys_Free(ptr);
return 0;
}

if (ptr)
return MemSys_TrackedReAllocate(ptr, n);
else
return MemSys_TrackedAllocate(n, pFile, nLine, MemSys_AllocStyleC);
}

void MemSys_Free(void *ptr)
{
MemSys_TrackedFree(ptr, MemSys_AllocStyleC);
}
#endif

void MemSys_NumberToString(char *pBuffer, unsigned long nLen, unsigned long nValue)
{
char *pOut = &pBuffer[nLen-1];
long n = 0;

memset(pBuffer, ' ', nLen);
pBuffer[nLen] = 0;
do
{
*pOut = (char) ((nValue % 10) + '0');
nValue = nValue / 10;
pOut--;
n++;
if (n==3 && nValue)
{
n = 0;
*pOut = ',';
pOut--;
}
}
while (nValue != 0);
}

unsigned long MemSys_BeginTrackingSection(void)
{
g_MemSys_DebugID++;
return g_MemSys_DebugID;
}

// NOTE THIS VERSION OF MY MEMORY DUMP FUNC ISN'T COMPLETE
void MemSys_DumpAllocationsSince(unsigned long nTrackingID, bool bSingleAlloc, bool bLine, bool bFile, bool bTotal)
{
char tempBuffer[512];
unsigned long nPrevLine = 0xFFFFFFFF;
char *pPrevFile = (char *)1;

unsigned long nAllocsPerLine=0, nMemPerLine=0;
unsigned long nAllocsPerFile=0, nMemPerFile=0;
unsigned long nAllocsTotal=0, nMemTotal=0;
MemSys_Node *pNode;

OBTAINMUT;
g_MemSys_MemList.Sort();
for (pNode = g_MemSys_MemList.m_oHead.m_pNext; pNode->m_pNext; pNode = pNode->m_pNext)
{
if (pNode->m_nDebugID <= nTrackingID)
{
EASSERT(false);
// WRITEME;
// File change? Dump last line and file
if (pPrevFile != pNode->m_pFile)
{
if (pPrevFile != (char *)1)
{
if (!bSingleAlloc)
{
// Line dump
}
if (nAllocsPerLine != nAllocsPerFile)
{
// File dump
}
}
}
else if (nPrevLine != pNode->m_nLine)
{
if (!bSingleAlloc)
{
// Line change? Dump last line
}
}

// Dump single allocs
// Update line values
// Update file values
// Update total values.

sprintf (tempBuffer, "%s(%d): %d bytes\n", pNode->m_pFile, pNode->m_nLine, pNode->m_nSize);
EOutputDebugString(tempBuffer);
}
}
RELEASEMUT;
}

unsigned long MemSys_MemoryAllocatedSince(unsigned long nTrackingID)
{
unsigned long nMem = 0;
MemSys_Node *pNode;

OBTAINMUT;
for (pNode = g_MemSys_MemList.m_oHead.m_pNext; pNode->m_pNext; pNode = pNode->m_pNext)
{
if (pNode->m_nDebugID <= nTrackingID)
nMem += pNode->m_nSize;
}
RELEASEMUT;
return nMem;
}


Share this post


Link to post
Share on other sites
Your operator new shouldn't, or doesn't need to have a size parameter. Use "sizeof" instead. What you're doing may be calling the array version to alloc, not your one. step through your code line by line and make sure it goes exactly where you expect the whole time.

Share this post


Link to post
Share on other sites
Quote:
Original post by iMalc
Your operator new shouldn't, or doesn't need to have a size parameter. Use "sizeof" instead. What you're doing may be calling the array version to alloc, not your one. step through your code line by line and make sure it goes exactly where you expect the whole time.

As was pointed out, it IS required.

Also, it's necessary for new[] to know how much memory is needed.

Also it's necessary in a global new since you don't know which class you're doing the sizeof() for.

Also if you ever subclass the class with a class-based new operator it the size parameter lets you know the accurate amount of memory to allocate, instead of you insisting on allocating too little.

Also, as I said, there may be compiler dependant extra data, such as the array size, which is added into the size. If you use sizeof, you're guaranteeing your operator new won't work on ANY class.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this