Allocator crash after alloc/dealloc

Started by
11 comments, last by l0k0 11 years, 3 months ago

'ptr' does not point to itself. Even if it did this would not be correct, since the arg is a copy with the same value rather than the same pointer. You need to search out the pointer value manually.

virtual void dealloc(char* ptr, size_t = 0) {MemChunk *pCurChunk = (MemChunk*) ((char*) ptr - sizeof(MemChunk));


You must have edited this out, but I looked into it and finally made sense of it. After looking it over, I realized that this line of code was going back 8 bytes from 'ptr' but my real intention was to basically subtract 8 [ sizeof (MemChunk* )* 2 ] from the actual address of the pointer. I _think_ I accomplished that, like this.
MemChunk *pCurChunk = (MemChunk*) reinterpret_cast<char*>(reinterpret_cast<ptrdiff_t>(ptr)- static_cast<ptrdiff_t>(sizeof(MemChunk*) * 2));
Whether this fixed my problem is still yet to be tested extensively, but it passed the quick test I put together for it.

I also took your advice and added a data member to the MemChunk struct which does allow me to use an array index, making things a lot more readable.

Lastly, I realized that the addresses from mFirst to mLast were descending, which was the opposite of what I wanted it to do (which accounts for the weird subtraction in the getPtr() function). To fix that, I made the loop that links the nodes together run backwards (from mMaxSize to 0). That allowed me to directly access the memory in the getPtr function the way I intended.



Ah yeah, I edited that post like 20 times as I was wrestling with the forum and also trying to understand what the code as up to. I was confused as to what direction the list was going, but there's a lot of other stuff in there that seemed weird to me as well. I recognize that this is experimentation code. If I may point a few things out about the general design:

If you can access the allocated members by array index there's no need for MemChunk. You're just implementing a very complicated array, or perhaps reinventing std::vector at length. If all elements are equivalent in type then an indexed allocation is all you need, right?

In the case that you're wanting to be able to allocate multiple objects with one call then there's a simpler approach that may appeal to you:

Make yourself a struct like MemChunk. It needs next/prev pointers and should contain a 'size' indicator of some sort (element count or byte length or both). It may also contain metadata, such as the name of the allocation or whatever.

In your allocator class create a pointers to this type, 'root', 'head' and 'tail' or whatever. Point them all to the base of the allocation and zerofill the first sizeof(YourStruct) bytes of the allocation.

'root' shall never move. You need it to free the master allocation.

When you want to allocate, set the 'byteSize' member of 'tail' to the number of bytes you want, set your other metadata, then calculate a new pointer position: (sizeof(YourStruct) + byteSize). Set the 'next' member of 'tail' to that value, record the position of 'tail' then set 'tail' to tail->next. Finally, set tail->prev to the old 'tail' value and tail->next to NULL, indicating the end of the list.

Essentially 'tail' refers to the next allocatable space. For any linked node, if node->next is NULL then that's the end of the list and no valid data follows it. If root->next is NULL then no allocations are present.

To delete/free an allocation via its pointer simply subtract sizeof(YourStruct) from that pointer and unlink the node at that location. Check to see if the node is == head, and if so then unlink simply by saying head = head->next. Otherwise do a bidirectional unlink as normal.

You can enumerate determine the available space as follows:

endPtr = (address of root) + (size of master allocation)
available bytes = endPtr - (address of tail)

Just ensure that you use a byte-length type when you do the math there, or better yet, convert the pointers to an size_t and do the math on those. (size_t is unsigned 64-bit integer on an x64 system)

If you delete allocations that are not at the end then the memory will become fragmented. Rather than trying to cram things into the cracks just throw a bool indicating that a deallocation has caused fragmentation. If you run out of space and the fragmented flag is true you can traverse the nodes and compact the memory using &lt;string.h&gt; memmove() (I'll probably catch hell for saying that since it's a C function.). At that point check the available space as mentioned above and if it's still not enough then you can reallocate the master and copy in the data. Alternatively you can have const functions that return the available space and fragmentation status and expose the realloc and defrag functions. (if realloc is called when fragmentation is true just defrag into the new allocation)

Hope that helps. smile.png

Semi-edit - I clicked post and my phone took a crap, so I C&P my post body and reloaded, then Codarki had posted. Sorry if I'm reiterating. I consider myself ninja'd.

(OMG I'm about to go super saiyan and punch this parser in the dong.)
void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.
Advertisement

Does this not strike you as being a little silly?:


		mMemory = new (std::nothrow) char[mMaxSize * mOffset];
		// mMemory = malloc(mMaxSize * mOffset);
		if (!mMemory)
		{
			//Do something more informational here
			throw std::bad_alloc();
			return;
		}
I mean why explicitly use nothrow only to ultimately end up replicating the throw behaviour?
The return is also pointless since it can never be reached.

I'm aware of this, thank you. This also wasn't what my post was about, the code originally had other behavior but in the interim of deciding how to refactor the code, I changed it (rather than deleting it and letting new throw the exception). As I said in the OP, it's a learning exercise, so if you have nothing worth while to contribute, please don't.

@Khatharr, Thank you for your suggestions, I actually wrote this a long time ago and didn't understand what I was doing( to an even greater extent than I do now xD). I'll try you suggestions and see where they take me. If I run into any problems I can't fix on my own, I'll find myself right back here in the forum.

If I had to guess it seems you might be reading/writing unaligned data. Throw in some asserts that assure the data you are reading/writing to is aligned. You can read more about this here: http://stackoverflow.com/questions/227897/solve-the-memory-alignment-in-c-interview-question-that-stumped-me.

2 Other Notes Here:

1) Usually with pools the free list nodes are stored inside the slots in the pool themselves (sometimes as unsigned 16 bit offsets from the start of the pool to conserve space). By that I mean there is no need for the MemChunk and the Data itself to be stored next to eachother, but rather in the same location in memory (sort of like a union). If you think about it, if there is no data there, there is no need to have seperate space for both. If it is in the free list, it is by definition unused.

2) Why is memchunk doubly linked? If deallocating, just add to the head of the free list (newChunk->next = freeListHead). And when allocating take the node at the front of the linked list. freeListHead = freeListHead->next. Would simplify things no?

Point number 1 above might be the source of the unaligned data in the first place. :)

<shameless blog plug>
A Floating Point
</shameless blog plug>

This topic is closed to new replies.

Advertisement