From: Jeff King <peff@peff.net>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 0/10] towards clean leak-checker output
Date: Wed, 6 Sep 2017 08:39:43 -0400 [thread overview]
Message-ID: <20170906123943.335degmzmokqrd7p@sigill.intra.peff.net> (raw)
In-Reply-To: <CAN0heSrz=tVHj13X+yHJpAR=5o7j8EqbBdw8hvrhcnhbA04QAA@mail.gmail.com>
On Tue, Sep 05, 2017 at 10:41:51PM +0200, Martin Ågren wrote:
> FWIW, this series (combined with the other series you mentioned) makes
> t0000 and t0001 pass for me with gcc/clang. There are actually some
> leaks in t0000, they're just silently being reported to stderr, since
> the exit statuses from git are hidden by pipes. Maybe you're already
> aware of it. Depending on your definition of "running clean" it might be
> out of scope for this series, which is of course still very interesting
> and enlightening as it stands.
Yeah, I saw those. I don't think they're worth hunting down at this
point. It's quite probable that the same leaks are exercised elsewhere,
and we'll catch them later.
Once the whole test suite runs without reporting any leaks via abort(),
I'll feel more compelled to start grepping stderr for other cases. :)
> One is in cmd_show (you did mention git show) where we leak data in rev.
> The other is some use of strdup. I can't immediately figure out how to
> get a useful stacktrace (you mentioned this as well) and it's past
> bed-time here. I'll try to play more with this tomorrow.
I think I got funny truncated stack traces with just LSAN that went away
with ASAN, but I didn't dig. If you try that, remember that:
- Technically just SANITIZE=address turns on the leak detector, but
you need "leak" in the string to turn on the UNLEAK() magic. So use
"SANITIZE=address,leak".
- You'll have to set ASAN_OPTIONS=abort_on_error=1 in the environment
manually to enable leak detection.
Once the test suite passes with leak detection on, I think we'd probably
do both of those automatically (but until then they make ASAN by itself
unusable).
> Note for self: getting rid of all pipes would probably also help flush
> out a few leaks (or "introduce" them, depending on your viewpoint).
Agreed. These leaks are really just a special form of bug. Those pipes
could be hiding other bugs, too. But again, I'm not too concerned. The
places that pipe git are doing so because they are not meant to be
testing that particular command (and it should generally be covered
elsewhere).
-Peff
next prev parent reply other threads:[~2017-09-06 12:39 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-05 13:01 [PATCH 0/10] towards clean leak-checker output Jeff King
2017-09-05 13:03 ` [PATCH 01/10] test-lib: --valgrind should not override --verbose-log Jeff King
2017-09-05 13:04 ` [PATCH 02/10] test-lib: set LSAN_OPTIONS to abort by default Jeff King
2017-09-05 13:04 ` [PATCH 03/10] add: free leaked pathspec after add_files_to_cache() Jeff King
2017-09-05 13:04 ` [PATCH 04/10] update-index: fix cache entry leak in add_one_file() Jeff King
2017-09-05 13:04 ` [PATCH 05/10] config: plug user_config leak Jeff King
2017-09-05 13:04 ` [PATCH 06/10] reset: make tree counting less confusing Jeff King
2017-09-05 13:04 ` [PATCH 07/10] reset: free allocated tree buffers Jeff King
2017-09-05 13:04 ` [PATCH 08/10] repository: free fields before overwriting them Jeff King
2017-09-05 13:05 ` [PATCH 09/10] set_git_dir: handle feeding gitdir to itself Jeff King
2017-09-07 19:06 ` Brandon Williams
2017-09-05 13:05 ` [PATCH 10/10] add UNLEAK annotation for reducing leak false positives Jeff King
2017-09-05 22:05 ` Stefan Beller
2017-09-07 9:17 ` Jeff King
2017-09-07 20:38 ` Stefan Beller
2017-09-12 14:34 ` Kaartic Sivaraam
2017-09-12 15:05 ` Jeff King
2017-09-13 7:13 ` Kaartic Sivaraam
2017-09-06 17:16 ` Martin Ågren
2017-09-07 9:00 ` Jeff King
2017-09-12 13:41 ` Kaartic Sivaraam
2017-09-12 15:29 ` Jeff King
2017-09-13 6:44 ` Kaartic Sivaraam
2017-09-05 17:50 ` [PATCH 0/10] towards clean leak-checker output Martin Ågren
2017-09-05 19:02 ` Jeff King
2017-09-05 20:41 ` Martin Ågren
2017-09-06 12:39 ` Jeff King [this message]
2017-09-06 1:42 ` Junio C Hamano
2017-09-06 12:28 ` [PATCH 0/2] simplifying !RUNTIME_PREFIX Jeff King
2017-09-06 12:30 ` [PATCH 1/2] system_path: move RUNTIME_PREFIX to a sub-function Jeff King
2017-09-06 13:23 ` Johannes Schindelin
2017-09-06 13:27 ` Jeff King
2017-09-06 12:32 ` [PATCH 2/2] git_extract_argv0_path: do nothing without RUNTIME_PREFIX Jeff King
2017-09-08 6:38 ` [PATCH v2 10/10] add UNLEAK annotation for reducing leak false positives Jeff King
2017-09-19 20:45 ` Jonathan Tan
2017-09-19 21:03 ` Jeff King
2017-09-19 21:34 ` [PATCH for jk/leak-checkers] git-compat-util: make UNLEAK less error-prone Jonathan Tan
2017-09-19 21:46 ` Jeff King
2017-09-19 22:10 ` [PATCH for jk/leak-checkers v2] " Jonathan Tan
2017-09-20 1:45 ` [PATCH v2 10/10] add UNLEAK annotation for reducing leak false positives Junio C Hamano
2017-09-20 2:28 ` Jeff King
2017-09-20 5:12 ` Junio C Hamano
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170906123943.335degmzmokqrd7p@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=martin.agren@gmail.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.
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 NNTP newsgroup(s).