threads / patch / 14128
Subject: [PATCH 2/2] git-rev-parse: use leaky().
Pierre Habouzit[RFC] leaky()
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().
Pierre Habouzit[PATCH 1/2] Introduce leaky().
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(-)
Makefile, alloc.c, cache.h
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
Pierre HabouzitMark 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
Pierre HabouzitRe: [PATCH 1/2] Introduce leaky().
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
Jakub NarebskiRe: [PATCH 1/2] Introduce leaky().
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
Pierre HabouzitRe: [PATCH 1/2] Introduce leaky().
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
Junio C HamanoRe: [PATCH 1/2] Introduce leaky().
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.
Pierre HabouzitRe: [PATCH 1/2] Introduce leaky().
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
Re: [PATCH 1/2] Introduce leaky().
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>