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: Thu, 8 Sep 2022 14:40:28 -0400	[thread overview]
Message-ID: <Yxo3HIXYDxutU0wF@coredump.intra.peff.net> (raw)
In-Reply-To: <patch-1.1-fb2f0a660c0-20220908T153252Z-avarab@gmail.com>

On Thu, Sep 08, 2022 at 05:42:54PM +0200, Ævar Arnfjörð Bjarmason wrote:

> This is arguably not a compiler issue or bug, as the very notion of a
> variable being "in scope" as far as the leak checker is concerned is
> fuzzy at best in the face of compiler optimizations.
> 
> A similar issue came up before related to the config.c code in
> [2]. There the consensus seemed to be to avoid hacks in the C code to
> work around these compiler edge cases. In this case however skipping
> one test is easy enough. We can deal with these "!SANITIZE_LEAK"
> issues later, when we have fewer overall leaks.

IMHO this is the wrong approach. It is playing whack-a-mole with
individual false positives, when we know the root cause is compiler
optimizations. We should be invoking the compiler in a way that gives us
the most reliable result.

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.

> > Maybe this is a known problem, but it was certainly surprising to me.
> 
> It is surprising, but it's a (at least to me) known "issue". See
> e.g. https://lore.kernel.org/git/patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com/
> for a previous discussion between us. I took the consensus there to be
> that we shouldn't bend over backwards to worry about this.

Thanks, I'd forgotten about that conversation. I stand by my comments
there that we shouldn't be changing the code. But I really think just
using -O0 for the leak-check is a better solution than trying to label
false positives.

> But I see this one slipped through because I locally changed my
> default compiler from "clang" to "gcc" (again) a while ago, and thus
> missed testing clang at higher optimization levels. Usually it's GCC
> that's unhappy about this sort of thing.

FWIW, I found this using clang-14, because the gcc on my system (12.2.0)
does not seem to find actual leaks. It couldn't see any of the ones in
t1060 that I fixed yesterday.

> > But if you run this in a debugger, you'll find that under -O2 the "key"
> > variable has been optimized out! So LSan is producing the wrong result
> > due to the optimization. It doesn't know that "key" is conceptually
> > still reachable.
> > [...]
> 
> I don't know if we (or the compiler implementors) can call it the
> "wrong" result. Just as some warnings only pop up depending on
> optimization levels, this behavior also depends on it.

I think it's different than the case of warnings. In those cases the
optimizations let the compiler see more of what's going on in the code,
which lets them find a warn-able offense that they wouldn't otherwise
see (e.g., inlining a function lets the compiler see an invariant).
Those are real problems in the code that -O0 simply doesn't catch.

But this is different: there is no problem in the code. It is just that
the optimization of removing the variable does not work well with how
the leak-checker works. There is nothing we can do in the code to fix
it; the C conceptual model of variable scoping is all we have to work
with, and according to that, there is no leak. C's "as if" rule means
that the compiler can change the program as long as the behavior is the
same. And it is, from the perspective of the rest of the program. The
issue is that the leak-checker is itself a sort of undefined behavior;
it expects to poke at random parts of the stack at any moment and find a
consistent state, which is not something a normal C program can do.

> I've seen quite a few of these around die() codepaths. Presumably some
> re-arranging/eliminaton of the code around that NORETURN codepath...

The compiler probably does this kind of re-arranging and elimination all
the time. The reason it comes up around die() is that it means we invoke
the leak-checker mid-function. Even though the "key" variable here was
eliminated, the compiler would need to arrange that the pointer value is
_somewhere_ so that when we hit the end of the function we can call that
free(). But when we exit in the middle, we end up doing the leak-check
in a funny state.

It may be that this is influenced by the NORETURN flag. Presumably "key"
is held in a register, and the call to die(), knowing that it does not
need to return, may choose not to preserve that register. So turning
NORETURN into a noop _might_ help, but only because it's giving the
optimizer less information to work with (and it might not if the
optimizer can figure it out on its own via inline functions, for
example). The root cause is the optimizer, so disabling it is the
most complete fix.

-Peff

  reply	other threads:[~2022-09-08 18:42 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 [this message]
2022-09-23  8:28     ` Ævar Arnfjörð Bjarmason
2022-09-23 21:01       ` Jeff King
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=Yxo3HIXYDxutU0wF@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).