[0/2] Squash leaks in t0000

Message

John Cai via GitGitGadget Sept. 18, 2021, 1:49 p.m. UTC

Carlo points out that t0000 currently doesn't pass with leak-checking
enabled in:
https://public-inbox.org/git/[e-mail]/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71

Here's a series that I've sat on for a while, which adds some UNLEAK's to
"fix" this situation - see the individual patches for a justification of why
an UNLEAK seems appropriate.

ATB, Andrzej

Andrzej Hunt (2):
  log: UNLEAK rev to silence a large number of leaks
  log: UNLEAK original pending objects

 builtin/log.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


base-commit: 186eaaae567db501179c0af0bf89b34cbea02c26
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1092%2Fahunt%2Fleaks-t0000-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1092/ahunt/leaks-t0000-v1
Pull-Request: https://github.com/git/git/pull/1092

Comments

Carlo Marcelo Arenas Belón Sept. 18, 2021, 5:28 p.m. UTC | #1

On Sat, Sep 18, 2021 at 6:49 AM Andrzej Hunt via GitGitGadget
<[e-mail]> wrote:
> Carlo points out that t0000 currently doesn't pass with leak-checking
> enabled in:
> https://public-inbox.org/git/[e-mail]/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71

Did you figure out why it doesn't trigger on maint even if the code
seems to be mostly the same?
At least seems to trigger consistently in master, next and seen.

> Here's a series that I've sat on for a while, which adds some UNLEAK's to
> "fix" this situation - see the individual patches for a justification of why
> an UNLEAK seems appropriate.

While I see that UNLEAK in this specific case, might be an ok "fix", I
have to admit that not finding a repo_clear_revisions() (or equivalent
function) that could be used to clear revs seems like a problem worth
fixing as well for the future.

Will reply with my WIP so we can see if it could work either as an
alternative to this, or at least lay some foundations so that a long
running process that needs to use a `struct revision` or some of this
logic can in the future without having to deal with leaks.

Thanks and "Reviewed-by: Carlo Marcelo Arenas Belón
<[e-mail]>" if needed.

Carlo

Ævar Arnfjörð Bjarmason Sept. 19, 2021, 10:58 a.m. UTC | #2

On Sat, Sep 18 2021, Andrzej Hunt via GitGitGadget wrote:

> Carlo points out that t0000 currently doesn't pass with leak-checking
> enabled in:
> https://public-inbox.org/git/[e-mail]/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71
>
> Here's a series that I've sat on for a while, which adds some UNLEAK's to
> "fix" this situation - see the individual patches for a justification of why
> an UNLEAK seems appropriate.
>
> ATB, Andrzej
>
> Andrzej Hunt (2):
>   log: UNLEAK rev to silence a large number of leaks
>   log: UNLEAK original pending objects
>
>  builtin/log.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

I sent a re-roll of that series[1] that bypasses the issue by no longer
running t0000-basic.sh, so there won't be an immediate need for a fixup
series like this.

As for these patches & approach, I think that these unleak fixes that
just narrowly squash some failure in a specific test aren't worth doing,
and are actually counter-productive.

We should instead eventually fix the leaks more generally and make the
built-ins use those APIs.

Maybe our differing approaches there are because we've got different
end-goals in mind. My end-goal is three-fold:

 A. Make git's core APIs nicer, in most cases that we're not freeing
    memory is a result of a rather messy API that's not quite sure who
    should be managing its memory. This usually makes using it correctly
    harder in other ways.

 B. Make those APIs not leak memory, so we can use them as libraries.

 C. Have regression tests testing [*B*]

Given that, I think that fixing memory leaks in built-in when we're
about to exit is completely pointless as a goal in itself. We're about
to exit anyway, why care that we're leaking memory?

The only reason, I think, is that we're doing it as a proxy to get to a
combination of [*A*] and [*B*] above. Once we know that we can run "git
log" in various modes without it leaking, it's likely that most or all
of the revisions walking API, refname resolution, object lookup
etc. isn't leaking.

I have a WIP branch that would obsolete this[2], see the commit at its
tip. As shown there you're fixing a leak in cmd_show(), but omit the
same leak in its sister functions.

At that point we won't need these UNLEAK(), and as a follow-up any
concerns about spending too much time in a built-in just to clean up
could rather easily be done with something like a GIT_DESTRUCT_LEVEL[3],
i.e. we'd conditionally skip the freeing in some cases.

I'm not saying that there's no point in adding UNLEAK() somewhere, but I
really don't see it in this case. We didn't *need* to mark
t0000-basic.sh as leak-free right away, I just did so because it was the
first test, and I naïvely thought it would stay that way while my series
cooked.

I'd think that when building on top of my SANITIZE=leak series you'd
want instead of UNLEAK() to instead label the test as
TEST_PASSES_SANITIZE_LEAK=true, but just omit some specific breakages
with a use of the "SANITIZE_LEAK" prerequisite.

Maybe there's cases where you'd want to use
TEST_PASSES_SANITIZE_LEAK=true, but the leak is so deep in the guts of
some API that a transitional UNLEAK() is worth it, *and* you can't just
mark some other test that mostly tests the command you're interested in
with TEST_PASSES_SANITIZE_LEAK=true.

But so far I haven't seen such cases, e.g. there's cases where "git tag"
leaks in obscure cases, but not in some common cases with some of my
preliminary fixes. In that state I can usually find a test that uses
"git tag" in some way and mark that as TEST_PASSES_SANITIZE_LEAK=true,
instead of sprinkling UNLEAK() in builtin/tag.c just so I can mark the
main "git tag" test as passing.

1. 62833https://lore.kernel.org/git/[e-mail]/
2. https://github.com/git/git/compare/master...avar:avar/tests-post-add-sanitize-leak-test-mode-fix-leaks
3. https://lore.kernel.org/git/[e-mail]/

Andrzej Hunt Sept. 19, 2021, 3:38 p.m. UTC | #3

On 18/09/2021 19:28, Carlo Arenas wrote:
> 
>> Here's a series that I've sat on for a while, which adds some UNLEAK's to
>> "fix" this situation - see the individual patches for a justification of why
>> an UNLEAK seems appropriate.
> 
> While I see that UNLEAK in this specific case, might be an ok "fix", I
> have to admit that not finding a repo_clear_revisions() (or equivalent
> function) that could be used to clear revs seems like a problem worth
> fixing as well for the future.
> 
> Will reply with my WIP so we can see if it could work either as an
> alternative to this, or at least lay some foundations so that a long
> running process that needs to use a `struct revision` or some of this
> logic can in the future without having to deal with leaks.

Great - in this case I think it's best to ignore my patches since 
actually fixing the leaks is obviously a better solution :) !

ATB,

Andrzej

Junio C Hamano Sept. 20, 2021, 5:55 p.m. UTC | #4

"Andrzej Hunt via GitGitGadget" <[e-mail]> writes:

> Carlo points out that t0000 currently doesn't pass with leak-checking
> enabled in:
> https://public-inbox.org/git/[e-mail]/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71
>
> Here's a series that I've sat on for a while, which adds some UNLEAK's to
> "fix" this situation - see the individual patches for a justification of why
> an UNLEAK seems appropriate.

It seems that discussion on 1/2 seemed to be heading in an
improvement but has petered out?  

I think the simplest fix in these two patches are worth taking, even
if we plan to further improve either by refining the granularity of
UNLEAK application or by introducing repo_clear_revisions() as Carlo
mentions (which is a preferred way to do this if we can manage it),
on top.

Thanks.

Ævar Arnfjörð Bjarmason Sept. 21, 2021, 11:01 p.m. UTC | #5

On Mon, Sep 20 2021, Junio C Hamano wrote:

> "Andrzej Hunt via GitGitGadget" <[e-mail]> writes:
>
>> Carlo points out that t0000 currently doesn't pass with leak-checking
>> enabled in:
>> https://public-inbox.org/git/[e-mail]/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71
>>
>> Here's a series that I've sat on for a while, which adds some UNLEAK's to
>> "fix" this situation - see the individual patches for a justification of why
>> an UNLEAK seems appropriate.
>
> It seems that discussion on 1/2 seemed to be heading in an
> improvement but has petered out?  
>
> I think the simplest fix in these two patches are worth taking, even
> if we plan to further improve either by refining the granularity of
> UNLEAK application or by introducing repo_clear_revisions() as Carlo
> mentions (which is a preferred way to do this if we can manage it),
> on top.

I think per Andrzej's own [1] it's best to not pick up this series.

I've got a lot of memory leak fixes queued up locally, I'm just waiting
on the SANITIZE=leak CI mode to land on master so I can add new tests to
the whitelist as I fix the memory leaks, that includes "real" fixes for
the ones Andrzej's added "UNLEAK()"'s for here.

Hence my meniton of this sort of thing being counter-productive[2],
i.e. I'd need to monkeypatch revert this on top just to make sure I was
still finding leaks that are hidden by these new UNLEAK() (which hide
some really common ones).

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/[e-mail]/