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.
