mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: Taylor Blau <>
Cc:, Derrick Stolee <>,
	Johannes Schindelin <>,
	Junio C Hamano <>,
	Victoria Dye <>
Subject: Re: [PATCH] Makefile: suppress annotated leaks with certain ASan options
Date: Fri, 20 Jan 2023 15:15:10 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Fri, Jan 20, 2023 at 01:46:16PM -0500, Taylor Blau wrote:

> However, it is possible to use the leak sanitizer without
> `SANITIZE=leak`. This happens when building with `SANITIZE=address` and
> enabling the leak sanitizer via the `ASAN_OPTIONS` variable (by
> including the string "detect_leaks=1").
> This renders `UNLEAK()` useless when doing `SANITIZE=address` builds
> which also use the leak checker.

Yeah. I focused on LSan when adding the sanitize/unleak infrastructure
just because it was faster than a full ASan run, which made iterating on
fixes easier. I do think in the long run, once the test suite is leak
free, we may want to support leak-checking via ASan for the simple
reason that it can be done for "free" during the existing ASan
build/test, rather than requiring an extra LSan job.

I do think there's some complexity here, though.

One problem UNLEAK() is that compile-time switch, but whether ASan does
leak detection is a run-time choice. So you are stuck with either:

  - you always turn on UNLEAK() for ASan builds, in which case test runs
    using the default ASAN_OPTIONS we set do the extra work even though
    they are not doing any leak detection. I doubt it's very measurable,
    though (it's just shoving a few bytes onto a linked list),
    especially compared to the overall slowness of ASan.

  - you predicate the build-time choice on ASAN_OPTIONS. But this means

      make SANITIZE=address
      cd t
      ASAN_OPTIONS=detect_leaks=1 ./t0000-*.sh

    will confusingly fail to use UNLEAK().

Your patch does the second one, but I think the first may be the
least-bad option.

The other issue I'd worry about is optimizations. Generally you want to
use -O2 with ASan, because it speeds things up (even more than just
regular -O2, I think, because it is optimizing the ASan instrumentation
code, too). I don't know offhand of any cases where it would find or not
find cases based on optimization level, though I could believe they

But for leak-checking, we've already seen real cases where using LSan
with higher optimization levels can lead to false positives (because the
optimizer drops a value that is still in scope but not used in a code
path that leads to exit()).

So it may be that we really do want to keep leak-checking to "-O0
-fsanitize=leak", and reserve "-O2 -fsanitize=address" for finding
address bugs.

> I found this while playing around with GitHub's ASan-enabled CI builds
> for our internal fork following a merge with v2.38.3.
> The check-chainlint recipe in t/Makefile started using "git diff" via
> d00113ec34 (t/Makefile: apply to existing self-tests,
> 2022-09-01), which triggered a leak in some of GitHub's custom code. I
> was surprised when marking the variable with UNLEAK() didn't do the
> trick, and ended up down this rabbit hole ;-).

Yes, but I'd ask why your ASan builds were checking for leaks in the
first place. There are presumably tons of leaks they'd detect, since the
test suite is far from leak-free.


  parent reply	other threads:[~2023-01-20 20:15 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-20 18:46 [PATCH] Makefile: suppress annotated leaks with certain ASan options Taylor Blau
2023-01-20 19:41 ` Junio C Hamano
2023-01-20 20:15 ` Jeff King [this message]
2023-01-20 20:46   ` Junio C Hamano
2023-01-20 20:55   ` Jeff King

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:

  List information:

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

  git send-email \ \ \ \ \ \ \ \ \

* 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

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