On Fri, Sep 23 2022, Jeff King wrote: > On Fri, Sep 23, 2022 at 10:28:29AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > 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? Not all the time, yes, and I've been getting closer to compiling my daily driver with it.. > 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. Yeah, I know it sucks, but I use it for interactive use, "git push" and the like, so I usually don't care if it's ~2x slower. I even run with SANITIZE=address sometimes. Wanting to have non-O0 there is less about thinking the higher -On helps, and more to avoid it being a special snowflake, i.e. if I run with -O2 -g usually I'd like to just add -fsanitize=leak to that, and not have to also change the optimization level. > 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. I think this (and to your "conter-example" below) is correct on your part. I.e. I don't see a good reason for why this would happen. I have been able to reliably reproduce some leaks as being flaky (and have avoided adding them to the tests). I wonder if that explains it, i.e. there was another underlying issue, and the optimization level happened to trigger some race (or whatever was going on there, I haven't looked in any depth into those either...). >> 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. Probably not the only spot, but the only spot under the TEST_PASSES_SANITIZE_LEAK umbrella. > 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. FWIW I'm not sure it's NORETURN, and haven't had time to dig...
