From: Philipp K. K. <pk...@sp...> - 2012-12-28 19:21:48
|
While working on the stm8 port, I noticed that the other ports * Allcoate memory in newAsmop using Safe_calloc() or traceAlloc(). * Do not free this memory in freeAsmop(). This looks like a serious memory leak to me, but since all ports do it it probably isn't. What am I missing here? Philipp |
From: Maarten B. <sou...@ds...> - 2012-12-29 00:14:04
|
Hi Philipp, I'm not sure and I haven't checked either, but I guess that you get dangling pointers if they are freed. Maybe this only was true at some time in the past, maybe it still is. I guess if you look at the freeAsmop history you'll find a version that had free removed. Perhaps the memory can be freed now, it's worth a try. Maarten > While working on the stm8 port, I noticed that the other ports > > * Allcoate memory in newAsmop using Safe_calloc() or traceAlloc(). > * Do not free this memory in freeAsmop(). > > This looks like a serious memory leak to me, but since all ports do it > it probably isn't. What am I missing here? > > Philipp |
From: Raphael N. <rn...@we...> - 2012-12-29 06:37:36
|
Hi, > I'm not sure and I haven't checked either, but I guess that you get > dangling pointers if they are freed. Maybe this only was true at some time > in the past, maybe it still is. I guess if you look at the freeAsmop > history you'll find a version that had free removed. Perhaps the memory > can be freed now, it's worth a try. We had a discussion on memory leaks discovered by Valgrind several months ago. Basically, sdcc is a short running program, which is supposed to handle rather small input files and convert them into equally rather short output. We thus simply do not care to release all allocated memory for two reasons: * performance (releasing thousands of objects can and does take considerable time; letting the OS simply cleanup the whole virtual memory associated with the program at its exit is much faster) * safety (as Maarten pointed out, dangling pointers could result in premature program termination; I am aware of the fact that this also covers programming errors, but still the current approach seems to work pretty well). I would recommend against changing such a crucial point just for the sake of code cleanliness. One might add a comment to indicate where free() should be placed and why it is not. Raphael >> While working on the stm8 port, I noticed that the other ports >> >> * Allcoate memory in newAsmop using Safe_calloc() or traceAlloc(). >> * Do not free this memory in freeAsmop(). >> >> This looks like a serious memory leak to me, but since all ports do it >> it probably isn't. What am I missing here? >> >> Philipp |
From: Philipp K. K. <pk...@sp...> - 2012-12-29 10:04:03
|
On 29.12.2012 07:37, Raphael Neider wrote: > Hi, > >> I'm not sure and I haven't checked either, but I guess that you get >> dangling pointers if they are freed. Maybe this only was true at some time >> in the past, maybe it still is. I guess if you look at the freeAsmop >> history you'll find a version that had free removed. Perhaps the memory >> can be freed now, it's worth a try. > > We had a discussion on memory leaks discovered by Valgrind several > months ago. Basically, sdcc is a short running program, which is > supposed to handle rather small input files and convert them into > equally rather short output. We thus simply do not care to release all > allocated memory for two reasons: > > * performance (releasing thousands of objects can and does take > considerable time; letting the OS simply cleanup the whole virtual > memory associated with the program at its exit is much faster) > * safety (as Maarten pointed out, dangling pointers could result in > premature program termination; I am aware of the fact that this also > covers programming errors, but still the current approach seems to > work pretty well). For safety, we could do reference counting for the asmops. I'll have to look into this more closely, but with the new register allocator, we can easily call aopOp a few thousand times per iCode by default (and with the --max-allocs-per-node 1000000 I use when optimization matters, it can easily be a few million times). So a memory leak here could matter. Philipp |
From: Erik P. <epe...@iv...> - 2012-12-29 08:34:25
|
On Fri, 28 Dec 2012, Philipp Klaus Krause wrote: > While working on the stm8 port, I noticed that the other ports > > * Allcoate memory in newAsmop using Safe_calloc() or traceAlloc(). > * Do not free this memory in freeAsmop(). > > This looks like a serious memory leak to me, but since all ports do it > it probably isn't. What am I missing here? > > Philipp At some point in the distant past (that is, before I started my involvement with the project nearly 10 years ago) SDCC was using some version of the Boehm garbage collector. In that case, freeing unused memory should happen automatically when new memory allocations are attempted. However, this was eventually abandoned (perhaps due to bugs in the GC implementation; I do not know the reason) and the wrapper functions redirected back to the standard C library functions. So yes, there are a substantial number of leaks. I think it's just been a very low priority since the amount of memory installed on the typical computer now dwarfs SDCC's memory footprint even with the leaks. Erik |