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

From: Jeff King <peff@peff.net>
Date: Sep 23, 2022, 21:01 UTC
Message-ID: <Yy4eo6500C0ijhk+@coredump.intra.peff.net>
In-Reply-To: <220923.86k05u4hfd.gmgdl@evledraar.gmail.com>

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

Show 7 quoted lines

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

Show 5 quoted lines

> 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