Re: [PATCH] tests: opt "git --config-env" test out of SANITIZE=leak

On Thu, Sep 08 2022, Jeff King wrote:

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

quoted

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.

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.

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

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

quoted

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.

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.