git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"
Date: Wed, 27 Oct 2021 05:06:12 -0400	[thread overview]
Message-ID: <YXkWhFGS/uTAnsI1@coredump.intra.peff.net> (raw)
In-Reply-To: <211027.86v91iyis4.gmgdl@evledraar.gmail.com>

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

> >   - 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.
> 
> 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.

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

But...

> 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.

...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.

> 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.

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).

> >   - 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.
> 
> 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).

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.

> 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...

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

  reply	other threads:[~2021-10-27  9:06 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-21  3:39 [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak' Taylor Blau
2021-10-21  3:39 ` [PATCH 01/11] midx.c: clean up chunkfile after reading the MIDX Taylor Blau
2021-10-21  5:50   ` Junio C Hamano
2021-10-21 11:34   ` Ævar Arnfjörð Bjarmason
2021-10-21 16:16   ` Junio C Hamano
2021-10-22  3:04     ` Taylor Blau
2021-10-21  3:39 ` [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file Taylor Blau
2021-10-21  5:00   ` Eric Sunshine
2021-10-21  5:54     ` Junio C Hamano
2021-10-21 16:27   ` Junio C Hamano
2021-10-21  3:39 ` [PATCH 03/11] t/helper/test-read-midx.c: free MIDX within read_midx_file() Taylor Blau
2021-10-21  3:39 ` [PATCH 04/11] builtin/pack-objects.c: don't leak memory via arguments Taylor Blau
2021-10-21  3:39 ` [PATCH 05/11] builtin/repack.c: avoid leaking child arguments Taylor Blau
2021-10-21 13:32   ` Derrick Stolee
2021-10-21 18:47     ` Junio C Hamano
2021-10-21 16:37   ` Junio C Hamano
2021-10-22  3:21     ` Taylor Blau
2021-10-21  3:40 ` [PATCH 06/11] builtin/multi-pack-index.c: don't leak concatenated options Taylor Blau
2021-10-21  3:40 ` [PATCH 07/11] pack-bitmap.c: avoid leaking via midx_bitmap_filename() Taylor Blau
2021-10-21 16:54   ` Junio C Hamano
2021-10-22  4:27     ` Taylor Blau
2021-10-21  3:40 ` [PATCH 08/11] pack-bitmap.c: don't leak type-level bitmaps Taylor Blau
2021-10-21 16:59   ` Junio C Hamano
2021-10-21  3:40 ` [PATCH 09/11] pack-bitmap.c: more aggressively free in free_bitmap_index() Taylor Blau
2021-10-21  5:10   ` Eric Sunshine
2021-10-21 18:32     ` Junio C Hamano
2021-10-22  4:29       ` Taylor Blau
2021-10-21 18:43   ` Junio C Hamano
2021-10-21  3:40 ` [PATCH 10/11] pack-bitmap-write.c: don't return without stop_progress() Taylor Blau
2021-10-21  5:12   ` Eric Sunshine
2021-10-21 11:31   ` Ævar Arnfjörð Bjarmason
2021-10-21 18:39     ` Junio C Hamano
2021-10-22  4:32       ` Taylor Blau
2021-10-23 20:28       ` Junio C Hamano
2021-10-23 20:32         ` SubmittingPatchs: clarify choice of base and testing Junio C Hamano
2021-10-23 20:59           ` Ævar Arnfjörð Bjarmason
2021-10-23 21:31             ` Junio C Hamano
2021-10-23 21:40             ` Junio C Hamano
2021-10-25  8:59           ` Fabian Stelzer
2021-10-25 16:48             ` Junio C Hamano
2021-10-25 16:56               ` Junio C Hamano
2021-10-25 17:00                 ` Junio C Hamano
2021-12-23 23:12           ` [PATCH v2] " Junio C Hamano
2021-12-28 17:47             ` Elijah Newren
2021-12-30 10:20             ` Fabian Stelzer
2021-12-30 20:18               ` Re* " Junio C Hamano
2021-10-21  3:40 ` [PATCH 11/11] t5319: UNLEAK() the remaining leaks Taylor Blau
2021-10-21 11:50 ` [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak' Ævar Arnfjörð Bjarmason
2021-10-22  4:39   ` Taylor Blau
2021-10-22  8:23     ` Ævar Arnfjörð Bjarmason
2021-10-22 10:32       ` [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions" Ævar Arnfjörð Bjarmason
2021-10-26 20:23         ` Taylor Blau
2021-10-26 21:11           ` Jeff King
2021-10-26 21:30             ` Taylor Blau
2021-10-26 21:48               ` Jeff King
2021-10-27  8:04             ` Ævar Arnfjörð Bjarmason
2021-10-27  9:06               ` Jeff King [this message]
2021-10-27 20:21                 ` Junio C Hamano
2021-10-27 20:57                 ` Ævar Arnfjörð Bjarmason
2021-10-29 20:56                   ` Jeff King
2021-10-29 21:05                     ` Jeff King
2021-10-27  7:51           ` Ævar Arnfjörð Bjarmason
2021-10-21 13:37 ` [PATCH 00/11] midx: clean up t5319 under 'SANITIZE=leak' Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YXkWhFGS/uTAnsI1@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).