Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
Hi Martin, On Fri, 23 Nov 2018, Martin Ågren wrote: > On Fri, 23 Nov 2018 at 11:13, Johannes Schindelin > wrote: > > On Mon, 30 Oct 2017, Pranit Bauva wrote: > > > > > On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren > > > wrote: > > > > On 27 October 2017 at 17:06, Pranit Bauva > > > > wrote: > > > >> +static void free_terms(struct bisect_terms *terms) > > > >> +{ > > > >> + if (!terms->term_good) > > > >> + free((void *) terms->term_good); > > > >> + if (!terms->term_bad) > > > >> + free((void *) terms->term_bad); > > > >> +} > > > > > You leave the pointers dangling, but you're ok for now since this is the > > > > last thing that happens in `cmd_bisect__helper()`. Your later patches > > > > add more users, but they're also ok, since they immediately assign new > > > > values. > > > > > > > > In case you (and others) find it useful, the below is a patch I've been > > > > sitting on for a while as part of a series to plug various memory-leaks. > > > > `FREE_AND_NULL_CONST()` would be useful in precisely these situations. > > > > > > Honestly, I wouldn't be the best person to judge this. > > > > Git's source code implicitly assumes that any `const` pointer refers to > > memory owned by another code path. It is therefore probably not a good > > idea to encourage `free()`ing a `const` pointer. > > Yeah, I never submitted that patch as part of a real series. I remember > having a funky feeling about it, and whatever use-case I had for this > macro, I managed to solve the memory leak in some other way in a > rewrite. > > Thanks for a sanity check. I am glad you agree, and it's just fair that I contribute a sanity check on this here list when I have benefitted so many times from the same. Ciao, Johannes
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
On Fri, 23 Nov 2018 at 11:13, Johannes Schindelin wrote: > On Mon, 30 Oct 2017, Pranit Bauva wrote: > > > On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren > > wrote: > > > On 27 October 2017 at 17:06, Pranit Bauva wrote: > > >> +static void free_terms(struct bisect_terms *terms) > > >> +{ > > >> + if (!terms->term_good) > > >> + free((void *) terms->term_good); > > >> + if (!terms->term_bad) > > >> + free((void *) terms->term_bad); > > >> +} > > > You leave the pointers dangling, but you're ok for now since this is the > > > last thing that happens in `cmd_bisect__helper()`. Your later patches > > > add more users, but they're also ok, since they immediately assign new > > > values. > > > > > > In case you (and others) find it useful, the below is a patch I've been > > > sitting on for a while as part of a series to plug various memory-leaks. > > > `FREE_AND_NULL_CONST()` would be useful in precisely these situations. > > > > Honestly, I wouldn't be the best person to judge this. > > Git's source code implicitly assumes that any `const` pointer refers to > memory owned by another code path. It is therefore probably not a good > idea to encourage `free()`ing a `const` pointer. Yeah, I never submitted that patch as part of a real series. I remember having a funky feeling about it, and whatever use-case I had for this macro, I managed to solve the memory leak in some other way in a rewrite. Thanks for a sanity check. Martin
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
Hi Pranit, (Cc:ing Tanushree because they will try to pick up this patch series as part of the Outreachy program.) On Mon, 30 Oct 2017, Pranit Bauva wrote: > On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren wrote: > > On 27 October 2017 at 17:06, Pranit Bauva wrote: > >> +static void free_terms(struct bisect_terms *terms) > >> +{ > >> + if (!terms->term_good) > >> + free((void *) terms->term_good); > >> + if (!terms->term_bad) > >> + free((void *) terms->term_bad); > >> +} > > > > These look like no-ops. Remove `!` for correctness, or `if (...)` for > > simplicity, since `free()` can handle NULL. > > I probably forgot to do this here. I will make the change. > > > You leave the pointers dangling, but you're ok for now since this is the > > last thing that happens in `cmd_bisect__helper()`. Your later patches > > add more users, but they're also ok, since they immediately assign new > > values. > > > > In case you (and others) find it useful, the below is a patch I've been > > sitting on for a while as part of a series to plug various memory-leaks. > > `FREE_AND_NULL_CONST()` would be useful in precisely these situations. > > Honestly, I wouldn't be the best person to judge this. Git's source code implicitly assumes that any `const` pointer refers to memory owned by another code path. It is therefore probably not a good idea to encourage `free()`ing a `const` pointer. Which brings me back to the question: who really owns that allocated memory to which `term_good` and `term_bad` refer? Ciao, Johannes
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
On 27/10/17 16:06, Pranit Bauva wrote: > Reimplement the `bisect_write` shell function in C and add a > `bisect-write` subcommand to `git bisect--helper` to call it from > git-bisect.sh > > Using `--bisect-write` subcommand is a temporary measure to port shell > function in C so as to use the existing test suite. As more functions > are ported, this subcommand will be retired but its implementation will > be called by some other methods. > > Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD > from the global shell script thus we need to pass it to the subcommand > using the arguments. We then store them in a struct bisect_terms and > pass the memory address around functions. > > Add a log_commit() helper function to write the contents of the commit message > header to a file which will be re-used in future parts of the code as > well. > > Also introduce a function free_terms() to free the memory of `struct > bisect_terms` and set_terms() to set the values of members in `struct > bisect_terms`. > > Helped-by: Ramsay Jones> Mentored-by: Lars Schneider > Mentored-by: Christian Couder > Signed-off-by: Pranit Bauva > --- > builtin/bisect--helper.c | 107 > +-- > git-bisect.sh| 25 ++- > 2 files changed, 108 insertions(+), 24 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 12754448f7b6a..6295f53c850a8 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -12,15 +12,37 @@ static GIT_PATH_FUNC(git_path_bisect_expected_rev, > "BISECT_EXPECTED_REV") > static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK") > static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") > static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD") > +static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") > > static const char * const git_bisect_helper_usage[] = { > N_("git bisect--helper --next-all [--no-checkout]"), > N_("git bisect--helper --write-terms "), > N_("git bisect--helper --bisect-clean-state"), > N_("git bisect--helper --bisect-reset []"), > + N_("git bisect--helper --bisect-write > []"), > NULL > }; > > +struct bisect_terms { > + const char *term_good; > + const char *term_bad; > +}; > + > +static void free_terms(struct bisect_terms *terms) > +{ > + if (!terms->term_good) > + free((void *) terms->term_good);> + if (!terms->term_bad) > + free((void *) terms->term_bad); You can pass a NULL pointer to free(), so the conditionals here are not required. > +} > + > +static void set_terms(struct bisect_terms *terms, const char *bad, > + const char *good) > +{ > + terms->term_good = xstrdup(good); > + terms->term_bad = xstrdup(bad); > +} > + > /* > * Check whether the string `term` belongs to the set of strings > * included in the variable arguments. > @@ -146,6 +168,72 @@ static int bisect_reset(const char *commit) > return bisect_clean_state(); > } > > +static void log_commit(FILE *fp, char *fmt, const char *state, > +struct commit *commit) > +{ > + struct pretty_print_context pp = {0}; > + struct strbuf commit_msg = STRBUF_INIT; > + char *label = xstrfmt(fmt, state); > + > + format_commit_message(commit, "%s", _msg, ); > + > + fprintf(fp, "# %s: [%s] %s\n", label, oid_to_hex(>object.oid), > + commit_msg.buf); > + > + strbuf_release(_msg); > + free(label); > +} > + > +static int bisect_write(const char *state, const char *rev, > + const struct bisect_terms *terms, int nolog) > +{ > + struct strbuf tag = STRBUF_INIT; > + struct object_id oid; > + struct commit *commit; > + FILE *fp = NULL; > + int retval = 0; > + > + if (!strcmp(state, terms->term_bad)) { > + strbuf_addf(, "refs/bisect/%s", state); > + } else if (one_of(state, terms->term_good, "skip", NULL)) { > + strbuf_addf(, "refs/bisect/%s-%s", state, rev); > + } else { > + error(_("Bad bisect_write argument: %s"), state); > + goto fail; > + } > + > + if (get_oid(rev, )) { > + error(_("couldn't get the oid of the rev '%s'"), rev); > + goto fail; > + } > + > + if (update_ref(NULL, tag.buf, , NULL, 0, > +UPDATE_REFS_MSG_ON_ERR)) > + goto fail; > + > + fp = fopen(git_path_bisect_log(), "a"); > + if (!fp) { > + error_errno(_("couldn't open the file '%s'"), > git_path_bisect_log()); > + goto fail; > + } > + > + commit = lookup_commit_reference(); > + log_commit(fp, "%s", state, commit); > + > + if (!nolog) > + fprintf(fp, "git bisect %s %s\n", state, rev); > + > + goto finish;
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
Hey Junio, On Fri, Oct 27, 2017 at 11:49 PM, Junio C Hamanowrote: > Pranit Bauva writes: > >> - bisect_write "$state" "$rev" >> + git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" >> "$TERM_BAD" || exit > > I can see why two extra "terms" parameters need to be passed to this > helper at this step; looking at patches around 4/8 and 6/8 where C > code can directly find out what words are used for GOOD and BAD, we > should be able to lose these two extra parameters from this helper > by internally making a call to get_terms() from bisect_write() ;-) Yes quite true, but then after converting bisect_skip() we can completely get rid of this line and then it won't be needed in the ported C code. PS: I have already ported that function but those patches are local as of now. Regards, Pranit Bauva
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
Hey Martin, On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågrenwrote: > On 27 October 2017 at 17:06, Pranit Bauva wrote: >> +static void free_terms(struct bisect_terms *terms) >> +{ >> + if (!terms->term_good) >> + free((void *) terms->term_good); >> + if (!terms->term_bad) >> + free((void *) terms->term_bad); >> +} > > These look like no-ops. Remove `!` for correctness, or `if (...)` for > simplicity, since `free()` can handle NULL. I probably forgot to do this here. I will make the change. > You leave the pointers dangling, but you're ok for now since this is the > last thing that happens in `cmd_bisect__helper()`. Your later patches > add more users, but they're also ok, since they immediately assign new > values. > > In case you (and others) find it useful, the below is a patch I've been > sitting on for a while as part of a series to plug various memory-leaks. > `FREE_AND_NULL_CONST()` would be useful in precisely these situations. Honestly, I wouldn't be the best person to judge this. Regards, Pranit Bauva
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
On 10/30/2017 02:38 PM, Stephan Beyer wrote: > This last change is not necessary. You never use "res". Whoops, ignore this! I compared old and new diff and mixed something up, it seems. Sorry, Stephan
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
Also just a minor in addition to the others: > @@ -153,9 +241,10 @@ int cmd_bisect__helper(int argc, const char **argv, > const char *prefix) > WRITE_TERMS, > BISECT_CLEAN_STATE, > CHECK_EXPECTED_REVS, > - BISECT_RESET > + BISECT_RESET, > + BISECT_WRITE > } cmdmode = 0; > - int no_checkout = 0; > + int no_checkout = 0, res = 0; This last change is not necessary. You never use "res". Stephan
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
Pranit Bauvawrites: > - bisect_write "$state" "$rev" > + git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" > "$TERM_BAD" || exit I can see why two extra "terms" parameters need to be passed to this helper at this step; looking at patches around 4/8 and 6/8 where C code can directly find out what words are used for GOOD and BAD, we should be able to lose these two extra parameters from this helper by internally making a call to get_terms() from bisect_write() ;-)
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
On 27 October 2017 at 17:06, Pranit Bauvawrote: > +static void free_terms(struct bisect_terms *terms) > +{ > + if (!terms->term_good) > + free((void *) terms->term_good); > + if (!terms->term_bad) > + free((void *) terms->term_bad); > +} These look like no-ops. Remove `!` for correctness, or `if (...)` for simplicity, since `free()` can handle NULL. You leave the pointers dangling, but you're ok for now since this is the last thing that happens in `cmd_bisect__helper()`. Your later patches add more users, but they're also ok, since they immediately assign new values. In case you (and others) find it useful, the below is a patch I've been sitting on for a while as part of a series to plug various memory-leaks. `FREE_AND_NULL_CONST()` would be useful in precisely these situations. -- >8 -- Subject: git-compat-util: add FREE_AND_NULL_CONST() wrapper Commit 481df65 ("git-compat-util: add a FREE_AND_NULL() wrapper around free(ptr); ptr = NULL", 2017-06-15) added `FREE_AND_NULL()`. One advantage of this macro is that it reduces risk of copy-paste errors and reviewer-fatigue, especially when cleaning up more than just a single pointer. However, `FREE_AND_NULL()` can not be used with a const-pointer: struct foo { const char *bar; } ... FREE_AND_NULL(baz.bar); This causes the compiler to barf as `free()` is called: "error: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type". A naive attempt to remedy this might look like this: FREE_AND_NULL((void *)baz.bar); Now the assignment is problematic: "error: lvalue required as left operand of assignment". Add a `FREE_AND_NULL_CONST()` wrapper macro which acts as `FREE_AND_NULL()`, except it casts to `(void *)` when freeing. As a demonstration, use this in two existing code paths that were identified by some grepping. Future patches will use it to clean up struct-fields: FREE_AND_NULL_CONST(baz.bar); This macro is a slightly bigger hammer than `FREE_AND_NULL` and has a slightly larger potential for misuse. Document that `FREE_AND_NULL` should be preferred. Signed-off-by: Martin Ågren --- argv-array.c | 3 +-- git-compat-util.h | 8 submodule.c | 3 +-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/argv-array.c b/argv-array.c index 5d370fa33..433a5997a 100644 --- a/argv-array.c +++ b/argv-array.c @@ -59,8 +59,7 @@ void argv_array_pop(struct argv_array *array) { if (!array->argc) return; - free((char *)array->argv[array->argc - 1]); - array->argv[array->argc - 1] = NULL; + FREE_AND_NULL_CONST(array->argv[array->argc - 1]); array->argc--; } diff --git a/git-compat-util.h b/git-compat-util.h index cedad4d58..ca3dcbc58 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -815,6 +815,14 @@ extern FILE *fopen_or_warn(const char *path, const char *mode); */ #define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0) +/* + * FREE_AND_NULL_CONST(ptr) is like FREE_AND_NULL(ptr) except it casts + * to (void *) when calling free. This is useful when handling, e.g., a + * `const char *`, but it can also be misused. Prefer FREE_AND_NULL, and + * use this only when you need to and it is safe to do so. + */ +#define FREE_AND_NULL_CONST(p) do { free((void *)(p)); (p) = NULL; } while (0) + #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc))) #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc))) diff --git a/submodule.c b/submodule.c index 63e7094e1..7759f9050 100644 --- a/submodule.c +++ b/submodule.c @@ -364,8 +364,7 @@ int parse_submodule_update_strategy(const char *value, { enum submodule_update_type type; - free((void*)dst->command); - dst->command = NULL; + FREE_AND_NULL_CONST(dst->command); type = parse_submodule_update_type(value); if (type == SM_UPDATE_UNSPECIFIED) -- 2.15.0.rc2.5.g97dd00bb7
[PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
Reimplement the `bisect_write` shell function in C and add a `bisect-write` subcommand to `git bisect--helper` to call it from git-bisect.sh Using `--bisect-write` subcommand is a temporary measure to port shell function in C so as to use the existing test suite. As more functions are ported, this subcommand will be retired but its implementation will be called by some other methods. Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD from the global shell script thus we need to pass it to the subcommand using the arguments. We then store them in a struct bisect_terms and pass the memory address around functions. Add a log_commit() helper function to write the contents of the commit message header to a file which will be re-used in future parts of the code as well. Also introduce a function free_terms() to free the memory of `struct bisect_terms` and set_terms() to set the values of members in `struct bisect_terms`. Helped-by: Ramsay JonesMentored-by: Lars Schneider Mentored-by: Christian Couder Signed-off-by: Pranit Bauva --- builtin/bisect--helper.c | 107 +-- git-bisect.sh| 25 ++- 2 files changed, 108 insertions(+), 24 deletions(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 12754448f7b6a..6295f53c850a8 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -12,15 +12,37 @@ static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV") static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK") static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD") +static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static const char * const git_bisect_helper_usage[] = { N_("git bisect--helper --next-all [--no-checkout]"), N_("git bisect--helper --write-terms "), N_("git bisect--helper --bisect-clean-state"), N_("git bisect--helper --bisect-reset []"), + N_("git bisect--helper --bisect-write []"), NULL }; +struct bisect_terms { + const char *term_good; + const char *term_bad; +}; + +static void free_terms(struct bisect_terms *terms) +{ + if (!terms->term_good) + free((void *) terms->term_good); + if (!terms->term_bad) + free((void *) terms->term_bad); +} + +static void set_terms(struct bisect_terms *terms, const char *bad, + const char *good) +{ + terms->term_good = xstrdup(good); + terms->term_bad = xstrdup(bad); +} + /* * Check whether the string `term` belongs to the set of strings * included in the variable arguments. @@ -146,6 +168,72 @@ static int bisect_reset(const char *commit) return bisect_clean_state(); } +static void log_commit(FILE *fp, char *fmt, const char *state, + struct commit *commit) +{ + struct pretty_print_context pp = {0}; + struct strbuf commit_msg = STRBUF_INIT; + char *label = xstrfmt(fmt, state); + + format_commit_message(commit, "%s", _msg, ); + + fprintf(fp, "# %s: [%s] %s\n", label, oid_to_hex(>object.oid), + commit_msg.buf); + + strbuf_release(_msg); + free(label); +} + +static int bisect_write(const char *state, const char *rev, + const struct bisect_terms *terms, int nolog) +{ + struct strbuf tag = STRBUF_INIT; + struct object_id oid; + struct commit *commit; + FILE *fp = NULL; + int retval = 0; + + if (!strcmp(state, terms->term_bad)) { + strbuf_addf(, "refs/bisect/%s", state); + } else if (one_of(state, terms->term_good, "skip", NULL)) { + strbuf_addf(, "refs/bisect/%s-%s", state, rev); + } else { + error(_("Bad bisect_write argument: %s"), state); + goto fail; + } + + if (get_oid(rev, )) { + error(_("couldn't get the oid of the rev '%s'"), rev); + goto fail; + } + + if (update_ref(NULL, tag.buf, , NULL, 0, + UPDATE_REFS_MSG_ON_ERR)) + goto fail; + + fp = fopen(git_path_bisect_log(), "a"); + if (!fp) { + error_errno(_("couldn't open the file '%s'"), git_path_bisect_log()); + goto fail; + } + + commit = lookup_commit_reference(); + log_commit(fp, "%s", state, commit); + + if (!nolog) + fprintf(fp, "git bisect %s %s\n", state, rev); + + goto finish; + +fail: + retval = -1; +finish: + if (fp) + fclose(fp); + strbuf_release(); + return retval; +} + int cmd_bisect__helper(int argc, const char **argv, const char *prefix) { enum { @@ -153,9 +241,10 @@ int cmd_bisect__helper(int argc,