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: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] tests: opt "git --config-env" test out of SANITIZE=leak
Date: Fri, 23 Sep 2022 17:01:23 -0400	[thread overview]
Message-ID: <Yy4eo6500C0ijhk+@coredump.intra.peff.net> (raw)
In-Reply-To: <220923.86k05u4hfd.gmgdl@evledraar.gmail.com>

On Fri, Sep 23, 2022 at 10:28:29AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > In the long run, when all leaks are plugged, we'd want to ditch the
> > whole SANITIZE_LEAK system anyway. So we are better off preventing false
> > positives than trying to gloss over them.
> 
> In the long run when all leaks are plugged I'd prefer to be able to
> compile a git with CFLAGS=-O3 and -fsanitize=leak, and have it run "git
> config" without erroring out about a leak.

Why? Do you want to run a leak-checking copy of Git all the time? If so,
I have bad news for you performance-wise. Running the tests marked as
leak-passing with -O2 (but not -fsanitize=leak) takes ~101s of CPU.
Running with -O0 takes ~111s. Running with -fsanitize=leak takes ~241s.
So the improvement from compiler optimizations is not a big win there,
relatively speaking.

Or are you thinking that -O3 reveals new information we would not find
under other optimization levels? I don't think this is the case. While
that does sometimes find new opportunities for static analysis (via
inlining code, etc), I don't think it helps with run-time analysis. And
as we've seen here, it actively makes things _worse_ by introducing
false positives.

> So I'd really prefer to keep this patch as-is. I'd agree with you if the
> "whack-a-mole" was painful, but in this case it's really not. I think
> it's just this one edge-case in the whole codebase (out of the things
> marked leak-free).

Is it just this one spot? This is already the second one we've discussed
on the list, and I think you indicated there were more spots where you
intentionally held back on setting TEST_PASSES_SANITIZE_LEAK when you
saw hits under higher optimization levels.

It really is a potential problem anywhere we'd call a NORETURN function,
because the compiler (rightly) realizes there is no point in making sure
we can call a later free() that we'll never reach.

> That's inching us closer to what I think should be the end goal,
> i.e. this SANITIZE=leak is just a transition mechanism, so having it
> enforce -O0 would mean that we can't have -fsanitize=leak Just Work in
> the future (both for the tests & when manually compiled).

I think TEST_PASSES_SANITIZE_LEAK is a transition mechanism. But we'll
always want SANITIZE=leak, and I don't think it makes sense for ordinary
builds to use it.

> This is just from vague recollection, and I couldn't find an example now
> with some light digging (which was just seeing if any of the tests with
> a "!SANITIZE_LEAK" prereq passed on -O0, but not -O3 with clang), but I
> think I've run into cases where higher optimization levels have also
> spotted genuine leaks that I've fixed.

If you have a counter-example, I'm all ears, but I have trouble
imagining how -O3 could actually find a leak not present in -O0. Again,
because we're testing runtime behavior, and the optimizations should not
be changing the visible behavior of our program (and the problem is the
leak checker itself is testing "invisible" things that may not actually
be important, like the state of a variable on the stack when we call
exit()).

-Peff

  reply	other threads:[~2022-09-23 21:03 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-08  5:29 LSan curiosity in t1300 Jeff King
2022-09-08 15:42 ` [PATCH] tests: opt "git --config-env" test out of SANITIZE=leak Ævar Arnfjörð Bjarmason
2022-09-08 18:40   ` Jeff King
2022-09-23  8:28     ` Ævar Arnfjörð Bjarmason
2022-09-23 21:01       ` Jeff King [this message]
2022-09-26  8:56         ` Ævar Arnfjörð Bjarmason
2022-09-08 18:03 ` LSan curiosity in t1300 Junio C Hamano
2022-09-08 18:15   ` 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:
  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=Yy4eo6500C0ijhk+@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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).