[RFC] leaky()

[RFC] leaky()

[Pierre Habouzit]

In the parse-options thread, it appears that we may somethings leak some
tiny bits of memory on purpose. So that it makes life of people
searching for real memory leaks easier.

The second patch marks the leaks I'm responsible for in git (and that
I'm aware of) as leaky().

[PATCH 1/2] Introduce leaky().

[Pierre Habouzit]

This can be used to mark allocated memory as a "leak". This can be used to
collect memory at the exit of the command, so that tools like valgrind can
be used to check for actual memory leak without noise.

COLLECT_LEAKS_AT_EXIT must be set to that purpose, else 'leaky' is the
transparent macro.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 Makefile |    5 +++++
 alloc.c  |   20 ++++++++++++++++++++
 cache.h  |    5 +++++
 3 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 9c16482..6be15c8 100644
--- a/Makefile
+++ b/Makefile
@@ -151,6 +151,8 @@ all::
 # Define NO_EXTERNAL_GREP if you don't want "git grep" to ever call
 # your external grep (e.g., if your system lacks grep, if its grep is
 # broken, or spawning external process is slower than built-in grep git has).
+#
+# Define COLLECT_LEAKS_AT_EXIT if you want memory marked as leaky() at exit.
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -947,6 +949,9 @@ endif
 ifdef NO_EXTERNAL_GREP
 	BASIC_CFLAGS += -DNO_EXTERNAL_GREP
 endif
+ifdef COLLECT_LEAKS_AT_EXIT
+	BASIC_CFLAGS += -DCOLLECT_LEAKS_AT_EXIT
+endif
 
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK=NoThanks
diff --git a/alloc.c b/alloc.c
index 216c23a..1afe810 100644
--- a/alloc.c
+++ b/alloc.c
@@ -74,3 +74,23 @@ void alloc_report(void)
 	REPORT(commit);
 	REPORT(tag);
 }
+
+#ifdef COLLECT_LEAKS_AT_EXIT
+static void **leaks;
+int leaknb, leaksz;
+
+static void release_leaks(void)
+{
+	while (leaknb-- > 0)
+		free(*leaks++);
+	free(leaks);
+}
+
+void *leaky(void *ptr)
+{
+	if (leaksz == 0)
+		atexit(&release_leaks);
+	ALLOC_GROW(leaks, leaknb + 1, leaksz);
+	return leaks[leaknb++] = ptr;
+}
+#endif
diff --git a/cache.h b/cache.h
index 101ead5..33603bb 100644
--- a/cache.h
+++ b/cache.h
@@ -777,6 +777,11 @@ int decode_85(char *dst, const char *line, int linelen);
 void encode_85(char *buf, const unsigned char *data, int bytes);
 
 /* alloc.c */
+#ifdef COLLECT_LEAKS_AT_EXIT
+extern void *leaky(void *);
+#else
+# define leaky(x) x
+#endif
 extern void *alloc_blob_node(void);
 extern void *alloc_tree_node(void);
 extern void *alloc_commit_node(void);
-- 
1.5.6.120.g3adb8.dirty

[PATCH 2/2] git-rev-parse: use leaky().

[Pierre Habouzit]

Mark the mallocs that cmd_parseopt never frees as leaky so that nobody
loses time trying to fix them.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 builtin-rev-parse.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index a7860ed..02defbb 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -319,12 +319,12 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		s = strchr(sb.buf, ' ');
 		if (!s || *sb.buf == ' ') {
 			o->type = OPTION_GROUP;
-			o->help = xstrdup(skipspaces(sb.buf));
+			o->help = leaky(xstrdup(skipspaces(sb.buf)));
 			continue;
 		}
 
 		o->type = OPTION_CALLBACK;
-		o->help = xstrdup(skipspaces(s));
+		o->help = leaky(xstrdup(skipspaces(s)));
 		o->value = &parsed;
 		o->flags = PARSE_OPT_NOARG;
 		o->callback = &parseopt_dump;
@@ -349,10 +349,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		if (s - sb.buf == 1) /* short option only */
 			o->short_name = *sb.buf;
 		else if (sb.buf[1] != ',') /* long option only */
-			o->long_name = xmemdupz(sb.buf, s - sb.buf);
+			o->long_name = leaky(xmemdupz(sb.buf, s - sb.buf));
 		else {
 			o->short_name = *sb.buf;
-			o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);
+			o->long_name = leaky(xmemdupz(sb.buf + 2, s - sb.buf - 2));
 		}
 	}
 	strbuf_release(&sb);
-- 
1.5.6.120.g3adb8.dirty

Re: [PATCH 1/2] Introduce leaky().

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]

On Tue, Jun 24, 2008 at 08:14:33PM +0000, Pierre Habouzit wrote:
> +static void release_leaks(void)
> +{
> +	while (leaknb-- > 0)
> +		free(*leaks++);
crap I sent the wrong patch... this is supposed to be:
+		free(leaks[leaknb]);
> +	free(leaks);
> +}

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] Introduce leaky().

[Jakub Narebski]

Pierre Habouzit <madcoder@debian.org> writes:

> diff --git a/Makefile b/Makefile

> +#
> +# Define COLLECT_LEAKS_AT_EXIT if you want memory marked as leaky() at exit.

I think s/at exit/to be freed &/;


> diff --git a/cache.h b/cache.h

Hmmm... cache?

>  /* alloc.c */
> +#ifdef COLLECT_LEAKS_AT_EXIT
> +extern void *leaky(void *);
> +#else
> +# define leaky(x) x
> +#endif

Not 

+# define leaky(x) (x)

to be careful?
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 1/2] Introduce leaky().

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

On Tue, Jun 24, 2008 at 09:28:30PM +0000, Jakub Narebski wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > diff --git a/Makefile b/Makefile
> 
> > +#
> > +# Define COLLECT_LEAKS_AT_EXIT if you want memory marked as leaky() at exit.
> 
> I think s/at exit/to be freed &/;

  err obviously.

> > diff --git a/cache.h b/cache.h
> 
> Hmmm... cache?

  well cache.h has the prototypes for alloc.c that feels like the proper
place, but I don't care much :)

> >  /* alloc.c */
> > +#ifdef COLLECT_LEAKS_AT_EXIT
> > +extern void *leaky(void *);
> > +#else
> > +# define leaky(x) x
> > +#endif
> 
> Not 
> 
> +# define leaky(x) (x)
> 
> to be careful?

ack.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] Introduce leaky().

[Junio C Hamano]

I looked at the patch (and your updated parseopt series that depends on
this that is queued at madism.org) again, but I have to say I do not like
it, not because I do not want leaks, nor because I do not want explicit
markings.  The issue of helping valgrind sessions is worth addressing.

However.

First of all, it is unclear what the semantics of leaky() is.  Do you want
to mark all things that will be unreacheable from anywhere in the program
at the end, or do you want to mark all things that we have obtained from
malloc(3) and friends but have not free(3)ed?  Without a clear semantics
to use as the guiding principle, it would be hard for people to use this
macro properly to help debugging leaks.

Specifically, how would you envision to handle allocations of objects that
will be reachable from object.c::obj_hash at the program end?  We do not
have to (and we should not) mark them using the former semantics, but we
should with the latter semantics.

The user would also need to worry about not freeing the original
allocation pointer when something is realloc(3)ed, which means either
finding the last realloc(3) of the object (that is logically the same, but
just being extended) and mark the pointer as leaky() after that realloc,
or unregister the original pointer from leaks before calling realloc and
register what comes back.  It will easily get messy.

By the way, the series queued in your repository still has "s/pring/print/"
typo in 4/7 and "argv not NULL terminated" comment in 6/7.

Re: [PATCH 1/2] Introduce leaky().

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On Thu, Jun 26, 2008 at 06:46:43PM +0000, Junio C Hamano wrote:
> The user would also need to worry about not freeing the original
> allocation pointer when something is realloc(3)ed, which means either
> finding the last realloc(3) of the object (that is logically the same, but
> just being extended) and mark the pointer as leaky() after that realloc,
> or unregister the original pointer from leaks before calling realloc and
> register what comes back.  It will easily get messy.

  Hmm indeed, maybe it isn't such a good idea then.

> By the way, the series queued in your repository still has "s/pring/print/"
> typo in 4/7 and "argv not NULL terminated" comment in 6/7.

  I'll fix that and pushed again, without the leaky() series dependency
(I've put in a comment that I'm aware that it's a leak), and with the
two fixes you mention done.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] Introduce leaky().

[Jan Hudec]

On Thu, Jun 26, 2008 at 23:33:04 +0200, Pierre Habouzit wrote:
> On Thu, Jun 26, 2008 at 06:46:43PM +0000, Junio C Hamano wrote:
> > The user would also need to worry about not freeing the original
> > allocation pointer when something is realloc(3)ed, which means either
> > finding the last realloc(3) of the object (that is logically the same, but
> > just being extended) and mark the pointer as leaky() after that realloc,
> > or unregister the original pointer from leaks before calling realloc and
> > register what comes back.  It will easily get messy.
> 
>   Hmm indeed, maybe it isn't such a good idea then.

I don't think it's necessary either. Valgrind (unlike other leak detectors
the poor windows folks are stuck with) is quite smart and will only report
memory as leaked when there is no pointer to it. So it should not report any
memory pointed to by index or any other static structure.

Additionally since valgrind allows one to write rules to exclude
uninteresting reports. So a good approach would be to just keep such
exclusion file around in git with the known uninteresting cases.

> > By the way, the series queued in your repository still has "s/pring/print/"
> > typo in 4/7 and "argv not NULL terminated" comment in 6/7.
> 
>   I'll fix that and pushed again, without the leaky() series dependency
> (I've put in a comment that I'm aware that it's a leak), and with the
> two fixes you mention done.
> 
> -- 
> ·O·  Pierre Habouzit
> ··O                                                madcoder@debian.org
> OOO                                                http://www.madism.org


-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>