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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Sep 08 2022, Jeff King wrote:

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

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

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