Re: gEDA-user: Small patch to aid use of lib dmalloc
On Tue, 2010-12-07 at 14:59 +1100, Stephen Ecob wrote: > I propose the following solution: > > 1. replace all calls to MyCalloc() with calls to calloc() > 2. replace all calls to MyMalloc() with calls to malloc() > 3. replace all calls to MyRealloc() with calls to realloc() > 4. replace all calls to SaveFree() with calls to free() > 5. retain the MYFREE() macro as its pointer clearing side effect is required > 6. treat calls to MyStrdup() on a case by case basis: > 6a. calls known to never supply a NULL pointer: replace MyStrdup(x, > y) with strdup(x) > 6b. calls known to sometimes supply a NULL pointer: replace > MyStrdup(x, y) with: (x) ? strdup(x) : (x) I propose similar, but with the convenience macro: STRDUP(x) ((x) ? strdup(x) : (x)) Then any callers of MyStrdup can be safely swapped to STRDUP() rather then strdup(). FWIW, g_strdup from glib allows the passed string to be NULL (and will return NULL in that case). > 8. Instead of simply retaining MYFREE(p) (point 5), we could replace > each use of it with an explicit: > free(p); > p = NULL; I dropped the p = NULL; for cases where p was then immediately reassigned, or I noted that the code was discarding or memset zeroing he container of p within the same function. (Common in our destructors / cleanup code). I've got the basis for the patch which I can send. (Just not all cases of MyStrdup have been fixed). -- Peter Clifton Electrical Engineering Division, Engineering Department, University of Cambridge, 9, JJ Thomson Avenue, Cambridge CB3 0FA Tel: +44 (0)7729 980173 - (No signal in the lab!) Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me) ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
On Tue, Dec 7, 2010 at 3:06 PM, timecop wrote:
>> 5. retain the MYFREE() macro as its pointer clearing side effect is required
>> 8. Instead of simply retaining MYFREE(p) (point 5), we could replace
>> each use of it with an explicit:
>> free(p);
>> p = NULL;
>
> Is this "MY" prefix actually in the code?
Sure is :)
> Most sane places call the kind of free your'e talking about here call
> this macro SAFE_FREE() or SAFE_RELEASE() or something. it does
> something along the lines of
>
> if (arg) { free(arg); arg = null; }
Actually, in PCB the nearest equivalent to that is called "SaveFree"
___
geda-user mailing list
geda-user@moria.seul.org
http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
> Is this "MY" prefix actually in the code? It's free software, you could just look at the code yourself. Yes, the MY prefix is used in pcb, otherwise why would we be talking about it? ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
> 5. retain the MYFREE() macro as its pointer clearing side effect is required
> 8. Instead of simply retaining MYFREE(p) (point 5), we could replace
> each use of it with an explicit:
> free(p);
> p = NULL;
Is this "MY" prefix actually in the code?
Most sane places call the kind of free your'e talking about here call
this macro SAFE_FREE() or SAFE_RELEASE() or something. it does
something along the lines of
if (arg) { free(arg); arg = null; }
___
geda-user mailing list
geda-user@moria.seul.org
http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
I propose the following solution: 1. replace all calls to MyCalloc() with calls to calloc() 2. replace all calls to MyMalloc() with calls to malloc() 3. replace all calls to MyRealloc() with calls to realloc() 4. replace all calls to SaveFree() with calls to free() 5. retain the MYFREE() macro as its pointer clearing side effect is required 6. treat calls to MyStrdup() on a case by case basis: 6a. calls known to never supply a NULL pointer: replace MyStrdup(x, y) with strdup(x) 6b. calls known to sometimes supply a NULL pointer: replace MyStrdup(x, y) with: (x) ? strdup(x) : (x) 6c. calls where we aren't sure: treat the same as 6b. Notes 7. 1,2&3 mean that we discard the following side effects of the mymem.c allocators: 7a. logging messages when MEM_DEBUF defined 7b. 0 == 1 logic 7c. alloc failure handling via MyFatal() -> EmergencySave() My personal feeling is that 7a & 7b are essentially useless and that 7c has almost zero probability of ever actually saving someone from losing their PCB. 8. Instead of simply retaining MYFREE(p) (point 5), we could replace each use of it with an explicit: free(p); p = NULL; Personally I prefer two perspicuous lines to a one line opaque macro. Comments? PS If we achieve consensus I'm happy to code the patch. ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
> So calls with a NULL pointer are rare, but there are at least 3 > callers that require tolerance of it. I wouldn't call 3% rare. Uncommon, perhaps, but given who those three callers are, it's far more significant than mere numbers show. ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
I count 54 locations in head that call MyStrdup() A run time check of calls to MyStrdup() shows: create.c:197 made 0 NULL calls, 48 good calls create.c:219 made 0 NULL calls, 32 good calls create.c:238 made 0 NULL calls, 1 good calls create.c:240 made 0 NULL calls, 1 good calls create.c:286 made 127 NULL calls, 0 good calls create.c:614 made 0 NULL calls, 23 good calls create.c:809 made 364 NULL calls, 436 good calls create.c:810 made 0 NULL calls, 800 good calls create.c:896 made 40 NULL calls, 3576 good calls create.c:897 made 0 NULL calls, 3616 good calls create.c:921 made 0 NULL calls, 4212 good calls create.c:1000 made 0 NULL calls, 924 good calls create.c:1017 made 0 NULL calls, 3552 good calls file.c:321 made 0 NULL calls, 3 good calls file.c:393 made 0 NULL calls, 2 good calls file.c:431 made 0 NULL calls, 2 good calls file.c:1206 made 0 NULL calls, 53 good calls file.c:1297 made 0 NULL calls, 1 good calls main.c:960 made 0 NULL calls, 16 good calls misc.c:726 made 0 NULL calls, 20 good calls misc.c:945 made 0 NULL calls, 1 good calls So calls with a NULL pointer are rare, but there are at least 3 callers that require tolerance of it. Unfortunately my test coverage is low (based on a quick run of loading a small board and autorouting it), as my test only sees calls from 21 of the 54 possible callers. ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
On Tue, 2010-12-07 at 11:58 +1100, Stephen Ecob wrote: > On Tue, Dec 7, 2010 at 11:48 AM, Peter Clifton wrote: > > On Tue, 2010-12-07 at 10:04 +1100, Stephen Ecob wrote: > > > >> Dropping the (a) ? (a) : 1 foolishness would be cleaner, but could > >> expose latent bugs in the 71 callers of the mymem allocators. > >> I'm happy to proceed either way. What is your preference ? > > > > Let me push a big patch nuking the My* allocation functions, that would > > make me feel much happier. > > Great! It'll be good to lay those functions to rest :) Aside from the error checking we know about, there is also the symantic that MyStrdup (NULL) returns NULL, rather than seg-faulting. Also, in a number of cases, replacing MYFREE() requires you to explicitly NULL the free'd variable. I'm still all for making these changes.. just that it will require a bit more thought than I've got time for tonight. The scope is approximately: 30 files changed, 251 insertions(+), 432 deletions(-) But I've got a segfault on start due to the naive s/MyStrdup/strdup/ changes that involves. (I've adjusted the MYFREE invocations as appropriate). -- Peter Clifton Electrical Engineering Division, Engineering Department, University of Cambridge, 9, JJ Thomson Avenue, Cambridge CB3 0FA Tel: +44 (0)7729 980173 - (No signal in the lab!) Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me) ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
On 07/12/10 11:55, Peter Clifton wrote: On Tue, 2010-12-07 at 00:48 +, Peter Clifton wrote: On Tue, 2010-12-07 at 10:04 +1100, Stephen Ecob wrote: Dropping the (a) ? (a) : 1 foolishness would be cleaner, but could expose latent bugs in the 71 callers of the mymem allocators. I'm happy to proceed either way. What is your preference ? Let me push a big patch nuking the My* allocation functions, that would make me feel much happier. Damnit, MyMalloc does some other "helpful" things, like checking for a memory allocation failure and aborting with a useful diagnostic message. I'd be pretty happy to drop the messages (as it is not such a common occurrence), but I do note that the changes I'm about to propose will change a known handled failure path with a nice error message and termination, to retuning a NULL pointer which is then probably used, "probably" followed by segfault. On Linux at least, "malloc" will tend to always succeed rather than returning out of memory. You could wrap malloc so that it does a longjmp to the start of the program where the error is displayed then exits. Gracefully handling malloc failures is too tedious and itself bug-prone, so just design the program so that they should never happen, given an adequte amount of memory. ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
On Tue, Dec 7, 2010 at 11:48 AM, Peter Clifton wrote: > On Tue, 2010-12-07 at 10:04 +1100, Stephen Ecob wrote: > >> Dropping the (a) ? (a) : 1 foolishness would be cleaner, but could >> expose latent bugs in the 71 callers of the mymem allocators. >> I'm happy to proceed either way. What is your preference ? > > Let me push a big patch nuking the My* allocation functions, that would > make me feel much happier. Great! It'll be good to lay those functions to rest :) ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
On Tue, 2010-12-07 at 00:48 +, Peter Clifton wrote: > On Tue, 2010-12-07 at 10:04 +1100, Stephen Ecob wrote: > > > Dropping the (a) ? (a) : 1 foolishness would be cleaner, but could > > expose latent bugs in the 71 callers of the mymem allocators. > > I'm happy to proceed either way. What is your preference ? > > Let me push a big patch nuking the My* allocation functions, that would > make me feel much happier. Damnit, MyMalloc does some other "helpful" things, like checking for a memory allocation failure and aborting with a useful diagnostic message. I'd be pretty happy to drop the messages (as it is not such a common occurrence), but I do note that the changes I'm about to propose will change a known handled failure path with a nice error message and termination, to retuning a NULL pointer which is then probably used, "probably" followed by segfault. On Linux at least, "malloc" will tend to always succeed rather than returning out of memory. -- Peter Clifton Electrical Engineering Division, Engineering Department, University of Cambridge, 9, JJ Thomson Avenue, Cambridge CB3 0FA Tel: +44 (0)7729 980173 - (No signal in the lab!) Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me) ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
On Tue, 2010-12-07 at 10:04 +1100, Stephen Ecob wrote: > Dropping the (a) ? (a) : 1 foolishness would be cleaner, but could > expose latent bugs in the 71 callers of the mymem allocators. > I'm happy to proceed either way. What is your preference ? Let me push a big patch nuking the My* allocation functions, that would make me feel much happier. There is of course the risk that it exposes a latent bug, but I'm prepared to take that on my head. The current BSD man pages suggest realloc and free should work with a NULL pointer, and I'm pretty sure we have cases where we might call free (NULL); directly elsewhere. -- Peter Clifton Electrical Engineering Division, Engineering Department, University of Cambridge, 9, JJ Thomson Avenue, Cambridge CB3 0FA Tel: +44 (0)7729 980173 - (No signal in the lab!) Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me) ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
On Tue, Dec 7, 2010 at 12:31 AM, Peter Clifton wrote: > On Mon, 2010-12-06 at 16:58 +1100, Stephen Ecob wrote: >> If you use (or intend to use) lib dmalloc with PCB you will find the >> following useful. >> >> I've uploaded a patch against current heaad to sourceforge, ID >> #3129279, described as follows: > > Looks useful, I will push it, however, please explain the following first: > > +# define MyCalloc(a, b, c) calloc((a) ? (a) : 1, (b) ? (b) : 1) > +# define MyMalloc(a, b) malloc((a) ? (a) : 1) > +# define MyRealloc(a, b, c) ((a) ? realloc(a, (b) ? (b) : 1) : malloc((b) ? > (b) : 1)) > > I don't see why we are adding a synonym with requesting zero bytes / > zero items, with requesting one item? I preserved the semantics of the original functions, in case some part of the code relies on them. > Is there anywhere (BROKEN) in PCB which requests zero items / bytes, and > expects that call to work? Agreed, the semantics of the mymem allocation functions are foolish. A quick check of the parts of PCB that I'm currently using show no dependance on the 0==1 foolishness, but I haven't had time to test all 71 callers of these functions. > I'd rather commit: > > +# define MyCalloc(a, b, c) calloc((a), (b)) > +# define MyMalloc(a, b) malloc((a)) > +# define MyRealloc(a, b, c) realloc((a), (b)) > > NB: realloc will "do the right thing" if the previous pointer passed is > NULL, no need to check and swap for a malloc call. Yes, but note the (probably ancient) comment in mymem.c: * this is a save version because BSD doesn't support the * handling of NULL pointers in realloc() I don't know if that is still true for BSD, but we should preserve the workaround unless we are confident that all of PCB's supported platforms work correctly with realloc(NULL, ...) > One final nit, I'd prefer not to see the double negative in the header > files: > > #ifndef HAVE_LIBDMALLOC > NON D-Malloc stuff > #else > D-Malloc stuff > #endif > > > Could more readably be defined as: > > #ifdef HAVE_LIBDMALLOC > D-Malloc stuff > #else > NON D-Malloc stuff > #endif Sure, happy to fix that. > If you feel that your version is better as it matches the various other > #ifndef HAVE_LIBDMALLOC you added, then fair enough, I'll push as is > pending an answer on the #define questions. > > > FWIW, I'd love to see our own custom memory allocation routines die in a > fire. Lets plan to eventually replace them with malloc / realloc / free > calls, and this won't be an issue. I thoroughly agree :) > I did loose some respect for the dmalloc author(s) when I noticed on > their page they can't spell "Microsoft Windows" correctly. "Windoze" > > In general, valgrind (probably not even conceived of when dmalloc was > first written), is a much more useful tool for memory leak diagnosis, so > I wouldn't go _too_ far out of our way to make things dmalloc suitable. Sure. > You still won't see good back-traces for the r_tree allocations, for > example. (Given your usage and symptoms, I guess some r_trees are being > leaked in the autorouter code?) Just found the leak this morning, turns out it's yet another thing to dislike in mymem.c ;-) So regarding the patch: Dropping the (a) ? (a) : 1 foolishness would be cleaner, but could expose latent bugs in the 71 callers of the mymem allocators. I'm happy to proceed either way. What is your preference ? ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
On Monday 06 Dec 2010 13:31:09 Peter Clifton wrote: > I did loose some respect for the dmalloc author(s) when I noticed on > their page they can't spell "Microsoft Windows" correctly. "Windoze" > > In general, valgrind (probably not even conceived of when dmalloc was > first written), is a much more useful tool for memory leak diagnosis, so > I wouldn't go _too_ far out of our way to make things dmalloc suitable. Seconding this -- valgrind / memcheck is absolutely excellent, and generally gives much better results than dmalloc in my experience. For example, I recently had to debug a program where I requested memory directly from the operating system using mmap(), and Valgrind's support for tracking use of uninitialised memory turned out to be invaluable. Callgrind is also very helpful. :-) Cheers, Peter -- Peter Brett Remote Sensing Research Group Surrey Space Centre ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
Re: gEDA-user: Small patch to aid use of lib dmalloc
On Mon, 2010-12-06 at 16:58 +1100, Stephen Ecob wrote: > If you use (or intend to use) lib dmalloc with PCB you will find the > following useful. > > I've uploaded a patch against current heaad to sourceforge, ID > #3129279, described as follows: Looks useful, I will push it, however, please explain the following first: +# define MyCalloc(a, b, c) calloc((a) ? (a) : 1, (b) ? (b) : 1) +# define MyMalloc(a, b) malloc((a) ? (a) : 1) +# define MyRealloc(a, b, c) ((a) ? realloc(a, (b) ? (b) : 1) : malloc((b) ? (b) : 1)) I don't see why we are adding a synonym with requesting zero bytes / zero items, with requesting one item? Is there anywhere (BROKEN) in PCB which requests zero items / bytes, and expects that call to work? I'd rather commit: +# define MyCalloc(a, b, c) calloc((a), (b)) +# define MyMalloc(a, b) malloc((a)) +# define MyRealloc(a, b, c) realloc((a), (b)) NB: realloc will "do the right thing" if the previous pointer passed is NULL, no need to check and swap for a malloc call. One final nit, I'd prefer not to see the double negative in the header files: #ifndef HAVE_LIBDMALLOC NON D-Malloc stuff #else D-Malloc stuff #endif Could more readably be defined as: #ifdef HAVE_LIBDMALLOC D-Malloc stuff #else NON D-Malloc stuff #endif If you feel that your version is better as it matches the various other #ifndef HAVE_LIBDMALLOC you added, then fair enough, I'll push as is pending an answer on the #define questions. FWIW, I'd love to see our own custom memory allocation routines die in a fire. Lets plan to eventually replace them with malloc / realloc / free calls, and this won't be an issue. I did loose some respect for the dmalloc author(s) when I noticed on their page they can't spell "Microsoft Windows" correctly. "Windoze" In general, valgrind (probably not even conceived of when dmalloc was first written), is a much more useful tool for memory leak diagnosis, so I wouldn't go _too_ far out of our way to make things dmalloc suitable. You still won't see good back-traces for the r_tree allocations, for example. (Given your usage and symptoms, I guess some r_trees are being leaked in the autorouter code?) -- Peter Clifton Electrical Engineering Division, Engineering Department, University of Cambridge, 9, JJ Thomson Avenue, Cambridge CB3 0FA Tel: +44 (0)7729 980173 - (No signal in the lab!) Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me) ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
gEDA-user: Small patch to aid use of lib dmalloc
If you use (or intend to use) lib dmalloc with PCB you will find the following useful. I've uploaded a patch against current heaad to sourceforge, ID #3129279, described as follows: PCB supports the use of the dmalloc library as a configuration option. This patch makes the PCB source code more "friendly" to libdmalloc in the following ways: * Some source files that allocate memory from the heap but fail to include now do so * The mymem.c low level memory routines are replaced with corresponding libdmalloc calls (if libdmalloc is available). This is helpful as these routines otherwise obscure the __FILE__ and __LINE__ information of lines that use the heap. ___ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
