Dennis Schridde schreef: > Am Freitag, 29. Dezember 2006 16:12 schrieb Giel van Schijndel: > >> 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. >> > Are you sure the removal of memMemoryReport() from memShutDown() doesn't > change anything? I remember reading some memory allocation stats on game > close... > > There are also some LOG_NEVERs which don't seem so unused... > > never: blkReset: ../src/init.c at 1024: memory allocated : > never: blkReset: ../src/init.c at 1024: memory usage: > never: Blocks allocated 24525k, Memory allocated 4484k > > memory: memMemoryReport: No memory allocated > The output of memMemoryReport depends upon the definition of REALLY_DEBUG_MALLOC. Which is definednowhere throughout the source, in fact it is only undefined once in lib/framework/mem.h:20 without being defined before.
If however it where to be defined then memRecReport (also found in mem.c) actually does something (otherwise it only returns 0). When REALLY_DEBUG_MALLOC isn't defined however, all memMemoryReport does is either: debug(LOG_MEMORY, "memMemoryReport: No memory allocated"); or debug(LOG_MEMORY, "memMemoryReport: Total memory allocated is %d bytes", TotMem); where TotMem = 0 so it really is this: debug(LOG_MEMORY, "memMemoryReport: Total memory allocated is 0 bytes"); When REALLY_DEBUG_MALLOC is defined however, memRecReport prints out (by means of debug( LOG_MEMORY, "..." )) a bunch of allocated memory chunks along with the file and line where it got requested plus the requested size. Then finally it also prints out the total used size. Conclusion: when REALLY_DEBUG_MALLOC is not defined then memMemoryReport is of no value, if however it is defined it might be useful in tracking memory usage. So I've adjusted my patch to only add some `#ifdef REALLY_DEBUG_MALLOC'-statements around the code that uses memMemoryReport. Then in for your info an explanation why in block.c there are three functions removed: blkPrintDetails, blkReport and blkSpecialFree. Except for blkPrintDetails none of them ever get called, and blkPrintDetails only gets called by blkReport. -- Giel
Index: lib/framework/block.c
===================================================================
--- lib/framework/block.c (revision 581)
+++ lib/framework/block.c (working copy)
@@ -135,7 +135,9 @@
if (psHeap->psMemTreap != NULL)
{
debug( LOG_NEVER, "blkDestroy: %s at %d: memory allocated :\n",
psHeap->pFileName, psHeap->line );
+#ifdef REALLY_DEBUG_MALLOC
memRecReport(psHeap->psMemTreap);
+#endif
}
#endif
@@ -150,97 +152,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 +360,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
-
-
- }
-
}
@@ -491,7 +377,9 @@
if (psHeap->psMemTreap != NULL)
{
debug( LOG_NEVER, "blkReset: %s at %d: memory allocated :\n",
psHeap->pFileName, psHeap->line );
+#ifdef REALLY_DEBUG_MALLOC
memRecReport(psHeap->psMemTreap);
+#endif
}
psHeap->psMemTreap = NULL;
psHeap->free = FALSE;
Index: lib/framework/block.h
===================================================================
--- lib/framework/block.h (revision 581)
+++ 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 581)
+++ 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)
@@ -84,7 +88,9 @@
void memShutDown(void)
{
// Report any memory still allocated
+#ifdef REALLY_DEBUG_MALLOC
memMemoryReport();
+#endif
// Free up the allocated memory
memTreapDestroy((TREAP_NODE *)psMemRoot);
@@ -159,25 +165,25 @@
}
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 );
+ debug( LOG_NEVER, "malloc returning NULL: [%s - %d] %d
bytes\n", 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 +196,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 is better!!!)
memset((UBYTE *)(pMemBase) + sizeof(MEM_NODE) + SAFETY_ZONE_SIZE,
INITIALISE_BYTE, Size);
#endif
@@ -230,21 +239,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 +268,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 +303,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 +315,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 +324,6 @@
return;
}
-
RFREE(pMemToFree);
}
@@ -366,10 +362,10 @@
}
+#ifdef REALLY_DEBUG_MALLOC
/* 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)
@@ -387,10 +383,9 @@
memRecReport((MEM_NODE *)psRoot->psRight) +
psRoot->size;
}
-#endif
- psRoot = psRoot;
return 0;
}
+#endif
#ifdef DEBUG_MALLOC
#define MAXMODULES (32)
@@ -473,11 +468,11 @@
}
+#ifdef REALLY_DEBUG_MALLOC
/* Report on currently allocated memory.
*/
void memMemoryReport(void)
{
-#ifdef DEBUG_MALLOC
SDWORD TotMem;
if (!psMemRoot)
@@ -498,10 +493,9 @@
}
}
-#endif
}
+#endif
-
/* Display the memory treap */
void memDisplayTreap(void)
{
Index: lib/framework/mem.h
===================================================================
--- lib/framework/mem.h (revision 581)
+++ lib/framework/mem.h (working copy)
@@ -50,11 +50,13 @@
/* Check a pointer refers to a valid block of memory */
BOOL memPointerValid(void *pPtr, size_t Size);
+#ifdef REALLY_DEBUG_MALLOC
/* Report on currently allocated memory */
void memMemoryReport(void);
-/* Display the memory treap */
-void memDisplayTreap(void);
+/* Recursive function to print out the list of memory blocks */
+extern SDWORD memRecReport(MEM_NODE *psRoot);
+#endif
#ifdef DEBUG_MALLOC
@@ -65,18 +67,13 @@
#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
-
-#endif
+#endif // _mem_h_
Index: lib/framework/memint.h
===================================================================
--- lib/framework/memint.h (revision 581)
+++ lib/framework/memint.h (working copy)
@@ -26,9 +26,5 @@
/* 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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
