patch, 11 partsmidx: clean up t5319 under 'SANITIZE=leak'

Here is a topic that Ævar got me interested when he mentioned that t5319 is leaky[1].

This is the result of trying to get t5319 to pass when compiling with SANITIZE=leak. About 50% of the fixes are in the MIDX code, another 40% are in the pack-bitmap code, and the remaining 10% are sort of random.

I tried to separate these out based on their respective areas. The last 10% are all from leaking memory in the rev_info structure, which I punted on for now by just UNLEAK()-ing it. That's all done in the last patch. If we choose to take that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a little gross, so I'm happy to leave it out if others would prefer.

This is based on tb/fix-midx-rename-while-mapped, with a back-merge of js/windows-ci-path-fix in order to appease CI. It also contains a little bit of overlap with a patch[2] from Ævar in another series, which he and I discovered independently. I cherry-picked his patch, since I haven't seen much action on that series lately.

[1]: https://lore.kernel.org/git/87wnmph73b.fsf@evledraar.gmail.com/ [2]: https://lore.kernel.org/git/patch-v3-08.10-e0a3510dd88-20211013T222329Z-avarab@gmail.com/

Taylor Blau (10):
  midx.c: clean up chunkfile after reading the MIDX
  midx.c: don't leak MIDX from verify_midx_file
  t/helper/test-read-midx.c: free MIDX within read_midx_file()
  builtin/pack-objects.c: don't leak memory via arguments
  builtin/repack.c: avoid leaking child arguments
  builtin/multi-pack-index.c: don't leak concatenated options
  pack-bitmap.c: avoid leaking via midx_bitmap_filename()
  pack-bitmap.c: don't leak type-level bitmaps
  pack-bitmap.c: more aggressively free in free_bitmap_index()
  t5319: UNLEAK() the remaining leaks

Ævar Arnfjörð Bjarmason (1):
  pack-bitmap-write.c: don't return without stop_progress()

 builtin/log.c               |  1 +
 builtin/multi-pack-index.c  |  4 ++++
 builtin/pack-objects.c      | 12 ++++++++----
 builtin/repack.c            | 35 ++++++++++++++++++++++++-----------
 builtin/rev-list.c          |  2 ++
 midx.c                      |  7 +++++--
 pack-bitmap-write.c         |  8 +++++---
 pack-bitmap.c               | 18 ++++++++++++++++--
 t/helper/test-read-midx.c   |  3 ++-
 t/t5319-multi-pack-index.sh |  1 +
 10 files changed, 68 insertions(+), 23 deletions(-)

In order to read the contents of a MIDX, we initialize a chunkfile structure which can read the table of contents and assign pointers into different sections of the file for us.

We do call free(), since the chunkfile struct is heap allocated, but not the more appropriate free_chunkfile(), which also frees memory that the structure itself owns.

Call that instead to avoid leaking memory in this function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
The patch contents are from Ævar, but the message is mine. I hope that
he doesn't mind me forging his sign-off here.

 midx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Taylor Blau <me@ttaylorr.com> writes:

The former is not something we can mechanically locate, but the latter we should be able to. And indeed this is the only instance the following experiment finds.

	$ cat >contrib/coccinelle/chunkfile.cocci <<-\EOF
	@@
	identifier f !~ "^free_chunkfile$";
	struct chunkfile *cf;
	@@
	  f(...) {<...
	- free(cf)
	+ free_chunkfile(cf)
	  ...>}
	EOF
	$ make contrib/coccinelle/chunkfile.cocci.patch

Thanks.

On Wed, Oct 20 2021, Taylor Blau wrote:

I don't mind, and (and maybe we should have an in-tree text file for this) like I think Jeff King has noted for his inline-posted patches for discussion you can assume my SOB for anything I post here on list (except noted otherwise) in terms of copyright.

Taylor Blau <me@ttaylorr.com> writes:

Not a fault of this patch, but I think the code is calling close() on an already closed file descriptor in cleanup_fail codepath, when "goto cleanup_fail" is reached after xmmap() returned, e.g. when oid_version() does not match hash_version, when we failed to read the ToC.

Also, it is not clear why is it a dying offence if we do not find packnames chunk, but it is just a "pretend as if we do not have this midx file and everybody else is happy" when we failed to read the ToC.

On Thu, Oct 21, 2021 at 09:16:27AM -0700, Junio C Hamano wrote:

Yep, I agree with you on both of those. I would be happier to see fewer die()s deep within midx.c. I'll start a list for myself of some potential future cleanups in this area that don't involve memory leaks.

My hunch is that there's enough subtlty here that we shouldn't tie the two (fixing leaks, and other general issues/tidiness in the MIDX code) together. So I'll keep track of the latter separately and address those in future series.

Thanks, Taylor

The function midx.c:verify_midx_file() allocate a MIDX struct by calling load_multi_pack_index(). But when cleaning up, it calls free() without freeing any resources associated with the MIDX.

Call the more appropriate close_midx() which does free those resources, which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On Wed, Oct 20, 2021 at 11:39 PM Taylor Blau <me@ttaylorr.com> wrote:

s/allocate/allocates/

Using the `goto` idiom to ensure cleanup makes perfect sense. For a few reasons[*], I did spend a some moments wondering if the cognitive load might be a bit lower by instead adding two close_midx() calls -- one at the early return and one just before the normal return -- rather than using a `goto`, but it's so subjective that it's not worth worrying about.

FOOTNOTES

[*] First, unlike most cases in which the `goto` jumps over relatively short blocks of code, the distance in this case between `goto` and the new label is significant and it's not easy to see at a glance what is being skipped and how important it might be. Second, `pairs` is still NULL at the point of the `goto`; I spent extra time checking and double-checking what free(pairs) was doing in this instance. Finally, there are enough start/stop-progress calls in this function that it requires extra mental effort to determine that this `goto` is indeed safe (at least for the present).

Eric Sunshine <sunshine@sunshineco.com> writes:

Carefully reading the patch is very good, but at least in a patch like this, it is immediately obvious that the patch is not making any of the things the footnote worries about any worse than the original, by replacing "return" (which by definition will skip all the things the goto did jump over) with a "goto" to the bottom, no?

Thanks.

Taylor Blau <me@ttaylorr.com> writes:

Nice.

By the way, the function starts like so:

    int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags)
    {
            struct pair_pos_vs_id *pairs = NULL;
            uint32_t i;
            struct progress *progress = NULL;
            struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
            verify_midx_error = 0;

            if (!m) {
                    int result = 0;
                    struct stat sb;
                    char *filename = get_midx_filename(object_dir);
                    if (!stat(filename, &sb)) {
                            error(_("multi-pack-index file exists, but failed to parse"));
                            result = 1;
                    }
                    free(filename);
                    return result;
            }

but after seeing die() sprinkled in load_multi_pack_index() with checks during parsing, I am not sure if this is a good error reporting mechanism we are seeing here.

It is wonderful to plug leaks here and there, but to be honest, even with only a very little parts I saw in this code, I think there are other things that need clean-up here.

Also, the way the file-scope global verify_midx_error is used is beyond words _ugly_, if the only reason for its use is to make midx_report() look simpler, which is what I think is happening.

Not very impressed, but all of the above is not a new issue introduced by this patch.

Thanks.

When calling `read_midx_file()` to show information about a MIDX or list the objects contained within it we fail to call `close_midx()`, leaking the memory allocated to store that MIDX.

Fix this by calling `close_midx()` before exiting the function. We can drop the "early" return when `show_objects` is non-zero, since the next instruction is also a return.

(We could just as easily put a `cleanup` label here as with previous patches. But the only other time we terminate the function early is when we fail to load a MIDX in the first place. `close_midx()` does handle a NULL argument, but the extra complexity is likely not warranted).

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/helper/test-read-midx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

When constructing arguments to pass to setup_revision(), pack-objects only frees the memory used by that array after calling get_object_list().

Ensure that we call strvec_clear() whether or not we use the arguments array by cleaning up whenever we exit the function (and rewriting one early return to jump to a label which frees the memory and then returns).

We could avoid setting this array up altogether unless we are in the if-else block that calls get_object_list(), but setting up the argument array is intermingled with lots of other side-effects, e.g.:

    if (exclude_promisor_objects) {
      use_internal_rev_list = 1;
      fetch_if_missing = 0;
      strvec_push(&rp, "--exclude-promisor-objects");
    }

So it would be awkward to check exclude_promisor_objects twice: first to set use_internal_rev_list and fetch_if_missing, and then again above get_object_list() to push the relevant argument onto the array.

Instead, leave the array's construction alone and make sure to free it unconditionally.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/pack-objects.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

`git repack` invokes a handful of child processes: one to write the actual pack, and optionally ones to repack promisor objects and update the MIDX.

In none of these cases do we bother to call child_process_clear(), which frees the memory associated with each child's arguments and environment.

In order to do so, tweak each function that spawns a child process to have a `cleanup` label that we always visit before returning from each function. Then, make sure that we call child_process_clear() as a part of that label.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

On 10/20/2021 11:39 PM, Taylor Blau wrote:

nit: "ret" is short for "return" which makes me think "this will be used as 'return ret;'" but we don't do that. Instead, we use it as the _result_ of an inner call:

For that reason, I would rename this to "res" or "result".

Here, you are taking the result of an inner call, but actually returning it. This makes sense to be "ret".

Also here is good.

Thanks, -Stolee

Derrick Stolee <stolee@gmail.com> writes:

Yup, my reading hiccupped there, too.

Yeah, if we are introducing a new variable, result is a good name.

Taylor Blau <me@ttaylorr.com> writes:

I have a slight aversion against the fact that this patch makes us call child_process_clear(), when we know we may have called finish_command(). Callers of finish_command() in other codepaths rely on the fact that finish_command() calls child_process_clear(), which means that because of this change, we are now constrained to keep child_process_clear() idempotent?

On Thu, Oct 21, 2021 at 09:37:07AM -0700, Junio C Hamano wrote:

Good point. In functions besides cmd_repack(), it's easy enough to just call child_process_clear() once before returning instead of using a label. That has the added benefit of side-stepping the variable name question that you and Stolee raised (but I think whether we call it "result", "res", or "ret" is besides the point).

In cmd_repack(), I'd like to wrap the other cleanup-related calls underneath a 'cleanup' label. So there we want to track whether or not we have called finish_command(), so that we only call child_process_clear() when we *haven't* already called it via finish_command().

Thanks, Taylor

The `multi-pack-index` builtin dynamically allocates an array of command-line option for each of its separate modes by calling add_common_options() to concatante the common options with sub-command specific ones.

Because this operation allocates a new array, we have to be careful to remember to free it. We already do this in the repack and write sub-commands, but verify and expire don't. Rectify this by calling FREE_AND_NULL as the other modes do.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/multi-pack-index.c | 4 ++++
 1 file changed, 4 insertions(+)

To construct the path for a MIDX bitmap, midx_bitmap_filename() calls get_midx_filename(), and uses the result as an argument to xstrfmt(). But get_midx_filename() also uses xstrfmt, and it assumes that the caller will free the results.

get_midx_filename() could take a strbuf and write to that, but it would make more work for every other caller. midx_bitmap_filename() could store the result of get_midx_filename() in a temporary variable which is freed before returning.

But that intermediate allocation feels wasteful. Since this function is simple enough, we can instead "inline" its implementation into the xstrfmt call itself. That is a little gross, but arguably better than allocating memory unnecessarily.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Taylor Blau <me@ttaylorr.com> writes:

The function being "simple enough" does not help maintenance burden this change brings in at all, does it? Unless it is accompanied by a comment "the template string used here must be kept in sync with what is used in the get_midx_filename() defined elsewhere" near the lines this patch touches, perhaps in big red letters, that is.

Without this caller, get_midx_filename() could be file-scope static in midx.c and can be eliminated from midx.h, it seems. If there are many other filenames like midx_bitmap that are derrived from midx_filename, it might be worth extending get_midx_filename to take an extra format parameter (i.e. "I want midx-related filename for this object directory, but I am getting this .bitmap kind"). Another remedy may be to move this function to midx.c and make it sit next to what this function must be always in sync with.

Or use a C preprocessor macro for "%s/pack/multi-pack-index" in midx.h and use it here and there to make sure they will stay in sync.

But after reading all the users of get_midx_filename(), especially given that it is mostly internal implementation detail inside midx.c, I think rewriting

	f() {
	char *midx = get_midx_filename(...);

	... use midx ...

	cleanup:
	free(midx);
	}

into

	f() {
	struct strbuf midx;

	get_midx_filename(&midx, ...);

	... use midx.buf ...

	cleanup:
	strbuf_release(&midx);
	}

is not too bad a fix for this.

On Thu, Oct 21, 2021 at 09:54:39AM -0700, Junio C Hamano wrote:

I was initially worried that this was too heavy-handed an approach. But I hadn't actually tried it, and updating the half-dozen callers of get_midx_filename() took only a couple of minutes.

But just because something is easy doesn't necessarily mean that it is a good idea to do ;). Luckily, this approach ended up being clean, and allows us to implement midx_bitmap_filename() without copying, open-coding, or leaking.

Thanks for the nice suggestion.

Taylor

test_bitmap_walk() is used to implement `git rev-list --test-bitmap`, which compares the result of the on-disk bitmaps with ones generated on-the-fly during a revision walk.

In fa95666a40 (pack-bitmap.c: harden 'test_bitmap_walk()' to check type bitmaps, 2021-08-24), we hardened those tests to also check the four special type-level bitmaps, but never freed those bitmaps. We should have, since each required an allocation when we EWAH-decompressed them.

Free those, plugging that leak, and also free the base (the scratch-pad bitmap), too.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 6 ++++++
 1 file changed, 6 insertions(+)

Taylor Blau <me@ttaylorr.com> writes:

I've looked at the patches up to this point. The ones I did not comment (including this one ;-) looked perfect, and all of them (including the ones I did comment) were cleanly described to easily read the reasoning behind these changes, which I highly appreciated.

Especially if I did not quite agree with the reasoning, it helped me clarify my thought where I found it not agreeable.

Thanks.

The function free_bitmap_index() is somewhat lax in what it frees. There are two notable examples:

  - While it does call kh_destroy_oid_map on the "bitmaps" map (which
    maps) commit OIDs to their corresponding bitmaps, the bitmaps
    themselves are not freed. Note here that we recycle already-freed
    ewah_bitmaps into a pool, but these are handled correctly by
    ewah_pool_free().

  - We never bother to free the extended index's "positions" map, which
    we always allocate in load_bitmap().

Fix both of these.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau <me@ttaylorr.com> wrote:

The parentheses placement seems off; it's not clear what the intent is. Perhaps either move the closing parenthesis to just before the comma or drop them altogether.

Eric Sunshine <sunshine@sunshineco.com> writes:

Yeah, I think we can do without them and the sentence becomes clearer (we can add a comma before "which", too).

On Thu, Oct 21, 2021 at 11:32:16AM -0700, Junio C Hamano wrote:

Yep, thanks both.

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

It is not a fault of this patch, but I was reading the corresponding load_bitmap() to see if we are covering everything here.

On the error path of that function, where it does "got failed", it does not clean after itself very well, it seems. If we read commits bitmap successfully but failed to read trees bitmap, for example, the resource held by commits bitmap is never reclaimed.

This resolves the remaining leaks uncovered by running t5319 when building with SANITIZE=leak. All of the leaks below are related to allocated memory stored in 'struct rev_info' which is never freed.

We could alternatively implement a function which frees all allocated memory associated with the rev_info struct, but doing this properly is a bigger undertaking than I want to deal with right now.

Instead, let's just UNLEAK() the few ones that are holding up t5319, which enables us to mark that test as TEST_PASSES_SANITIZE_LEAK.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/log.c               | 1 +
 builtin/pack-objects.c      | 1 +
 builtin/rev-list.c          | 2 ++
 pack-bitmap-write.c         | 2 ++
 t/t5319-multi-pack-index.sh | 1 +
 5 files changed, 7 insertions(+)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Fix a bug that's been here since 7cc8f971085 (pack-objects: implement bitmap writing, 2013-12-21), we did not call stop_progress() if we reached the early exit in this function.

We could call stop_progress() before we return, but better yet is to defer calling start_progress() until we need it.

This will matter in a subsequent commit where we BUG(...) out if this happens, and matters now e.g. because we don't have a corresponding "region_end" for the progress trace2 event.

Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap-write.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau <me@ttaylorr.com> wrote:

Nit: There is no subsequent commit in this series which "BUGS out", so
this comment is confusing.

On Wed, Oct 20 2021, Taylor Blau wrote:

Looks good :) Just a note that this is in "ab/only-single-progress-at-once" already in case you didn't spot it.

That series is marked for "next?" (with a question mark) by Junio in the latest what's cooking, so *hint* *hint* for any last minute review :)

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

I wonder if it would help contributors if we give them a simple rule to follow before sending their patches out:

 - You develop on an appropriate base (either on maint, or master,
   or a merge of selected topics in flight you absolutely have to
   depend on) as usual.

 - You test merge the result to 'seen' and to 'next'.  If you do not
   get heavy conflicts and the tests pass, you are OK.

 - Otherwise, you study what is conflicting, and should review it
   before you submit your topic.

Without understanding what others are doing in the same area, it is hard to make productive use of your time to develop new things, and if you are reading others' work in the same area anyway, it would be a minimum additional cost to write about what you learned in their work.

On Thu, Oct 21, 2021 at 11:39:34AM -0700, Junio C Hamano wrote:

That seems like sensible advice to me. When I sent the series out last night, I figured that it would be preferable to cherry-pick the commit from Ævar's series, since it wasn't clear whether or not all or parts of that series were actively moving forward (besides being marked as "next?" in your What's Cooking email).

But I'll send the next iteration based on a combined merge of my topic and Ævar's, and note your branch names for both of them clearly. To encourage both of our topics to move forward, I'll throw Ævar's onto my review queue.

Thanks, Taylor

Junio C Hamano <gitster@pobox.com> writes:

Addendum to this third step.

     ... Also, if you chose to depend on other topics when you
     decided on the base in the first step, giving them your review
     would be a good way to show others that _you_ understand the
     code that you are building on top of, which in turn may give
     others confidence in your topic.

Thanks.

We encourage identifying what, among many topics on `next`, exact topics a new work depends on, instead of building directly on `next`. Let's clarify this in the documentation.

Developers should know what they are building on top of, and be aware of which part of the system is currently being worked on. Encouraging them to make trial merges to `next` and `seen` themselves will incentivize them to read others' changes and understand them, eventually helping the developers to coordinate among themselves and reviewing each others' changes.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This, being a guide for those who submit their work, stops short
   of telling them to send reviews on others' patch that interacts
   with their work, but reading and understanding others' work is
   both necessary to make their own work to play well together with
   other topics, and to start reviewing others' work.

 Documentation/SubmittingPatches | 50 ++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 13 deletions(-)

On Sat, Oct 23 2021, Junio C Hamano wrote:

For what it's worth when I do this I base things on one either your gitster/* branches, what I've applied from the ML, or grabbed from the author's own repo, and then usually rebase it on the latest "master" or <topic> as appropriate so I don't have on old base, particularly in the case of gitster/*, you don't rebase those unless needed.

Anyway, I can see why you suggest the "base on master and merge", it has its benefits, but it is in apparent conflict with existing advice added in 76644e3268b (documentation: add tutorial for first contribution, 2019-05-17).

I.e. the "After Review Approval" section (of all places) discusses what to do in that situation. It is narrowly talking about submitting something on top of your own topic, but the advice should be the same in either case I'd think.

Maybe it would be useful to have a CI target that merged to next/master and reported how that went? It would have to be a soft failure, as we might have an easy merge conflict, or someone's pushing a revision to a topic that's already in "next" or "seen" (and might conflict). But having it as an FYI might be helpful.

This seems like a good clarification, but partially unrelated to the $subject, i.e. just the last bullet point is directly relevant, the first two are new general advice. Perhaps split those out into another commit?

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Sorry, but it would defeat the whole point of this suggestion. Letting CI do it to allow contributors not to worry about other people's work is the *last* thing I want to encourage. I want to see people get in the habit of making trial merges and ensuring their work works in the wider context than just "my patches work in 'master'---it is not my problem if it does not work when patches other people already wrote are present".

Not really. The original didn't say in what situations you would want to refer to other commits. Clarifying that made a three-bullet list, and all three are equally relevant.

Thanks.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

I do not mind if you help the effort by updating that document as well to match, but I have a feeling that the first contribution, with a follow-up to fix, is sufficiently different from a more advanced "I need something more than and not yet in 'master' for my new topic" use case.

No, the advice should be totally different between these two cases.

The "fix or enhance what you already sent" is a continuation of the same topic, just you, your reviewers and your maintainer were all not competent enough to catch the shortcomings before the topic hits 'master', and it does make sense to make correction directly on top.

What I am describing here is "I am doing something new, but uses what is not yet in 'master'. The author of that other thing I am depending on may or may not mean to help this new thing when they wrote it, but the important point is that this is not a continuation of their topic. It merely depends on them".

Thanks.

On 23.10.21 22:32, Junio C Hamano wrote:> * A new feature should be based on `master` in general. If the new

Using the topic branches from gitster/git that were merged? Or by selecting the specific commits from the merge into next?

I think this meant to say 'merged to "next|maint|seen"'? Or topics selected for being merged into master?

Thanks, Fabian

Fabian Stelzer <fs@gigacodes.de> writes:

If I were doing this, I would find the tip(s) of things I would depend on out of the output from

  $ git log --oneline --first-parent origin/master..origin/next

This lists a series of merge commits and the second parent of each merge commit is the tip of the topic that was merged to 'next'.

Ah, thanks for catching. I meant "not merged to 'master'" (will fix locally).

Depending on stuff that are already in 'master', unless you are preparing a fix that would also apply to the maintenance track, is rather easy---you can just build on top of 'master'.

And in general, I do not want to see a new topic based on another topic that is not yet in 'next'. If a developer has such a topic, I'd appreciate if the developer waits and shifts their attention to help the other topics they are planning to depend on---and one way to do so is to review these other topics ;-)

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Sorry, should have re-read it more carefully. The original text is saying what I wanted to say, but I misread it ;-)

We earlier in the document said that there are three possible bases; fixes are usually based on 'maint', new features on 'master', but as an exception, if your new feature need to depend on something not yet in 'master', then you start a branch from 'master', merge selected topics into that branch, and use the resulting branch as the 'base' of your topic. We do not want a new topic based on 'next', so preparing such a synthetic base by starting from 'next' and merging topics that are not yet in 'next' is unwelcome.

Perhaps there needs some rewording to clarify that the sentence is referring to that case.

Thanks again.

Junio C Hamano <gitster@pobox.com> writes:

So, I came up with this incremental clarification. I'd wait for other comments before moving the topic further.

 Documentation/SubmittingPatches | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

We encourage identifying what, among many topics on `next`, exact topics a new work depends on, instead of building directly on `next`. Let's clarify this in the documentation.

Developers should know what they are building on top of, and be aware of which part of the system is currently being worked on. Encouraging them to make trial merges to `next` and `seen` themselves will incentivize them to read others' changes and understand them, eventually helping the developers to coordinate among themselves and reviewing each others' changes.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 14 deletions(-)

 * I've been trying to clear the deck, and noticed that this has
   been untended for quite some time.  With some clarification to
   a place I was even confused myself while responding to Fabian's
   comments in the earlier round.

On Fri, Dec 24, 2021 at 5:25 PM Junio C Hamano <gitster@pobox.com> wrote:

I was going to comment that this would be a good place to mention --pretty=reference, but looking at the file in question, that is exactly what the text after this already does.

This patch looks good to me.

On 23.12.2021 15:12, Junio C Hamano wrote:

Just some minor pointers/typos. Looks good.

s/breaks/break

I'm not a native speaker, but `that is what` sounds wrong to me. `feature which you are enhancing` or `feature that you are enhancing` maybe?

Maybe add a hint to `--base=` for format-patch?

Fabian Stelzer <fs@gigacodes.de> writes:

Correct---we are adding multiple tests here.

I am not either, but it indeed does sound strange. "A commit that introduced a feature that you are enhancing" sounds good.

I do not think it belongs to this step. There is a separate section about sending patches that mentions the use of format-patch, where it might be more appropriate.

Thanks.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH v3] SubmittingPatchs: clarify choice of base and testing

We encourage identifying what, among many topics on `next`, exact topics a new work depends on, instead of building directly on `next`. Let's clarify this in the documentation.

Developers should know what they are building on top of, and be aware of which part of the system is currently being worked on. Encouraging them to make trial merges to `next` and `seen` themselves will incentivize them to read others' changes and understand them, eventually helping the developers to coordinate among themselves and reviewing each others' changes.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Range-diff:
1:  2a277ef302 ! 1:  f387f92719 SubmittingPatchs: clarify choice of base and testing
    @@ Documentation/SubmittingPatches: Make sure that you have tests for the bug you a
     -sure that the entire test suite passes.
     +feature does not trigger when it shouldn't.  After any code change,
     +make sure that the entire test suite passes.  When fixing a bug, make
    -+sure you have new tests that breaks if somebody else breaks what you
    ++sure you have new tests that break if somebody else breaks what you
     +fixed by accident to avoid regression.  Also, try merging your work to
     +'next' and 'seen' and make sure the tests still pass; topics by others
     +that are still in flight may have unexpected interactions with what
    @@ Documentation/SubmittingPatches: without external resources. Instead of giving a
     +
     +. A commit that introduced the root cause of a bug you are fixing.
     +
    -+. A commit that introduced a feature that is what you are enhancing.
    ++. A commit that introduced a feature that you are enhancing.
     +
     +. A commit that conflicts with your work when you made a trial merge
     +  of your work into `next` and `seen` for testing.

 Documentation/SubmittingPatches | 53 ++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 14 deletions(-)

On Wed, Oct 20 2021, Taylor Blau wrote:

I'll defer to you, but I've got a mild preference for keeping it out. An upcoming series of patches I'm submitting (I'm waiting on the current leak fixes to land) will fix most of those rev_info leaks. So not having UNLEAK() markings in-flight makes things a bit easier to manage.

On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:

If you don't mind, I think keeping this in (as a way to verify that we really did make t5319 leak-free) may be good for reviewers. When you clean these areas up, you'll have to remember to remove those UNLEAK()s, but hopefully they produce conflicts with your in-flight work that serve as not-too-annoying reminders.

I would certainly rather not have to UNLEAK() those at all, so I am very excited to see a series from you which handles freeing these resources appropriately. It was just too big a bite for me to chew off when preparing this quick series.

Thanks, Taylor

On Fri, Oct 22 2021, Taylor Blau wrote:

Having looked at this a bit more carefully I've upgraded that a bit from "I'll defer to you" to strongly objecting to this specific approach (but read to the end, I think you can have your cake and eat it too).

I just glanced at this series in my previous pass-over, and evidently didn't read the CL to the end. I thought based on the commit subject that it was just a change to some midx code impacting t5319.

But it's a bigger change across the whole test suite than that.

If you run:

    rm .prove; GIT_SKIP_TESTS=t0027 prove -j8 --state=save t[0-9]*.sh :: --immediate

Without this patch (t0027 will take forever), and then:

    GIT_SKIP_TESTS=t0027 prove -j8 --state=failed

You'll see that (I use GNU screen's logging feature to get the output)
we made these tests pass, not just your t5319:
    
    $ grep -w ok screenlog.11 
    t0056-git-C.sh .................................... ok                  
    t1060-object-corruption.sh ........................ ok                  
    t4212-log-corrupt.sh .............................. ok                  
    t4207-log-decoration-colors.sh .................... ok                  
    t5313-pack-bounds-checks.sh ....................... ok                  
    t5506-remote-groups.sh ............................ ok                  
    t5513-fetch-track.sh .............................. ok                  
    t5319-multi-pack-index.sh ......................... ok                  
    t5532-fetch-proxy.sh .............................. ok                  
    t5900-repo-selection.sh ........................... ok                  
    t6011-rev-list-with-bad-commit.sh ................. ok                  
    t6100-rev-list-in-order.sh ........................ ok                  
    t6114-keep-packs.sh ............................... ok                  
    t6131-pathspec-icase.sh ........................... ok                  
    t6003-rev-list-topo-order.sh ...................... ok                  
    t7702-repack-cyclic-alternate.sh .................. ok                  
    t9108-git-svn-glob.sh ............................. ok                  
    t9109-git-svn-multi-glob.sh ....................... ok                  
    t9127-git-svn-partial-rebuild.sh .................. ok                  
    t9133-git-svn-nested-git-repo.sh .................. ok                  
    t9168-git-svn-partially-globbed-names.sh .......... ok                  

I've got subsequent patches on top of what's now in-flight that fix those bigger leaks incrementally, and as part of that add "TEST_PASSES_SANITIZE_LEAK=true" to /all/ the relevant passing tests.

I.e. there isn't a 1=1 correspondance between all current passing tests and "TEST_PASSES_SANITIZE_LEAK=true" markings (some are still left unmarked). But there has been a 1=1 mapping between freshly pasing tests as a result of leak fixes and tests that get that "TEST_PASSES_SANITIZE_LEAK=true" marking.

I think it helps a lot to review those patches & assess their impact if code changes can be paired with relevant test suite changes, which isn't the case if we've got simultaneous UNLEAK() markings for major leaks in-flight.

So the practical effect of that is that I'll need to rebase all that, change my testing setup to test those one-at-at-time with "git rebase -i --exec" before submission (which I do anyway), but now with some selective revert of an earlier UNLEAK() to assert that I'm still fixing the things I expected.

Then either drop the parts of the commit messages that say "this fixes leak XYZ, making tests A, B, C pass" to omit any mention of "A, B, C", or reword it for the earlier now-landed UNLEAK() markings.

These are also just the fully passing tests I ran as a one-off for this E-Mail. For those I do more exhaustive runs where I'll e.g. spot if a test goes from N failing to N-X, and note it if that gets us much closer to 0.

But on the "cake and eat it too" front I also realize that it sucks to not be able to mark tests you made leak-free as passing just because I've got some larger more general leak fixes planned, and even if none of your code leaks, it might just be "git log" or whatever.

But we can just use LSAN_OPTIONS with a suppressions file for that. This diff below (assume my SOB yadayada) makes your tests pass if I eject the 11/11 and replace that with this:

Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally be able to add a "suppressions" file to the $LSAN_OPTIONS.

This allows for marking tests as passing with "TEST_PASSES_SANITIZE_LEAK=true" when they still have failure due more general widespread memory leaks, such as from the "git log" family of commands. We can now mark the "git -C" tests as passing.

For getting specific tests to pass this is preferable to using UNLEAK() in these codepaths, as I'll have fixes for those leaks soon, and being able to atomically mark relevant tests as passing with "TEST_PASSES_SANITIZE_LEAK=true" helps to explain those changes. See [1] for more details.

1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:

[...]

I just went ahead and hacked that up. If you're OK with that approach it would really help reduce the work for leak changes I've got planned, and as noted gives you the end-state of a passing 5319.

I don't know if it makes more sense for you to base on top of this if/when Junio picks it up, or to integrate it into your series etc. Maybe Junio will chime in ...

 t/t0056-git-C.sh        |  4 ++++
 t/test-lib-functions.sh | 42 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

On Fri, Oct 22, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote:

Hmm. This seems neat, but I haven't been able to get it to work without encountering a rather annoying bug along the way.

Here is the preamble of my modified t5319 to include all of the leaks I found in the previous round (but decided not to fix):

TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh todo_leaks <<-\EOF ^add_rev_cmdline$ ^cmd_log_init_finish$ ^prepare_revision_walk$ ^prep_parse_options$ EOF

So running that when git is compiled with SANITIZE=leak *should* work, but instead produces this rather annoying leak detection after t5319.7:

  =================================================================
  ==1785298==ERROR: LeakSanitizer: detected memory leaks

  Indirect leak of 41 byte(s) in 3 object(s) allocated from:
      #0 0x7f2f2f866db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
      #1 0x7f2f2f64b4aa in __GI___strdup string/strdup.c:42

  -----------------------------------------------------
  Suppressions used:
    count      bytes template
        1        576 ^add_rev_cmdline$
  -----------------------------------------------------

  SUMMARY: LeakSanitizer: 41 byte(s) leaked in 3 allocation(s).
  Aborted

Huh? Looking past __GI___strdup (which I assume is just a symbolification failure on ASan's part), who calls strdup()? That trace is annoyingly incomplete, and doesn't really give us anything to go off of.

It does seem to remind me of this:

  https://github.com/google/sanitizers/issues/148

though that issue goes in so many different directions that I'm not sure whether it really is the same issue or not. In any case, that leak *still* shows up even when suppressing xmalloc() and xrealloc(), so I almost think that it's a leak within ASan itself.

But most interesting is that those calls go away when I stop setting `suppressions` in $LSAN_OPTIONS by dropping the call to your todo_leaks.

So this all feels like a bug in ASan to me. I'm curious if it works on your system, but in the meantime I think the best path forward is to drop the last patch of my original series (the one with the three UNLEAK() calls) and to avoid relying on this patch for the time being.

Thanks, Taylor

On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:

Bugs aside, I'd much rather see UNLEAK() annotations than external ones, for all the reasons we introduced UNLEAK() in the first place:

  - it keeps the annotations near the code. Yes, that creates conflicts
    when the code is changed (or the leak is actually fixed), but that's
    a feature. It keeps them from going stale.

  - leak-checkers only know where things are allocated, not who is
    supposed to own them. So you're often annotating the wrong thing;
    it's not a strdup() call which is buggy and leaking, but the
    function five layers up the stack which was supposed to take
    ownership and didn't.

And if we avoid any annotation bugs by doing so, that's icing on the cake. :)

-Peff

On Tue, Oct 26, 2021 at 05:11:29PM -0400, Jeff King wrote:

I agree completely. I noted as much in my message here:

    https://lore.kernel.org/git/YXJAfICQN8s5Gm7s@nand.local/

but Ævar made it sound like his work would be made much easier without the conflict. Since I'm not in any kind of rush to make t5319 leak-free, I figured that queueing the parts of that series that wouldn't conflict with Ævar's ongoing work would be a net-positive.

TBH, I find this much more compelling. Either way, I don't feel strongly enough to deviate from v2 much, and I don't want to create more work for Ævar along the way.

Thanks, Taylor

On Tue, Oct 26, 2021 at 05:30:47PM -0400, Taylor Blau wrote:

Yeah, to be clear, if there's work in progress in an area, then _not_ annotating it (with either method) is perfectly fine with me in the meantime.

-Peff

On Tue, Oct 26 2021, Jeff King wrote:

It seems that I've gotten what I wanted from this side-thread, yay! Thanks both, not to pile on, but just to clarify:

I fully agree with you in general for "good" uses of UNLEAK(). E.g. consider:

    struct strbuf buf = xmalloc(...);
    [...]
    UNLEAK(buf);

If I was fixing leaks in that sort of code what I pointed out downthread in [1] wouldn't apply.

So to clarify, I'm not asking in [1] that UNLEAK() not be used at all while we have in-flight leak fixes. I.e. I'd run into a textual conflict, but that would be trivial to resolve, and obvious what the semantic & textual conflict was.

Rather, it's not marking specific leaks, but UNLEAK() on a container struct that's problematic.

Depending on how they're used those structs may or may not leak. So
e.g. Taylor's upthread 11/11[2] contained:
    
    diff --git a/builtin/rev-list.c b/builtin/rev-list.c
    index 36cb909eba..df3811e763 100644
    --- a/builtin/rev-list.c
    +++ b/builtin/rev-list.c
    @@ -549,6 +549,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
     
            argc = setup_revisions(argc, argv, &revs, &s_r_opt);
     
    +       UNLEAK(revs);
    +
            memset(&info, 0, sizeof(info));
            info.revs = &revs;
            if (revs.bisect)

Which if you on master replace that with:
    
    diff --git a/builtin/rev-list.c b/builtin/rev-list.c
    index 36cb909ebaa..3cce87e0eb7 100644
    --- a/builtin/rev-list.c
    +++ b/builtin/rev-list.c
    @@ -548,6 +548,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
                    revs.do_not_die_on_missing_tree = 1;
     
            argc = setup_revisions(argc, argv, &revs, &s_r_opt);
    +       BUG("using this?");
     
            memset(&info, 0, sizeof(info));
            info.revs = &revs;

You'll run into a failure with:

    GIT_TEST_PASSING_SANITIZE_LEAK=true ./t0002-gitfile.sh

I.e. our existing leak tests already take that codepath and don't run into a leak from using "revs".

So we wouldn't just be marking a known specific leak, but hiding all leaks & non-leaks in container from the top-level, and thus hide potential regressions, an addition to attaining the end-goal of marking some specific test as passing.

Now, that specific example isn't very useful right now, since we don't have a release_revisions() at all, but e.g. the first batch of fixes I've got for the revisions.[ch] leak fix most common cases (e.g. the pending array), but not some obscure ones.

Being able to incrementally mark those leaks as fixed on a test-by-test basis has the advantage over UNLEAK() that we won't have regressions while we're not at a 100% leak free (at which point we could remove the UNLEAK()).

As noted in [3] this case is because the LSAN suppressions list was in play, so we excluded the meaningful part of the stack trace (which is shown in that mail).

Hrm, now that I think about it I think that the cases where I had to resort to valgrind to get around such crappy stacktraces (when that was all I got, even without suppressions) were probably all because there was an UNLEAK() in play...

1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/f1bb8b73ffdb78995dc5653791f9a64adf216e21.1634787555.git.me@ttaylorr.com/
3. https://lore.kernel.org/git/211027.86zgquyk52.gmgdl@evledraar.gmail.com/

On Wed, Oct 27, 2021 at 10:04:17AM +0200, Ævar Arnfjörð Bjarmason wrote:

I'm OK with such a use of UNLEAK(), though of course you could probably just free the buffer in that instance.

But...

...that's the whole point of UNLEAK(), IMHO. You know that the container struct is leaked, but you don't care because the program is about to exit, or there's no easy way to avoid the leak.

So it's not the "container" element, but rather it can be a problem if people annotate too broadly (you will miss some leaks). In the case of rev_info, there is no way to _not_ leak right now, because it has no cleanup function. In the long run we should probably add one. But in the meantime, annotating them so we can find the _interesting_ leaks is worth doing (and that was the whole point of adding UNLEAK in the first place.

And I think the UNLEAK() calls in Taylor's patch are fine in that sense. While yes, _some_ runs of those commands will not leak, the point is to say that when they do leak, they are not interesting. And they are not interesting because that rev_info is in scope until the program exits, so anything it points to is only leaked at a moment where we no longer care.

I'd argue it _is_ a known specific leak. It is rev_info going out of scope that causes the leak, not rev_info holding on to memory in things like its pending array.

Now those can be interesting, too (if it no longer needs the memory, can it perhaps get rid of it). But IMHO all of that is pretty secondary to clearing the noise so you can find "true" leaks (ones where some sub-function really is allocating and then losing the pointer entirely, especially if it's called an arbitrary number of times).

I don't think that's true at all. The annotations you showed in that message, for example, are pointing at add_rev_cmdline(). But that is _not_ the source of the leak, nor where it should be fixed. It is a necessary part of how rev_info works. The leak is when the rev_info goes out of scope without anybody cleaning it up.

I don't see how UNLEAK() would impact stack traces. It should either make something not-leaked-at-all (in which case LSan will no longer mention it), or it does nothing (it throws some wasted memory into a structure which is itself not leaked).

-Peff

Jeff King <peff@peff.net> writes:

True.

I'd prefer to see judicious use of UNLEAK() over LSAN suppressions that lists things that are not the source of the problem. The list being kept in the tests that are far away from the actual source does not help, either.

Thanks.

On Wed, Oct 27 2021, Jeff King wrote:

It doesn't have one, but there are uses of setup_revisions() and rev_info usage that don't leak, as that builtin/rev-list.c case shows.

I mean, in that case it's not doing much of anything, but at least we test that setup_revisions() itself doesn't leak right now, but wouldn't with UNLEAK().

After the current in-flight patches I've got making diff_free() a bit more useful, and then a revisions_release() that'll handle most cases, which gets us started on mass-marking the tests as leak free.

So just FWIW I'm not saying "hey can we hold off on that UNLEAK() for far future xyz", but for a thing I've got queued up that I'd rather not start rewriting...

I don't per-se care about leaks at program exit either, but am making them leak-free as a proxy for making the relevant APIs useful for persistent use.

So I think the historical approach of just sprinkling UNLEAK() since we're exiting anyway is taking too narrow of a view.

In that case if we'd like to see if revisions.[ch] is leak-free we'd need parallel test-helper tests (or whatever) for all its features, or we can just piggy-back on every "git log" invocation in the test suite...

I wasn't trying to get into the philosophical discussion of whether a struct or a struct it contains can be said to be the "real" source of a leak :)

Yeah, I agree that from that POV there's no difference.

I'm merely referring to the practical aspect of patches I've got queued up, which do things like fix all leaks of rev_info.pending, mark tests that now pass, fix another rev_info.whatever, mark tests etc.

For rev_info in particular it's a bit iffy to UNLEAK() it, since it also contains pointers to stuff it doesn't "own", sometimes things are allocated and handed off to it, sometimes for a while, sometimes permanently.

So by UNLEAK()ing it you're not just unleaking "its" memory, but potentially hiding leaks in APIs that need to interact with it in that way. E.g. mailmap.c is one of those cases.

Yes, I think either categorically wrong here, or it applies to some other case I wasn't able to dig up. Or maybe not, doesn't Taylor's example take it from "Direct leak" to "Indirect leak" with the suppression in play? I think those were related somehow (but don't have that in front of me as I type this out).

E.g. (to reinforce your point) try compiling with SANITIZE=leak and running:

    $ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200
    1466000000 -> +0000
    +0200 -> +0000
    
    =================================================================
    ==335188==ERROR: LeakSanitizer: detected memory leaks
    
    Direct leak of 3 byte(s) in 1 object(s) allocated from:
        #0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
        #1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42
    
    SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).

Now try with "SANITIZE=" and valgrind can give you the "real" source:

    $ TZ=UTC valgrind --leak-check=full t/helper/test-tool date show:format:%z 1466000000 +0200
    ==336515== Memcheck, a memory error detector
    ==336515== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
    ==336515== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
    ==336515== Command: t/helper/test-tool date show:format:%z 1466000000 +0200
    ==336515==
    1466000000 -> +0000
    +0200 -> +0000
    ==336515==
    ==336515== HEAP SUMMARY:
    ==336515==     in use at exit: 1,208 bytes in 13 blocks
    ==336515==   total heap usage: 50 allocs, 37 frees, 27,593 bytes allocated
    ==336515==
    ==336515== 3 bytes in 1 blocks are definitely lost in loss record 1 of 13
    ==336515==    at 0x483877F: malloc (vg_replace_malloc.c:307)
    ==336515==    by 0x49C6E4A: strdup (strdup.c:42)
    ==336515==    by 0x261E3A: xstrdup (wrapper.c:29)
    ==336515==    by 0x152785: parse_date_format (date.c:991)
    ==336515==    by 0x116A34: show_dates (test-date.c:39)
    ==336515==    by 0x116DE9: cmd__date (test-date.c:116)
    ==336515==    by 0x115227: cmd_main (test-tool.c:127)
    ==336515==    by 0x115334: main (common-main.c:52)
    ==336515==
    ==336515== LEAK SUMMARY:
    ==336515==    definitely lost: 3 bytes in 1 blocks
    ==336515==    indirectly lost: 0 bytes in 0 blocks
    ==336515==      possibly lost: 0 bytes in 0 blocks
    ==336515==    still reachable: 1,205 bytes in 12 blocks
    ==336515==         suppressed: 0 bytes in 0 blocks
    ==336515== Reachable blocks (those to which a pointer was found) are not shown.
    ==336515== To see them, rerun with: --leak-check=full --show-leak-kinds=all
    ==336515==
    ==336515== For lists of detected and suppressed errors, rerun with: -s
    ==336515== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

I don't know why it habitually loses track of seemingly easy-to-track leaks sometimes, but it does. There isn't an UNLEAK() in play here.

When I run into these I try it with valgrind, fix the leak, then test again with SANITIZE=leak. I haven't found cases where it actually misses things, just cases where the stack traces suck.

On Wed, Oct 27, 2021 at 10:57:52PM +0200, Ævar Arnfjörð Bjarmason wrote:

I don't think that's true. If you UNLEAK() the rev_info in the caller, then it will only affect allocations that are still reachable from rev_info. I.e., things that are by definition not a leak in setup_revisions().

Now you could argue that setup_revisions() is "leaking" by allocating things and stuffing them into rev_info that it should not be. But we can never know that until we have an actual function that cleans up a rev_info, which defines what it's "supposed" to have ownership of.

Maybe we have callers that explicitly try to de-allocate bits of the rev_info. But IMHO that is the source of the whole problem: how is random code using rev_info supposed to know which of its internal details are owned or not? This should be documented and enforced with a single function.

Just to be clear: I am totally fine with dropping Taylor's UNLEAK patches (as I've said already). I was only arguing here that annotating via external files is worse than just adding an UNLEAK().

I'm also trying to combat what I see as mis-conceptions or inaccuracies about what UNLEAK() does or its implications (or even what counts as a "leak"). But I hope in the long run that we don't need _any_ kind of annotation, because we'll actually be leak-free. And then we don't have to care about any of this.

I don't think UNLEAK() can move something from "direct" to "indirect" in LSan's terminology. If rev_info points to an array of structs, and those structs point to allocated strings, then the array itself is a "direct" leak, and the strings are "indirect" (they are leaked, but presumably fixing the direct leak would also deallocate them).

If UNLEAK() makes the array not-leaked, then those indirect leaks don't become direct. They should be transitively not-leaked, too.

So these should be real leaks. Of course with the lousy stack trace it's hard to see what they are. But I don't see how UNLEAK() is responsible for making the lousy stack trace. You could try compiling with LSan but _not_ -DSUPPRESS_ANNOTATED_LEAKS and see if the result is similarly bad (but I expect it to be, since test-date.c does not have any UNLEAK() calls in it).

-Peff

On Fri, Oct 29, 2021 at 04:56:31PM -0400, Jeff King wrote:

I just tested this, and it is unrelated to UNLEAK(). Interestingly, compiling with SANITIZE=address does give the correct stack trace, so I think LSan is just buggy here for some reason.

We usually suppress leaks with ASAN_OPTIONS because there are so many (and we want to get at the more important signal of actual memory errors). But it wouldn't be hard to have that setting respect your PASSING_SANITIZE_LEAK variables. The big downside is that ASan runs of the test suite are much more expensive.

Hmm. A little googling turns up LSan's fast_unwind_on_malloc option. And indeed:

  $ LSAN_OPTIONS=fast_unwind_on_malloc=0 t/helper/test-tool date show:format:%z 1466000000 +0200
  1466000000 -> +0000
  +0200 -> +0000
  
  =================================================================
  ==39628==ERROR: LeakSanitizer: detected memory leaks
  
  Direct leak of 3 byte(s) in 1 object(s) allocated from:
      #0 0x7fa664e39b94 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:56
      #1 0x7fa664c124aa in __GI___strdup string/strdup.c:42
      #2 0x557636c40c2e in xstrdup /home/peff/compile/git/wrapper.c:29
      #3 0x557636b2ae52 in parse_date_format /home/peff/compile/git/date.c:991
      #4 0x557636aedb0e in show_dates t/helper/test-date.c:39
      #5 0x557636aedee1 in cmd__date t/helper/test-date.c:116
      #6 0x557636aec1f0 in cmd_main t/helper/test-tool.c:127
      #7 0x557636aec30d in main /home/peff/compile/git/common-main.c:52
      #8 0x7fa664babe49 in __libc_start_main ../csu/libc-start.c:314
      #9 0x557636aebec9 in _start (/home/peff/compile/git/t/helper/test-tool+0xcec9)
  
  SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).

So perhaps we ought to be setting that by default.

-Peff

On Tue, Oct 26 2021, Taylor Blau wrote:

There are similar cases where LSAN doesn't provide as meaningful of a stacktrace as valgrind, sometimes when tracing leaks I get a relatively bad stacktrace like that ending in some __GI___*<stdlib-name> function. I'll usually compile without SANITIZE=leak and just run a slower:

    valgrind --leak-check=full --show-leak-kinds=all

In this case however it's not a bug or bad leak tracing, but an artifact of us using these stacktrace exclusions.

If you run this manually you'll see:
    
    $ ~/g/git/git -c core.multiPackIndex=false rev-list --objects --all
    cd0747a9352b58d112f0010134351efc7bbad4a6
    [... snipped output ...]
    
    =================================================================
    ==58023==ERROR: LeakSanitizer: detected memory leaks
    
    Direct leak of 576 byte(s) in 1 object(s) allocated from:
        #0 0x7fd0bfae50c5 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:82
        #1 0x5637b3bd9163 in xrealloc /home/avar/g/git/wrapper.c:126
        #2 0x5637b3b6a1e4 in add_rev_cmdline /home/avar/g/git/revision.c:1482
        #3 0x5637b3b6a412 in handle_one_ref /home/avar/g/git/revision.c:1534
        #4 0x5637b3b49c4e in do_for_each_ref_helper /home/avar/g/git/refs.c:1483
        #5 0x5637b3b54afc in do_for_each_repo_ref_iterator refs/iterator.c:418
        #6 0x5637b3b49cc8 in do_for_each_ref /home/avar/g/git/refs.c:1498
        #7 0x5637b3b49d07 in refs_for_each_ref /home/avar/g/git/refs.c:1504
        #8 0x5637b3b6a563 in handle_refs /home/avar/g/git/revision.c:1578
        #9 0x5637b3b6e0e7 in handle_revision_pseudo_opt /home/avar/g/git/revision.c:2597
        #10 0x5637b3b6e9d5 in setup_revisions /home/avar/g/git/revision.c:2738
        #11 0x5637b39ebc58 in cmd_rev_list builtin/rev-list.c:550
        #12 0x5637b3932a89 in run_builtin /home/avar/g/git/git.c:461
        #13 0x5637b3932e98 in handle_builtin /home/avar/g/git/git.c:713
        #14 0x5637b3933105 in run_argv /home/avar/g/git/git.c:780
        #15 0x5637b39335ae in cmd_main /home/avar/g/git/git.c:911
        #16 0x5637b3a1a898 in main /home/avar/g/git/common-main.c:52
        #17 0x7fd0bf860d09 in __libc_start_main ../csu/libc-start.c:308
    
    Indirect leak of 41 byte(s) in 3 object(s) allocated from:
        #0 0x7fd0bfae4db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
        #1 0x7fd0bf8c7e4a in __GI___strdup string/strdup.c:42
    
    SUMMARY: LeakSanitizer: 617 byte(s) leaked in 4 allocation(s).

Which is why I put the "strdup" there, but forgot to tell you when I submitted the real PATCH version that that one shouldn't have the ^$ anchoring.

FWIW this will work on top of your series:
    
    diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
    index a3c72b68f7c..891de720b2c 100755
    --- a/t/t5319-multi-pack-index.sh
    +++ b/t/t5319-multi-pack-index.sh
    @@ -2,6 +2,17 @@
     
     test_description='multi-pack-indexes'
     . ./test-lib.sh
    +todo_leaks <<-\EOF
    +^add_rev_cmdline$
    +^cmd_log_init_finish$
    +^prep_parse_options$
    +^prepare_revision_walk$
    +^start_progress_delay$
    +
    +# An indirect leak reported because we excluded the leaks above
    +strdup$
    +EOF
    +
     
     GIT_TEST_MULTI_PACK_INDEX=0
     objdir=.git/objects

On 10/20/2021 11:39 PM, Taylor Blau wrote:

In my reading, I only found one nitpick that wasn't already caught by other reviewers. Feel free to ignore my comment as it was just something that caused me to pause while reading, but isn't that unusual.

Thanks, -Stolee