Re: gEDA-user: Small patch to aid use of lib dmalloc

[Stephen Ecob <silicon.on.inspiration@xxxxxxxxx>]

On Tue, Dec 7, 2010 at 12:31 AM, Peter Clifton <pcjc2@xxxxxxxxx> 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@xxxxxxxxxxxxxx
http://www.seul.org/cgi-bin/mailman/listinfo/geda-user