{"thread":{"id":"14128","subject":"[PATCH 2/2] git-rev-parse: use leaky().","startedAt":"2008-06-24T20:14:32Z","lastAt":"2008-07-22T18:09:44Z","messageCount":9,"participants":["Pierre Habouzit","Jakub Narebski","Junio C Hamano","Jan Hudec"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"80977","messageId":"1214338474-16822-1-git-send-email-madcoder@debian.org","threadId":"14128","inReplyTo":null,"subject":"[RFC] leaky()","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T20:14:32Z","receivedAt":"2008-06-24T20:14:32Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"In the parse-options thread, it appears that we may somethings leak some\ntiny bits of memory on purpose. So that it makes life of people\nsearching for real memory leaks easier.\n\nThe second patch marks the leaks I'm responsible for in git (and that\nI'm aware of) as leaky().\n"},{"id":"80976","messageId":"1214338474-16822-2-git-send-email-madcoder@debian.org","threadId":"14128","inReplyTo":"1214338474-16822-1-git-send-email-madcoder@debian.org","subject":"[PATCH 1/2] Introduce leaky().","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T20:14:33Z","receivedAt":"2008-06-24T20:14:33Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"This can be used to mark allocated memory as a \"leak\". This can be used to\ncollect memory at the exit of the command, so that tools like valgrind can\nbe used to check for actual memory leak without noise.\n\nCOLLECT_LEAKS_AT_EXIT must be set to that purpose, else 'leaky' is the\ntransparent macro.\n\nSigned-off-by: Pierre Habouzit \n---\n Makefile | 5 +++++\n alloc.c | 20 ++++++++++++++++++++\n cache.h | 5 +++++\n 3 files changed, 30 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9c16482..6be15c8 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -151,6 +151,8 @@ all::\n # Define NO_EXTERNAL_GREP if you don't want \"git grep\" to ever call\n # your external grep (e.g., if your system lacks grep, if its grep is\n # broken, or spawning external process is slower than built-in grep git has).\n+#\n+# Define COLLECT_LEAKS_AT_EXIT if you want memory marked as leaky() at exit.\n \n GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -947,6 +949,9 @@ endif\n ifdef NO_EXTERNAL_GREP\n \tBASIC_CFLAGS += -DNO_EXTERNAL_GREP\n endif\n+ifdef COLLECT_LEAKS_AT_EXIT\n+\tBASIC_CFLAGS += -DCOLLECT_LEAKS_AT_EXIT\n+endif\n \n ifeq ($(TCLTK_PATH),)\n NO_TCLTK=NoThanks\ndiff --git a/alloc.c b/alloc.c\nindex 216c23a..1afe810 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -74,3 +74,23 @@ void alloc_report(void)\n \tREPORT(commit);\n \tREPORT(tag);\n }\n+\n+#ifdef COLLECT_LEAKS_AT_EXIT\n+static void **leaks;\n+int leaknb, leaksz;\n+\n+static void release_leaks(void)\n+{\n+\twhile (leaknb-- > 0)\n+\t\tfree(*leaks++);\n+\tfree(leaks);\n+}\n+\n+void *leaky(void *ptr)\n+{\n+\tif (leaksz == 0)\n+\t\tatexit(&release_leaks);\n+\tALLOC_GROW(leaks, leaknb + 1, leaksz);\n+\treturn leaks[leaknb++] = ptr;\n+}\n+#endif\ndiff --git a/cache.h b/cache.h\nindex 101ead5..33603bb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -777,6 +777,11 @@ int decode_85(char *dst, const char *line, int linelen);\n void encode_85(char *buf, const unsigned char *data, int bytes);\n \n /* alloc.c */\n+#ifdef COLLECT_LEAKS_AT_EXIT\n+extern void *leaky(void *);\n+#else\n+# define leaky(x) x\n+#endif\n extern void *alloc_blob_node(void);\n extern void *alloc_tree_node(void);\n extern void *alloc_commit_node(void);\n-- \n1.5.6.120.g3adb8.dirty\n"},{"id":"80975","messageId":"1214338474-16822-3-git-send-email-madcoder@debian.org","threadId":"14128","inReplyTo":"1214338474-16822-2-git-send-email-madcoder@debian.org","subject":"[PATCH 2/2] git-rev-parse: use leaky().","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T20:14:34Z","receivedAt":"2008-06-24T20:14:34Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Mark the mallocs that cmd_parseopt never frees as leaky so that nobody\nloses time trying to fix them.\n\nSigned-off-by: Pierre Habouzit \n---\n builtin-rev-parse.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-rev-parse.c b/builtin-rev-parse.c\nindex a7860ed..02defbb 100644\n--- a/builtin-rev-parse.c\n+++ b/builtin-rev-parse.c\n@@ -319,12 +319,12 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\ts = strchr(sb.buf, ' ');\n \t\tif (!s || *sb.buf == ' ') {\n \t\t\to->type = OPTION_GROUP;\n-\t\t\to->help = xstrdup(skipspaces(sb.buf));\n+\t\t\to->help = leaky(xstrdup(skipspaces(sb.buf)));\n \t\t\tcontinue;\n \t\t}\n \n \t\to->type = OPTION_CALLBACK;\n-\t\to->help = xstrdup(skipspaces(s));\n+\t\to->help = leaky(xstrdup(skipspaces(s)));\n \t\to->value = &parsed;\n \t\to->flags = PARSE_OPT_NOARG;\n \t\to->callback = &parseopt_dump;\n@@ -349,10 +349,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\tif (s - sb.buf == 1) /* short option only */\n \t\t\to->short_name = *sb.buf;\n \t\telse if (sb.buf[1] != ',') /* long option only */\n-\t\t\to->long_name = xmemdupz(sb.buf, s - sb.buf);\n+\t\t\to->long_name = leaky(xmemdupz(sb.buf, s - sb.buf));\n \t\telse {\n \t\t\to->short_name = *sb.buf;\n-\t\t\to->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n+\t\t\to->long_name = leaky(xmemdupz(sb.buf + 2, s - sb.buf - 2));\n \t\t}\n \t}\n \tstrbuf_release(&sb);\n-- \n1.5.6.120.g3adb8.dirty\n"},{"id":"80978","messageId":"20080624201648.GE9189@artemis.madism.org","threadId":"14128","inReplyTo":"1214338474-16822-2-git-send-email-madcoder@debian.org","subject":"Re: [PATCH 1/2] Introduce leaky().","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T20:16:49Z","receivedAt":"2008-06-24T20:16:49Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Jun 24, 2008 at 08:14:33PM +0000, Pierre Habouzit wrote:\n> +static void release_leaks(void)\n> +{\n> +\twhile (leaknb-- > 0)\n> +\t\tfree(*leaks++);\ncrap I sent the wrong patch... this is supposed to be:\n+\t\tfree(leaks[leaknb]);\n> +\tfree(leaks);\n> +}\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80991","messageId":"m3skv2jzey.fsf@localhost.localdomain","threadId":"14128","inReplyTo":"1214338474-16822-2-git-send-email-madcoder@debian.org","subject":"Re: [PATCH 1/2] Introduce leaky().","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-24T21:28:30Z","receivedAt":"2008-06-24T21:28:30Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Pierre Habouzit writes:\n\n> diff --git a/Makefile b/Makefile\n\n> +#\n> +# Define COLLECT_LEAKS_AT_EXIT if you want memory marked as leaky() at exit.\n\nI think s/at exit/to be freed &/;\n\n\n> diff --git a/cache.h b/cache.h\n\nHmmm... cache?\n\n> /* alloc.c */\n> +#ifdef COLLECT_LEAKS_AT_EXIT\n> +extern void *leaky(void *);\n> +#else\n> +# define leaky(x) x\n> +#endif\n\nNot \n\n+# define leaky(x) (x)\n\nto be careful?\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"80995","messageId":"20080624221048.GI9189@artemis.madism.org","threadId":"14128","inReplyTo":"m3skv2jzey.fsf@localhost.localdomain","subject":"Re: [PATCH 1/2] Introduce leaky().","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-24T22:10:48Z","receivedAt":"2008-06-24T22:10:48Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Jun 24, 2008 at 09:28:30PM +0000, Jakub Narebski wrote:\n> Pierre Habouzit writes:\n> \n> > diff --git a/Makefile b/Makefile\n> \n> > +#\n> > +# Define COLLECT_LEAKS_AT_EXIT if you want memory marked as leaky() at exit.\n> \n> I think s/at exit/to be freed &/;\n\n err obviously.\n\n> > diff --git a/cache.h b/cache.h\n> \n> Hmmm... cache?\n\n well cache.h has the prototypes for alloc.c that feels like the proper\nplace, but I don't care much :)\n\n> > /* alloc.c */\n> > +#ifdef COLLECT_LEAKS_AT_EXIT\n> > +extern void *leaky(void *);\n> > +#else\n> > +# define leaky(x) x\n> > +#endif\n> \n> Not \n> \n> +# define leaky(x) (x)\n> \n> to be careful?\n\nack.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"81313","messageId":"7vbq1oqbjg.fsf@gitster.siamese.dyndns.org","threadId":"14128","inReplyTo":"1214338474-16822-2-git-send-email-madcoder@debian.org","subject":"Re: [PATCH 1/2] Introduce leaky().","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-26T18:46:43Z","receivedAt":"2008-06-26T18:46:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I looked at the patch (and your updated parseopt series that depends on\nthis that is queued at madism.org) again, but I have to say I do not like\nit, not because I do not want leaks, nor because I do not want explicit\nmarkings. The issue of helping valgrind sessions is worth addressing.\n\nHowever.\n\nFirst of all, it is unclear what the semantics of leaky() is. Do you want\nto mark all things that will be unreacheable from anywhere in the program\nat the end, or do you want to mark all things that we have obtained from\nmalloc(3) and friends but have not free(3)ed? Without a clear semantics\nto use as the guiding principle, it would be hard for people to use this\nmacro properly to help debugging leaks.\n\nSpecifically, how would you envision to handle allocations of objects that\nwill be reachable from object.c::obj_hash at the program end? We do not\nhave to (and we should not) mark them using the former semantics, but we\nshould with the latter semantics.\n\nThe user would also need to worry about not freeing the original\nallocation pointer when something is realloc(3)ed, which means either\nfinding the last realloc(3) of the object (that is logically the same, but\njust being extended) and mark the pointer as leaky() after that realloc,\nor unregister the original pointer from leaks before calling realloc and\nregister what comes back. It will easily get messy.\n\nBy the way, the series queued in your repository still has \"s/pring/print/\"\ntypo in 4/7 and \"argv not NULL terminated\" comment in 6/7.\n"},{"id":"81327","messageId":"20080626213304.GA22111@artemis.madism.org","threadId":"14128","inReplyTo":"7vbq1oqbjg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Introduce leaky().","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-26T21:33:04Z","receivedAt":"2008-06-26T21:33:04Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Jun 26, 2008 at 06:46:43PM +0000, Junio C Hamano wrote:\n> The user would also need to worry about not freeing the original\n> allocation pointer when something is realloc(3)ed, which means either\n> finding the last realloc(3) of the object (that is logically the same, but\n> just being extended) and mark the pointer as leaky() after that realloc,\n> or unregister the original pointer from leaks before calling realloc and\n> register what comes back. It will easily get messy.\n\n Hmm indeed, maybe it isn't such a good idea then.\n\n> By the way, the series queued in your repository still has \"s/pring/print/\"\n> typo in 4/7 and \"argv not NULL terminated\" comment in 6/7.\n\n I'll fix that and pushed again, without the leaky() series dependency\n(I've put in a comment that I'm aware that it's a leak), and with the\ntwo fixes you mention done.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"84344","messageId":"20080722180944.GB3561@efreet.light.src","threadId":"14128","inReplyTo":"20080626213304.GA22111@artemis.madism.org","subject":"Re: [PATCH 1/2] Introduce leaky().","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2008-07-22T18:09:44Z","receivedAt":"2008-07-22T18:09:44Z","isPatch":true,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"On Thu, Jun 26, 2008 at 23:33:04 +0200, Pierre Habouzit wrote:\n> On Thu, Jun 26, 2008 at 06:46:43PM +0000, Junio C Hamano wrote:\n> > The user would also need to worry about not freeing the original\n> > allocation pointer when something is realloc(3)ed, which means either\n> > finding the last realloc(3) of the object (that is logically the same, but\n> > just being extended) and mark the pointer as leaky() after that realloc,\n> > or unregister the original pointer from leaks before calling realloc and\n> > register what comes back. It will easily get messy.\n> \n> Hmm indeed, maybe it isn't such a good idea then.\n\nI don't think it's necessary either. Valgrind (unlike other leak detectors\nthe poor windows folks are stuck with) is quite smart and will only report\nmemory as leaked when there is no pointer to it. So it should not report any\nmemory pointed to by index or any other static structure.\n\nAdditionally since valgrind allows one to write rules to exclude\nuninteresting reports. So a good approach would be to just keep such\nexclusion file around in git with the known uninteresting cases.\n\n> > By the way, the series queued in your repository still has \"s/pring/print/\"\n> > typo in 4/7 and \"argv not NULL terminated\" comment in 6/7.\n> \n> I'll fix that and pushed again, without the leaky() series dependency\n> (I've put in a comment that I'm aware that it's a leak), and with the\n> two fixes you mention done.\n> \n> -- \n> ·O· Pierre Habouzit\n> ··O madcoder@debian.org\n> OOO http://www.madism.org\n\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec \n"}]}