Giel van Schijndel schreef:
> Giel van Schijndel schreef:
>
>> Christian Ohm schreef:
>>
>>> On Thursday, 28 December 2006 at 20:06, Dennis Schridde wrote:
>>>
>>>> I think not too long ago Per said something like simply replacing MALLOC
>>>> was
>>>> not possible... Dunno if I remember that correctly or how his patch fixed
>>>> that...
>>>>
>>> Just defining MALLOC/FREE to malloc/free etc. (see below) compiles a
>>> binary that runs at least the tutorial for a short while (haven't done
>>> more testing). So I guess we could just add those defines
>>> unconditionally, and if nothing breaks for a while, remove them from the
>>> code itself.
>>>
>>> --- lib/framework/mem.h (revision 579)
>>> +++ lib/framework/mem.h (working copy)
>>> @@ -78,5 +78,10 @@
>>>
>>> #endif // DEBUG_MALLOC
>>>
>>> +#define MALLOC(size) malloc(size)
>>> +#define FREE(ptr) free(ptr)
>>> +#define PTRVALID(ptr, size) (TRUE)
>>> +#define MEMORYREPORT(file)
>>> +#define MEMORYTREAP(file)
>>>
>>> #endif
>>>
>> While I'm sure that will work correctly in any release build I'm not so
>> sure it will in the debug build.
>> That's because the debug build assigns a struct around the malloc'ed
>> memory for memory tracing.
>>
>> Also you might want to use a bit different define for FREE, the current
>> codebase _might_ depend upon it, to prevent accessing freed memory:
>> #define FREE(ptr) do { free(ptr); ptr = NULL; } while(0)
>>
> Ok, pretend I didn't say using malloc() and free() like that would work
> fine. Because it won't, not as long as the blockheap memory system is
> being used. This system turned on by memSetBlockHeap(!NULL) and off by
> memSetBlockHeap(NULL), so before you can use plain malloc() and free()
> you'll have to remove all calls to memSetBlockHeap() (blockheap system
> is turned off on initialization of the memory system).
Well, I've done some cleaning up in the memory system, result is the
attached patch.This patch does the following: * remove lots of code that never gets executed (because of if(0), #if(0), or by using other condition checking which always sees a 0 as condition through some kind of function or variable). * remove code that doesn't actually do anything ( debug ( LOG_NEVER, "..." ); included) (e.g. calculating and assigning values to a variable which then never gets referred again) * remove code that *does* do something but is completely useless (e.g. setting all memory to 0x00 before passing it to free() ) * remove a whole lot of (unused/uncalled) debugging functions that do nothing more than outputting/printing stats about the current state of the memory allocations/usage * remove (or comment out) code that encourages buggy programming (e.g. assuming all acquired memory is initialized) * remove some preprocessor macros which don't ever get used (yes, I searched/scanned every single source file in the trunk) * turned some ASSERT (FALSE, "...") statements into the condition they are actually guarding rather than depending on the programmer to go and look for the if(...) condition right above this ASSERT * fixed (two I believe) some compiler warnings * inserted some comments to clarify what some very odd looking non-intuitive code does * moved variable declaration more down into code where it is initialized (i.e. applying RAII there where it easy to do so) * fixed some minor bugs (e.g. triggering two ASSERTS, while the first one was well enough to predict the second one would occur as well) If I've done everything correct, then all this patch should do is cleanup code _almost_ (i.e. some double triggered asserts will be gone, and some bugs because of lacking RAII will now not occur) without altering the program's behaviour. -- Giel
Index: lib/framework/block.c
===================================================================
--- lib/framework/block.c (revision 579)
+++ lib/framework/block.c (working copy)
@@ -131,14 +131,6 @@
{
BLOCK_HEAP_MEM *psCurr, *psNext;
-#ifdef DEBUG_BLOCK
- if (psHeap->psMemTreap != NULL)
- {
- debug( LOG_NEVER, "blkDestroy: %s at %d: memory allocated :\n",
psHeap->pFileName, psHeap->line );
- memRecReport(psHeap->psMemTreap);
- }
-#endif
-
LIST_REMOVE(psBlockList, psHeap, BLOCK_HEAP);
for(psCurr=psHeap->psBlocks; psCurr; psCurr=psNext)
@@ -150,97 +142,6 @@
RFREE(psHeap);
}
-void memMemoryDump(MEM_NODE *Node);
-
-
-void blkPrintDetails(BLOCK_HEAP *psHeap)
-{
- if (psHeap!=NULL)
- {
-
-#ifdef DEBUG_BLOCK
- UDWORD Left =
(UDWORD)((psHeap->psBlocks->pMem)+(psHeap->psBlocks->size)-(psHeap->psBlocks->pFree));
- debug( LOG_NEVER, "ptr=%p init=%d ext=%d used=%d (Start=$%p
Free=$%p Left=%d)\n", psHeap,psHeap->init, psHeap->ext,psHeap->TotalAllocated,
psHeap->psBlocks->pMem, psHeap->psBlocks->pFree, Left );
- memMemoryDump(psHeap->psMemTreap);
-#else
- debug( LOG_NEVER, "ptr=%p init=%d ext=%d\n", psHeap,
psHeap->init, psHeap->ext );
-#endif
-
- }
- else
- {
- debug( LOG_NEVER, "NULL POINTER IN BLOCK LIST\n" );
- }
-
-}
-
-// report on the blocks
-void blkReport(void)
-{
-
-
-
-#ifdef DEBUG
- UDWORD BlockNumber=0;
- BLOCK_HEAP *psCurHeap;
-
- debug( LOG_NEVER, "\n\nBlock Report. Current Block=%p:\n",
memGetBlockHeap() );
-
- psCurHeap=psBlockList;
-
- while (psCurHeap)
- {
- debug( LOG_NEVER, "Block %d)",BlockNumber++ );
- blkPrintDetails(psCurHeap);
-
- psCurHeap = psCurHeap->psNext;
- }
- debug( LOG_NEVER, "\n\n" );
-#endif
-}
-
-#if(0) // no longer used - uploaded in small chunks
-// This is a special free that checks to see if we can free up the memory
-// We can only do this if it is the last allocated memory in the block
-//
-// This can be used for scratch memory ... Critical on the Playstation where
memory is so tight
-//
-// - e.g. The sound data must be stored as one 400k file. This must be
loaded into scratch memory
-// we clearly do not have 400k of spare memory around.
-//
-//
-// Returns true or false
-//
-BOOL blkSpecialFree(BLOCK_HEAP *psHeap, void *Ptr)
-{
- BLOCK_HEAP_MEM *psCurr;
-
- UDWORD RequestedFreeMem=(UDWORD)Ptr;
-
- for(psCurr = psHeap->psBlocks; psCurr; psCurr = psCurr->psNext)
- {
-
- if ((UDWORD)psCurr->pLastAllocated == RequestedFreeMem)
- {
-#ifdef DEBUG_BLOCK
-
- UDWORD
BlockSize=((UDWORD)psCurr->pFree)-RequestedFreeMem;
- debug( LOG_NEVER, "FREED %d block bytes\n", BlockSize
); // del me now !
- psHeap->TotalAllocated-=BlockSize;
-#endif
-
- psCurr->pFree = psCurr->pLastAllocated;
- psCurr->pLastAllocated=0; // remove pointer so
that it cant be allocated again
-
-
- return(TRUE); // able to
return mem
- }
- }
- return(FALSE); // unable to free mem
-
-}
-#endif
-
// Allocate some memory from a block heap
void *blkAlloc(BLOCK_HEAP *psHeap, SDWORD size)
{
@@ -449,31 +350,6 @@
// pMemToFree = pMemToFree;
#endif
- {
-// BOOL bRes;
-
-#if(1)
-
- //DBPRINTF(("UNABLE TO FREE MEMORY\n"));
-
-
-#else
- bRes=blkSpecialFree(psHeap, pMemToFree); // Free it up
if we can ! - we can only free the last entry
-#ifdef DEBUG
- if (bRes==TRUE)
- {
- debug( LOG_NEVER, "blkFree called - memory successfully
released\n" );
- }
- else
- {
-// debug( LOG_NEVER, "blkFree called - memory NOT
released\n" );
- }
-#endif
-#endif
-
-
- }
-
}
@@ -488,11 +364,6 @@
#endif
#ifdef DEBUG_BLOCK
- if (psHeap->psMemTreap != NULL)
- {
- debug( LOG_NEVER, "blkReset: %s at %d: memory allocated :\n",
psHeap->pFileName, psHeap->line );
- memRecReport(psHeap->psMemTreap);
- }
psHeap->psMemTreap = NULL;
psHeap->free = FALSE;
@@ -516,8 +387,6 @@
psCurr->pFree = psCurr->pMem;
psCurr = psCurr->psNext;
}
-
- debug( LOG_NEVER, " Blocks allocated %dk, Memory allocated %dk\n",
block/1024, alloc/1024 );
}
@@ -626,24 +495,3 @@
psSuspendedHeap=NULL;
}
-
-
-
-static void blockCurrentBlockInfo(void)
-{
-#ifdef DEBUG
- BLOCK_HEAP *psCurHeap;
-
- psCurHeap=memGetBlockHeap();
- if (psCurHeap==NULL)
- {
- debug( LOG_NEVER, "*** No current block defined\n" );
- }
- else
- {
- UDWORD
Left=(UDWORD)((psCurHeap->psBlocks->pMem)+(psCurHeap->psBlocks->size)-(psCurHeap->psBlocks->pFree));
-
- debug( LOG_NEVER, "ptr=%p init=%d ext=%d used=%d (Start=$%p
Free=$%p Left=%d)\n", psCurHeap, psCurHeap->init, psCurHeap->ext,
psCurHeap->TotalAllocated, psCurHeap->psBlocks->pMem,
psCurHeap->psBlocks->pFree, Left );
- }
-#endif
-}
Index: lib/framework/block.h
===================================================================
--- lib/framework/block.h (revision 579)
+++ lib/framework/block.h (working copy)
@@ -83,9 +83,6 @@
// Note the call position for a blkAlloc or blkFree
extern void blkCallPos(const char *pFileName, SDWORD line);
-void blkPrintDetails(BLOCK_HEAP *psHeap);
-void blkReport(void);
-BOOL blkSpecialFree(BLOCK_HEAP *psHeap, void *Ptr);
void blockSuspendUsage(void);
void blockUnsuspendUsage(void);
Index: lib/framework/mem.c
===================================================================
--- lib/framework/mem.c (revision 579)
+++ lib/framework/mem.c (working copy)
@@ -32,7 +32,11 @@
* is trashed before it is freed.
* This is done automatically by Visual C's memory routines.
*/
-#define MEMORY_SET TRUE
+// No piece of code should ever depend upon memory being initialized (RAII:
Resource Acquisition Is Initialization)
+// Therefore I humbly comment this line out, which might very well result in
+// buggy code becoming even buggier (just search this file for the next
+// instance of MEMORY_SET). -- Giel
+//#define MEMORY_SET TRUE
/* Number of bytes after which memory amounts are displayed in Kb */
#define SHOW_KB_LIMIT (0x400)
@@ -83,9 +87,6 @@
/* Shutdown the memory system */
void memShutDown(void)
{
- // Report any memory still allocated
- memMemoryReport();
-
// Free up the allocated memory
memTreapDestroy((TREAP_NODE *)psMemRoot);
}
@@ -159,25 +160,23 @@
}
pMemBase = RMALLOC( Size + sizeof(MEM_NODE) + 2 * SAFETY_ZONE_SIZE );
- if (!pMemBase)
+ if (pMemBase == NULL)
{
-
-
- ASSERT( FALSE, "Warning: malloc returning NULL - [%s -
%d]",pFileName,LineNumber );
- debug( LOG_NEVER, "[%s - %d] %d bytes\n", pFileName,
LineNumber, Size );
+ ASSERT( pMemBase != NULL, "Warning: malloc returning NULL - [%s
- %d] (size %d bytes)", pFileName, LineNumber, Size );
return NULL;
}
/* Got the main bit of memory - set up the node entry */
+ // This part of code adds information about which file requested
+ // on what line what amount of memory.
psNode = (MEM_NODE *)pMemBase;
- psNode->pFile = (char *)RMALLOC( strlen(pFileName)+1 );
- if (!psNode->pFile)
+ psNode->pFile = (const char *)RMALLOC( strlen(pFileName)+1 );
+ if (psNode->pFile == NULL)
{
RFREE(pMemBase);
- debug( LOG_NEVER, "Warning: malloc returning NULL" );
return NULL;
}
- strcpy(psNode->pFile, pFileName);
+ strcpy((char *)psNode->pFile, pFileName);
psNode->line = LineNumber;
psNode->size = Size;
@@ -190,12 +189,15 @@
treapAddNode((TREAP_NODE **)&psMemRoot, (TREAP_NODE *)psNode,
memBlockCmp);
/* Now initialise the memory - try to catch unitialised variables */
- memset((UBYTE *)(pMemBase) + sizeof(MEM_NODE),
+ // Fill up the safety zone left of (or before) and right of (or after)
+ // the requested memory page, with SAFETY_ZONE_BYTE to be able to
+ // catch writing out of bounds.
+ memset((UBYTE *)(pMemBase) + sizeof(MEM_NODE),
// Left
SAFETY_ZONE_BYTE, SAFETY_ZONE_SIZE);
- memset((UBYTE *)(pMemBase) + sizeof(MEM_NODE) + SAFETY_ZONE_SIZE + Size,
+ memset((UBYTE *)(pMemBase) + sizeof(MEM_NODE) + SAFETY_ZONE_SIZE +
Size, // Right
SAFETY_ZONE_BYTE, SAFETY_ZONE_SIZE);
#ifdef MEMORY_SET
- /* The PC initialises malloc'ed memory already, no need to do it again
*/
+ // Initialize memory before returning its address (RAII!!!)
memset((UBYTE *)(pMemBase) + sizeof(MEM_NODE) + SAFETY_ZONE_SIZE,
INITIALISE_BYTE, Size);
#endif
@@ -230,21 +232,13 @@
void memFree(const char *pFileName, SDWORD LineNumber, void *pMemToFree)
{
MEM_NODE sNode, *psDeleted;
- SDWORD i, InvalidBottom, InvalidTop;
- UBYTE *pMemBase;
BLOCK_HEAP *psBlock;
-#ifdef MEMORY_SET
- SDWORD Size;
-#endif
- (void)LineNumber;
- (void)pFileName;
-
-
- ASSERT( (pFileName != NULL), "No filename passed to mem_Free" );
+ ASSERT( (pFileName != NULL), "No filename passed to memFree" );
ASSERT( (pMemToFree != NULL), "Attempt to free NULL pointer, called
by:\n"
"File:
%s\nLine: %d\n", pFileName, LineNumber );
+ if (pMemToFree == NULL) return;
// see if the pointer was allocated in a block
psBlock = blkFind(pMemToFree);
@@ -267,15 +261,19 @@
ASSERT( psDeleted != NULL,
- "Invalid pointer passed to mem_Free by:\n"
+ "Invalid pointer passed to memFree by:\n"
"File: %s\nLine: %d\n\n"
"Attempt to free already freed pointer?",
pFileName, LineNumber );
if (psDeleted)
{
/* The pointer is valid, check the buffer zones */
- pMemBase = (UBYTE *)(pMemToFree) - SAFETY_ZONE_SIZE;
- InvalidBottom = InvalidTop = 0;
+ UBYTE* pMemBase = (UBYTE *)(pMemToFree) - SAFETY_ZONE_SIZE;
+ UWORD InvalidBottom = 0;
+ UWORD InvalidTop = 0;
+ UWORD i = 0;
+
+ // Check wether out of bound writes have occured since
memMalloc()
for(i=0; i<SAFETY_ZONE_SIZE; i++)
{
if (pMemBase[i] != SAFETY_ZONE_BYTE)
@@ -298,15 +296,9 @@
psDeleted->pFile, psDeleted->line,
pFileName, LineNumber );
- /* Trash the memory before it is freed (The PC already does
this) */
-#ifdef MEMORY_SET
- Size = psDeleted->size;
- memset(pMemToFree, FREE_BYTE, Size);
-#endif
-
/* Now free the memory */
- RFREE(psDeleted->pFile);
+ RFREE((char *)psDeleted->pFile);
RFREE(psDeleted);
}
@@ -316,10 +308,8 @@
/* Replacement for Free for release builds */
void memFreeRelease(void *pMemToFree)
{
- BLOCK_HEAP *psBlock;
-
// see if the pointer was allocated in a block
- psBlock = blkFind(pMemToFree);
+ BLOCK_HEAP* psBlock = blkFind(pMemToFree);
if (psBlock != NULL)
{
// use a block heap rather than normal free
@@ -327,7 +317,6 @@
return;
}
-
RFREE(pMemToFree);
}
@@ -364,149 +353,3 @@
return FALSE;
#endif
}
-
-
-/* Recursive function to print out the list of memory blocks */
-SDWORD memRecReport(MEM_NODE *psRoot)
-{
-#ifdef REALLY_DEBUG_MALLOC
- if (psRoot)
- {
- if (psRoot->size < SHOW_KB_LIMIT)
- {
- debug(LOG_MEMORY, "memRecReport for %s line %d: \t%d
bytes",
- psRoot->pFile, psRoot->line, psRoot->size);
- }
- else
- {
- debug(LOG_MEMORY, "memRecReport for %s line %d: \t%d
kilobytes",
- psRoot->pFile, psRoot->line, (int) psRoot->size /
1024);
- }
-
- return memRecReport((MEM_NODE *)psRoot->psLeft) +
- memRecReport((MEM_NODE *)psRoot->psRight) +
- psRoot->size;
- }
-#endif
- psRoot = psRoot;
- return 0;
-}
-
-#ifdef DEBUG_MALLOC
-#define MAXMODULES (32)
-typedef struct
-{
- char pFile[128];
- int Count;
- int Total;
-} MEMMOD;
-static MEMMOD MemModuleInfo[MAXMODULES];
-
-static UDWORD MemTotalEntries;
-static UDWORD MemTotalModules;
-static UDWORD MemTotalAllocated;
-#endif
-
-/* Recursive function to total up the amount of mem allocated */
-static void memSummary(MEM_NODE *psRoot)
-{
-#ifdef DEBUG_MALLOC
-
- // bsort
- if (psRoot)
- {
- int i;
- BOOL FoundModule;
-
- MemTotalEntries++;
- MemTotalAllocated+=psRoot->size;
-
- FoundModule=FALSE;
- for (i=0;i<(SDWORD)MemTotalModules;i++)
- {
- if (strcmp(psRoot->pFile,MemModuleInfo[i].pFile)==0)
- {
- MemModuleInfo[i].Count++;
- MemModuleInfo[i].Total+=psRoot->size;
- FoundModule=TRUE;
- }
- }
- if (FoundModule==FALSE)
- {
- if (MemTotalModules <MAXMODULES)
- {
-
strcpy(MemModuleInfo[MemTotalModules].pFile,psRoot->pFile);
- MemModuleInfo[MemTotalModules].Count=1;
-
MemModuleInfo[MemTotalModules].Total=psRoot->size;
- MemTotalModules++;
- }
-
- }
-
-
-
- memSummary((MEM_NODE *)psRoot->psLeft);
- memSummary((MEM_NODE *)psRoot->psRight);
- }
- return ;
-#endif
-}
-
-void memMemoryDump(MEM_NODE *Node)
-{
-#ifdef DEBUG_MALLOC
-
- int i;
-
- MemTotalEntries=0;
- MemTotalModules=0;
- MemTotalAllocated=0;
- memSummary(Node);
-
- debug(LOG_MEMORY, "Memory Summary: %d bytes allocated in %d handy size
chunks", MemTotalAllocated, MemTotalEntries);
- for (i=0;i<(SDWORD)MemTotalModules;i++)
- {
- debug(LOG_MEMORY, "memMemoryDump: %d) [%s] %d allocations
totalling %d bytes",
- i, MemModuleInfo[i].pFile, MemModuleInfo[i].Count,
MemModuleInfo[i].Total);
- }
-#endif
-
-}
-
-/* Report on currently allocated memory.
- */
-void memMemoryReport(void)
-{
-#ifdef DEBUG_MALLOC
- SDWORD TotMem;
-
- if (!psMemRoot)
- {
- debug(LOG_MEMORY, "memMemoryReport: No memory allocated");
- }
- else
- {
- TotMem = memRecReport(psMemRoot);
-
- if (TotMem < SHOW_KB_LIMIT)
- {
- debug(LOG_MEMORY, "memMemoryReport: Total memory
allocated is %d bytes", TotMem);
- }
- else
- {
- debug(LOG_MEMORY, "memMemoryReport: Total memory
allocated is %d kilobytes", (int) TotMem / 1024);
- }
-
- }
-#endif
-}
-
-
-/* Display the memory treap */
-void memDisplayTreap(void)
-{
-#ifdef DEBUG_MALLOC
- debug(LOG_MEMORY, "Memory Allocation Treap:");
- treapDisplayRec((TREAP_NODE *)psMemRoot, 0);
-#endif
-}
Index: lib/framework/mem.h
===================================================================
--- lib/framework/mem.h (revision 579)
+++ lib/framework/mem.h (working copy)
@@ -17,7 +17,6 @@
#define DEBUG_HEAP TRUE
/* Uncomment below for even more debug spam */
#undef REALLY_DEBUG_HEAP
-#undef REALLY_DEBUG_MALLOC
#else
@@ -50,12 +49,6 @@
/* Check a pointer refers to a valid block of memory */
BOOL memPointerValid(void *pPtr, size_t Size);
-/* Report on currently allocated memory */
-void memMemoryReport(void);
-
-/* Display the memory treap */
-void memDisplayTreap(void);
-
#ifdef DEBUG_MALLOC
#define MALLOC(size) memMalloc(__FILE__, __LINE__, size)
@@ -65,16 +58,12 @@
#else
#define PTRVALID(ptr, size) (((ptr)==NULL)?FALSE:TRUE)
#endif
-#define MEMORYREPORT(file) memMemoryReport(file)
-#define MEMORYTREAP(file) memDisplayTreap(file)
#else // !DEBUG_MALLOC
#define MALLOC(size) memMallocRelease(size)
#define FREE(ptr) do { memFreeRelease(ptr); ptr = NULL; } while(0)
#define PTRVALID(ptr, size) (TRUE)
-#define MEMORYREPORT(file)
-#define MEMORYTREAP(file) memDisplayTreap(file)
#endif // DEBUG_MALLOC
Index: lib/framework/memint.h
===================================================================
--- lib/framework/memint.h (revision 579)
+++ lib/framework/memint.h (working copy)
@@ -26,9 +26,4 @@
/* compare two memory blocks */
extern SDWORD memBlockCmp(void *key1, void *key2);
-
-/* Recursive function to print out the list of memory blocks */
-extern SDWORD memRecReport(MEM_NODE *psRoot);
-
#endif
-
Index: src/order.c
===================================================================
--- src/order.c (revision 579)
+++ src/order.c (working copy)
@@ -2486,7 +2486,6 @@
//#warning memory report here !!!!!
// DBPRINTF(("test printf\n"));
// DBPRINTF(("droid=%p\n",psDroid);
-// memMemoryReport(NULL);
ASSERT( PTRVALID(psDroid, sizeof(DROID)),
"orderUnitLoc: Invalid unit pointer" );
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
