Debugging memory leaks

[fmassei]

Hello!
I made a short piece of code that I find very useful for debugging,
and I wanted to ask you if it is correct, somehow acceptable or if I
simply reinvented the wheel.
To deal with some bad bugs caused by memory leaks I ended up with this
simple solution: I made one header file that, when included, replaces
the malloc/calloc/realloc/free functions with some other functions
that do the actual job and insert (or remove) the pointers to the
allocated memory in a list. This way I can catch many errors as double
frees, allocations of zero size, or missing calls to free.
Obviously it works only when a given #define is set (NDEBUG, in my
case).
What do you think about it?
Thank you in advance!
(If you want to see the code I wrote a little page here:
http://technicalinsanity.org/out/simplegc/index.html)

[Ben Pfaff]

fm*****@gmail.c om writes:

I made a short piece of code that I find very useful for debugging,
and I wanted to ask you if it is correct, somehow acceptable or if I
simply reinvented the wheel.
To deal with some bad bugs caused by memory leaks I ended up with this
simple solution: I made one header file that, when included, replaces
the malloc/calloc/realloc/free functions with some other functions
that do the actual job and insert (or remove) the pointers to the
allocated memory in a list. This way I can catch many errors as double
frees, allocations of zero size, or missing calls to free.
Obviously it works only when a given #define is set (NDEBUG, in my
case).
What do you think about it?

You have reinvented the wheel. Many programmers have done
similarly to debug memory leaks. (That doesn't make it any less
useful to do so.)
--
Ben Pfaff
http://benpfaff.org

[dj3vande]

In article <b1************ *************** *******@t39g200 0prh.googlegrou ps.com>,
<fm*****@gmail. comwrote:

>Hello!
I made a short piece of code that I find very useful for debugging,
and I wanted to ask you if it is correct, somehow acceptable or if I
simply reinvented the wheel.
To deal with some bad bugs caused by memory leaks I ended up with this
simple solution: I made one header file that, when included, replaces
the malloc/calloc/realloc/free functions with some other functions
that do the actual job and insert (or remove) the pointers to the
allocated memory in a list. This way I can catch many errors as double
frees, allocations of zero size, or missing calls to free.
Obviously it works only when a given #define is set (NDEBUG, in my
case).
What do you think about it?

NDEBUG is usually #define'd to indicate that you DON'T want debugging
features turned on in the code. So if I'm understanding you correctly
and you're using it to turn on your debugging malloc family
replacement, that's probably Not Such A Good Idea.

Other than that, it sounds like you've reinvented a particularly useful
wheel; if you don't already have a wheel of that size and load-bearing-
ness, your version will probably do fine.

<OT>
The standard library malloc on my Mac supports most of the checking
features you describe, and allows them to be turned on by setting
appropriate environment variables before running the program. I
strongly suspect that this was inherited from FreeBSD, and would be
unsurprised if other BSDs have similar debugging support.

If you're using Linux/x86, you should take a look at Valgrind, which
can do all of that and more.
</OT>

Note that by replacing malloc and friends, you're breaking the contract
that the definition of the C language specifies between you and the
implementor of your compiler; it's possible that you'll break something
by doing this.
(For debugging memory problems, this is probably acceptable. The worst
that can happen is that you break things in a way that interferes with
your debugging, and even in that case reverting to the non-malloc-
debugging build will leave you back where you started, and all you lose
is the getting a little bit farther ahead that you were hoping for.)

If you haven't already implemented them (I didn't look at your code),
you might also find a few wrapper macros useful:
--------
#undef malloc
#define malloc(sz) my_malloc(sz,__ FILE__,__LINE__ )
#undef free
#define free(ptr) my_free(ptr,__F ILE__,__LINE__)
#undef realloc
#define realloc(ptr,sz) my_realloc(ptr, sz,__FILE__,__L INE__)
#undef calloc
#define calloc(num,sz) my_calloc(num,s z,__FILE__,__LI NE__)
--------
This allows your debugging versions (which will need to take filename
and line arguments, of course) to track where they were called from
and report that when they detect problems.
dave

--
Dave Vandervies dj3vande at eskimo dot com
You can save yourself a lot of work by just making up some results;
they'll be just as good as those you'd get if you actually ran the
survey. --Eric Sosman in comp.lang.c

[jameskuyper]

fmas...@gmail.c om wrote:

Hello!
I made a short piece of code that I find very useful for debugging,
and I wanted to ask you if it is correct, somehow acceptable or if I
simply reinvented the wheel.
To deal with some bad bugs caused by memory leaks I ended up with this
simple solution: I made one header file that, when included, replaces
the malloc/calloc/realloc/free functions with some other functions
that do the actual job and insert (or remove) the pointers to the
allocated memory in a list. This way I can catch many errors as double
frees, allocations of zero size, or missing calls to free.
Obviously it works only when a given #define is set (NDEBUG, in my
case).
What do you think about it?

By convention, macros such as gc_need_real should be all upper-case.
Of course, this is only a convention, but a useful one. If it is
#defined, you should keep in mind that there may already be standard
library macros defined with the same names you use. You should #undef
any such definitions before replacing them with your own. In
principle, if gc_need_real is #defined, the behavior of any program
which calls the normal malloc() familiy is undefined. In practice,
replacement of standard library functions often works, and is a
popular method of performing memory leak checks.

Keep in mind that your approach is completely useless for
investigating memory leaks that are due to direct calls to the
malloc() family from libraries that were built without using your
header files.

You are quite right to worry about whether you are "reinventin g the
wheel". You should check to see whether a debugging version of the
malloc() family is provided by your implementation. If one is
provided, it's probably safer to use than your own replacement. Also,
this same technique is used by a number of memory leak testing
packages, some of them much more sophisticated than anything you could
easily write. In both cases, the replacement library functions
actually replace the normal standard library functions, rather than
wrapping them. As a result, you can even detect memory leaks that
occur in libraries that were compiled without using your header file.

[Peter Nilsson]

dj3va...@csclub .uwaterloo.ca.i nvalid wrote:

If you haven't already implemented them (I didn't look
at your code), you might also find a few wrapper macros
useful:
--------
#undef malloc
#define malloc(sz) my_malloc(sz,__ FILE__,__LINE__ )
#undef free
#define free(ptr) my_free(ptr,__F ILE__,__LINE__)
#undef realloc
#define realloc(ptr,sz) my_realloc(ptr, sz,__FILE__,__L INE__)
#undef calloc
#define calloc(num,sz) my_calloc(num,s z,__FILE__,__LI NE__)
--------

These potentially cancel the idempotence of the <stdlib.h>
header. Better to supply (and use) genuine wrappers...

#ifndef NDEBUG
#define wrap_malloc(sz) my_malloc(sz, __FILE__, __LINE__)
#else
#define wrap_malloc(sz) malloc(sz)
#endif

--
Peter

[Peter Nilsson]

fmas...@gmail.c om wrote:

(If you want to see the code I wrote a little page
here:http://technicalinsanity.org/out/simplegc/index.html)

Some obvious things from a quick glance... some are style
issues, many are correctness issues...

Your include guards use identifiers reserved for the
implementation. Suggest you replace __SIMPLEGC_H__
with H_SIMPLEGC_H.

You might want to swap the comments...

#include <stdlib.h /* needed: printf */
#include <stdio.h /* needed: malloc/calloc/realloc/free */

Better still, delete them.

You incorrectly print size_t values with %d and pointers
with %x.

You should print debugging comments to stderr, not stdout.

You should use const char *, instead of char * when
declaring parameters that don't change the pointed to
string...

void *gc_malloc(size _t size, char *fname, size_t fline);

Also, I'd go with long, not size_t for fline. I've used
implementations where size_t's range is 0..65535, but
__LINE__ could exceed that. Of course, the same problem
exists if there are LONG_MAX lines, but I was much less
concerned about that. ;)

Note: NDEBUG means 'NO DEBUG'.

In any case, I suggest that you _not_ make your utility
functions subject to NDEBUG. Simply define the functions
you need. If they aren't used, they aren't used. Smart
linkers will remove them, but even if they don't, suppose
you're linking two modules where you want the debugging
on one, but not the other.

Two things regarding...

if ((p->fname = malloc(strlen(f name)+1))==NULL ) {

Supressing the macro with (malloc)(...) is simpler than
the gv_need_real kludge. Also, rather than copying the
string, I'd just assign it and put a condition on your
function that they must constant static duration strings,
e.g. string literals. [I doubt this will worry anyone
who uses your function.]

--
Peter

[Nate Eldredge]

Peter Nilsson <ai***@acay.com .auwrites:

Also, I'd go with long, not size_t for fline. I've used
implementations where size_t's range is 0..65535, but
__LINE__ could exceed that. Of course, the same problem
exists if there are LONG_MAX lines, but I was much less
concerned about that. ;)

Hmm. On such implementations , assuming `int' was also 16 bits, did an
occurence of __LINE__ on line 65537 expand to `65537L'? It seems like
it would be a bug if it didn't.

This is an interesting issue, because ordinarily the preprocessor
wouldn't know about things like the ranges for types. Also, people use
C preprocessors for a lot of non-C languages, and I bet they don't
expect that behavior.

[Keith Thompson]

Nate Eldredge <na**@vulcan.la nwrites:

Peter Nilsson <ai***@acay.com .auwrites:

>Also, I'd go with long, not size_t for fline. I've used
implementation s where size_t's range is 0..65535, but
__LINE__ could exceed that. Of course, the same problem
exists if there are LONG_MAX lines, but I was much less
concerned about that. ;)

Hmm. On such implementations , assuming `int' was also 16 bits, did an
occurence of __LINE__ on line 65537 expand to `65537L'? It seems like
it would be a bug if it didn't.

The L suffix isn't necessary. If int is 16 bits, then the unadorned
constant 65537 is of type long.

[...]

--
Keith Thompson (The_Other_Keit h) ks***@mib.org <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

[Nate Eldredge]

Keith Thompson <ks***@mib.orgw rites:

Nate Eldredge <na**@vulcan.la nwrites:

>Peter Nilsson <ai***@acay.com .auwrites:

>>Also, I'd go with long, not size_t for fline. I've used
implementatio ns where size_t's range is 0..65535, but
__LINE__ could exceed that. Of course, the same problem
exists if there are LONG_MAX lines, but I was much less
concerned about that. ;)

Hmm. On such implementations , assuming `int' was also 16 bits, did an
occurence of __LINE__ on line 65537 expand to `65537L'? It seems like
it would be a bug if it didn't.

The L suffix isn't necessary. If int is 16 bits, then the unadorned
constant 65537 is of type long.

Aha. 6.4.4.1 (5). Thanks. I think I've been using the L suffix
unnecessarily all along.

[fmassei]

Thank you all, you gave me a lot of inputs and good suggestions! I'm
going to modify the code asap.
Regarding the use of implementation dependent tools of course you are
right, but I'd like to have an implementation independent tool (even
if very very simple) just to be free to debug the applications in all
the target environments. That is the main reason because I prefer not
to use very advanced softwares like valgrind, even if sometimes they
can actually save your day.