{"thread":{"id":"58407","subject":"LSan curiosity in t1300","startedAt":"2022-09-08T05:30:05Z","lastAt":"2022-09-26T09:08:12Z","messageCount":8,"participants":["Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"462797","messageId":"Yxl91jfycCo7O7Pp@coredump.intra.peff.net","threadId":"58407","inReplyTo":null,"subject":"LSan curiosity in t1300","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T05:29:58Z","receivedAt":"2022-09-08T05:30:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If I run:\n\n  make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true CFLAGS=-O0\n\nlocally, then all tests pass or are skipped. But if switch to -O2, then\nt1300 starts failing!\n\nThe command that supposedly leaks is this:\n\n  git --config-env=foo.flag= config --bool foo.flag\n\nand the backtrace looks like:\n\n  Direct leak of 9 byte(s) in 1 object(s) allocated from:\n      #0 0x56195ae6dc62 in __interceptor_malloc (git+0x78c62) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n      #1 0x56195b0ad6b6 in do_xmalloc wrapper.c\n      #2 0x56195b0ad7bd in xmemdupz (git+0x2b87bd) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n      #3 0x56195af6f8d0 in git_config_push_env (git+0x17a8d0) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n      #4 0x56195ae71689 in handle_options git.c\n      #5 0x56195ae7060f in cmd_main (git+0x7b60f) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n      #6 0x56195af370c6 in main (git+0x1420c6) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n      #7 0x7f45b9029209 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n      #8 0x7f45b90292bb in __libc_start_main csu/../csu/libc-start.c:389:3\n      #9 0x56195ae420c0 in _start (git+0x4d0c0) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n\nBut here's the weird part. The function looks like this:\n\n  void git_config_push_env(const char *spec)\n  {\n          char *key;\n          const char *env_name;\n          const char *env_value;\n  \n          env_name = strrchr(spec, '=');\n          if (!env_name)\n                  die(_(\"invalid config format: %s\"), spec);\n          key = xmemdupz(spec, env_name - spec);\n          env_name++;\n          if (!*env_name)\n                  die(_(\"missing environment variable name for configuration '%.*s'\"),\n                      (int)(env_name - spec - 1), spec);\n  \n          env_value = getenv(env_name);\n          if (!env_value)\n                  die(_(\"missing environment variable '%s' for configuration '%.*s'\"),\n                      env_name, (int)(env_name - spec - 1), spec);\n  \n          git_config_push_split_parameter(key, env_value);\n          free(key);\n  }\n\nAnd it is complaining that we've leaked \"key\". But as you can see, we\nalways free it. The problem is that for this particular invocation we\ndie(\"missing environment variable name\"), so of course we don't make it\nto the free(). Normally this is OK, though. The \"key\" variable is still\non the stack, so the leak-checker should realize that it's still\nreachable.\n\nBut if you run this in a debugger, you'll find that under -O2 the \"key\"\nvariable has been optimized out! So LSan is producing the wrong result\ndue to the optimization. It doesn't know that \"key\" is conceptually\nstill reachable.\n\nMaybe this is a known problem, but it was certainly surprising to me.\nI'm not sure if we should do anything about it or not. It doesn't seem\nto trigger in CI, even though I don't see us taking any steps there to\nuse -O0 or similar. So we can perhaps ignore it for now, and this\nmessage can serve as a warning. But if we think LSan isn't reliable\nunder higher optimizations, we could perhaps teach the Makefile to\nprefer -O0 when it sees SANITIZE=leak.\n\n-Peff\n"},{"id":"462809","messageId":"patch-1.1-fb2f0a660c0-20220908T153252Z-avarab@gmail.com","threadId":"58407","inReplyTo":"Yxl91jfycCo7O7Pp@coredump.intra.peff.net","subject":"[PATCH] tests: opt \"git --config-env\" test out of SANITIZE=leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-08T15:42:54Z","receivedAt":"2022-09-08T15:43:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In 676063016ad (leak tests: mark some config tests as passing with\nSANITIZE=leak, 2021-10-31) t1300*.sh as a whole was marked as passing\nwith SANITIZE=leak, but as reported[1] certain compiler options and\noptimization levels will re-arrange the generated code to have the\nsanitizer fire.\n\nThis issue is reproducible with e.g. clang v12 with CFLAGS=-O2. The\nissue is that -fsanitize=leak will combine with other optimization\noptions, and as we eliminate functions, variables etc. the\n-fsanitize=leak criteria for considering something a leak might\neffectively change.\n\nThis is arguably not a compiler issue or bug, as the very notion of a\nvariable being \"in scope\" as far as the leak checker is concerned is\nfuzzy at best in the face of compiler optimizations.\n\nA similar issue came up before related to the config.c code in\n[2]. There the consensus seemed to be to avoid hacks in the C code to\nwork around these compiler edge cases. In this case however skipping\none test is easy enough. We can deal with these \"!SANITIZE_LEAK\"\nissues later, when we have fewer overall leaks.\n\n1. https://lore.kernel.org/git/Yxl91jfycCo7O7Pp@coredump.intra.peff.net/\n2. https://lore.kernel.org/git/patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com/\n---\n\nOn Thu, Sep 08 2022, Jeff King wrote:\n\n> If I run:\n>\n>   make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true CFLAGS=-O0\n>\n> locally, then all tests pass or are skipped. But if switch to -O2, then\n> t1300 starts failing!\n>\n> The command that supposedly leaks is this:\n>\n>   git --config-env=foo.flag= config --bool foo.flag\n>\n> and the backtrace looks like:\n>\n>   Direct leak of 9 byte(s) in 1 object(s) allocated from:\n>       #0 0x56195ae6dc62 in __interceptor_malloc (git+0x78c62) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n>       #1 0x56195b0ad6b6 in do_xmalloc wrapper.c\n>       #2 0x56195b0ad7bd in xmemdupz (git+0x2b87bd) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n>       #3 0x56195af6f8d0 in git_config_push_env (git+0x17a8d0) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n>       #4 0x56195ae71689 in handle_options git.c\n>       #5 0x56195ae7060f in cmd_main (git+0x7b60f) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n>       #6 0x56195af370c6 in main (git+0x1420c6) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n>       #7 0x7f45b9029209 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n>       #8 0x7f45b90292bb in __libc_start_main csu/../csu/libc-start.c:389:3\n>       #9 0x56195ae420c0 in _start (git+0x4d0c0) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)\n>\n> But here's the weird part. The function looks like this:\n>\n>   void git_config_push_env(const char *spec)\n>   {\n>           char *key;\n>           const char *env_name;\n>           const char *env_value;\n>   \n>           env_name = strrchr(spec, '=');\n>           if (!env_name)\n>                   die(_(\"invalid config format: %s\"), spec);\n>           key = xmemdupz(spec, env_name - spec);\n>           env_name++;\n>           if (!*env_name)\n>                   die(_(\"missing environment variable name for configuration '%.*s'\"),\n>                       (int)(env_name - spec - 1), spec);\n>   \n>           env_value = getenv(env_name);\n>           if (!env_value)\n>                   die(_(\"missing environment variable '%s' for configuration '%.*s'\"),\n>                       env_name, (int)(env_name - spec - 1), spec);\n>   \n>           git_config_push_split_parameter(key, env_value);\n>           free(key);\n>   }\n>\n> And it is complaining that we've leaked \"key\". But as you can see, we\n> always free it. The problem is that for this particular invocation we\n> die(\"missing environment variable name\"), so of course we don't make it\n> to the free(). Normally this is OK, though. The \"key\" variable is still\n> on the stack, so the leak-checker should realize that it's still\n> reachable.\n\n[Re-arranged your E-Mail a bit for the purposes of the flow of this\nreply]\n\n> Maybe this is a known problem, but it was certainly surprising to me.\n\nIt is surprising, but it's a (at least to me) known \"issue\". See\ne.g. https://lore.kernel.org/git/patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com/\nfor a previous discussion between us. I took the consensus there to be\nthat we shouldn't bend over backwards to worry about this.\n\nI have been diligently ensuring that I don't mark tests as leak-free\nif they \"pass\", but then \"fail\" on higher optimization levels.\n\nBut I see this one slipped through because I locally changed my\ndefault compiler from \"clang\" to \"gcc\" (again) a while ago, and thus\nmissed testing clang at higher optimization levels. Usually it's GCC\nthat's unhappy about this sort of thing.\n\n> But if you run this in a debugger, you'll find that under -O2 the \"key\"\n> variable has been optimized out! So LSan is producing the wrong result\n> due to the optimization. It doesn't know that \"key\" is conceptually\n> still reachable.\n> [...]\n\nI don't know if we (or the compiler implementors) can call it the\n\"wrong\" result. Just as some warnings only pop up depending on\noptimization levels, this behavior also depends on it.\n\nI've seen quite a few of these around die() codepaths. Presumably some\nre-arranging/eliminaton of the code around that NORETURN codepath...\n\n> [...]\n> I'm not sure if we should do anything about it or not. It doesn't seem\n> to trigger in CI, even though I don't see us taking any steps there to\n> use -O0 or similar. So we can perhaps ignore it for now, and this\n> message can serve as a warning. But if we think LSan isn't reliable\n> under higher optimizations, we could perhaps teach the Makefile to\n> prefer -O0 when it sees SANITIZE=leak.\n\nI think it's good to not force -O0 with SANITIZE=leak, but this is\nannoying, so let's do the below change. With it the tests pass for me\nwith clang under -O2/-O3.\n\nFWIW if you play with the \"check\" mode you'll probably also find that\nwe have some tests that are \"flaky\" as far as the leak checking is\nconcerned (\"t2012 t6415 t6435\", I think, at least I skip those locally\nwhen using it). IIRC some of that also depends on -On.\n\n t/t1300-config.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex c6661e61af5..a4953881db6 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1398,7 +1398,7 @@ test_expect_success 'git --config-env with missing value' '\n \tgrep \"invalid config format: config\" error\n '\n \n-test_expect_success 'git --config-env fails with invalid parameters' '\n+test_expect_success !SANITIZE_LEAK 'git --config-env fails with invalid parameters' '\n \ttest_must_fail git --config-env=foo.flag config --bool foo.flag 2>error &&\n \ttest_i18ngrep \"invalid config format: foo.flag\" error &&\n \ttest_must_fail git --config-env=foo.flag= config --bool foo.flag 2>error &&\n-- \n2.37.3.1492.gbb5af15b83b\n\n"},{"id":"462818","messageId":"xmqq8rmtrbfq.fsf@gitster.g","threadId":"58407","inReplyTo":"Yxl91jfycCo7O7Pp@coredump.intra.peff.net","subject":"Re: LSan curiosity in t1300","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-08T18:03:05Z","receivedAt":"2022-09-08T18:03:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> And it is complaining that we've leaked \"key\". But as you can see, we\n> always free it. The problem is that for this particular invocation we\n> die(\"missing environment variable name\"), so of course we don't make it\n> to the free(). Normally this is OK, though. The \"key\" variable is still\n> on the stack, so the leak-checker should realize that it's still\n> reachable.\n>\n> But if you run this in a debugger, you'll find that under -O2 the \"key\"\n> variable has been optimized out! So LSan is producing the wrong result\n> due to the optimization. It doesn't know that \"key\" is conceptually\n> still reachable.\n\nYikes.  \n\nIs that related to the cause of the other patch from you, i.e.\nwanting to get \"registers\" but unable to?  If they manage to get the\nvalue of the register that holds 'key', would this disappear, I\nwonder?\n\n> Maybe this is a known problem, but it was certainly surprising to me.\n> I'm not sure if we should do anything about it or not. It doesn't seem\n> to trigger in CI, even though I don't see us taking any steps there to\n> use -O0 or similar. So we can perhaps ignore it for now, and this\n> message can serve as a warning. But if we think LSan isn't reliable\n> under higher optimizations, we could perhaps teach the Makefile to\n> prefer -O0 when it sees SANITIZE=leak.\n\nNice note to leave here, I agree.\n\n"},{"id":"462820","messageId":"YxoxRus2NjsYmM63@coredump.intra.peff.net","threadId":"58407","inReplyTo":"xmqq8rmtrbfq.fsf@gitster.g","subject":"Re: LSan curiosity in t1300","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T18:15:34Z","receivedAt":"2022-09-08T18:15:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2022 at 11:03:05AM -0700, Junio C Hamano wrote:\n\n> > But if you run this in a debugger, you'll find that under -O2 the \"key\"\n> > variable has been optimized out! So LSan is producing the wrong result\n> > due to the optimization. It doesn't know that \"key\" is conceptually\n> > still reachable.\n> \n> Yikes.  \n> \n> Is that related to the cause of the other patch from you, i.e.\n> wanting to get \"registers\" but unable to?  If they manage to get the\n> value of the register that holds 'key', would this disappear, I\n> wonder?\n\nI don't think so. t1300 fails consistently for me with -O2, and it\ndoesn't have the \"register\" message (and never came up as one of the\nracy losers). I think the \"register\" thing is mostly just a harmless\nwarning that doesn't change the output, but simply confuses our \"did\nlsan produce any logs\" check.\n\n-Peff\n"},{"id":"462822","messageId":"Yxo3HIXYDxutU0wF@coredump.intra.peff.net","threadId":"58407","inReplyTo":"patch-1.1-fb2f0a660c0-20220908T153252Z-avarab@gmail.com","subject":"Re: [PATCH] tests: opt \"git --config-env\" test out of SANITIZE=leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T18:40:28Z","receivedAt":"2022-09-08T18:40:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2022 at 05:42:54PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This is arguably not a compiler issue or bug, as the very notion of a\n> variable being \"in scope\" as far as the leak checker is concerned is\n> fuzzy at best in the face of compiler optimizations.\n> \n> A similar issue came up before related to the config.c code in\n> [2]. There the consensus seemed to be to avoid hacks in the C code to\n> work around these compiler edge cases. In this case however skipping\n> one test is easy enough. We can deal with these \"!SANITIZE_LEAK\"\n> issues later, when we have fewer overall leaks.\n\nIMHO this is the wrong approach. It is playing whack-a-mole with\nindividual false positives, when we know the root cause is compiler\noptimizations. We should be invoking the compiler in a way that gives us\nthe most reliable result.\n\nIn the long run, when all leaks are plugged, we'd want to ditch the\nwhole SANITIZE_LEAK system anyway. So we are better off preventing false\npositives than trying to gloss over them.\n\n> > Maybe this is a known problem, but it was certainly surprising to me.\n> \n> It is surprising, but it's a (at least to me) known \"issue\". See\n> e.g. https://lore.kernel.org/git/patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com/\n> for a previous discussion between us. I took the consensus there to be\n> that we shouldn't bend over backwards to worry about this.\n\nThanks, I'd forgotten about that conversation. I stand by my comments\nthere that we shouldn't be changing the code. But I really think just\nusing -O0 for the leak-check is a better solution than trying to label\nfalse positives.\n\n> But I see this one slipped through because I locally changed my\n> default compiler from \"clang\" to \"gcc\" (again) a while ago, and thus\n> missed testing clang at higher optimization levels. Usually it's GCC\n> that's unhappy about this sort of thing.\n\nFWIW, I found this using clang-14, because the gcc on my system (12.2.0)\ndoes not seem to find actual leaks. It couldn't see any of the ones in\nt1060 that I fixed yesterday.\n\n> > But if you run this in a debugger, you'll find that under -O2 the \"key\"\n> > variable has been optimized out! So LSan is producing the wrong result\n> > due to the optimization. It doesn't know that \"key\" is conceptually\n> > still reachable.\n> > [...]\n> \n> I don't know if we (or the compiler implementors) can call it the\n> \"wrong\" result. Just as some warnings only pop up depending on\n> optimization levels, this behavior also depends on it.\n\nI think it's different than the case of warnings. In those cases the\noptimizations let the compiler see more of what's going on in the code,\nwhich lets them find a warn-able offense that they wouldn't otherwise\nsee (e.g., inlining a function lets the compiler see an invariant).\nThose are real problems in the code that -O0 simply doesn't catch.\n\nBut this is different: there is no problem in the code. It is just that\nthe optimization of removing the variable does not work well with how\nthe leak-checker works. There is nothing we can do in the code to fix\nit; the C conceptual model of variable scoping is all we have to work\nwith, and according to that, there is no leak. C's \"as if\" rule means\nthat the compiler can change the program as long as the behavior is the\nsame. And it is, from the perspective of the rest of the program. The\nissue is that the leak-checker is itself a sort of undefined behavior;\nit expects to poke at random parts of the stack at any moment and find a\nconsistent state, which is not something a normal C program can do.\n\n> I've seen quite a few of these around die() codepaths. Presumably some\n> re-arranging/eliminaton of the code around that NORETURN codepath...\n\nThe compiler probably does this kind of re-arranging and elimination all\nthe time. The reason it comes up around die() is that it means we invoke\nthe leak-checker mid-function. Even though the \"key\" variable here was\neliminated, the compiler would need to arrange that the pointer value is\n_somewhere_ so that when we hit the end of the function we can call that\nfree(). But when we exit in the middle, we end up doing the leak-check\nin a funny state.\n\nIt may be that this is influenced by the NORETURN flag. Presumably \"key\"\nis held in a register, and the call to die(), knowing that it does not\nneed to return, may choose not to preserve that register. So turning\nNORETURN into a noop _might_ help, but only because it's giving the\noptimizer less information to work with (and it might not if the\noptimizer can figure it out on its own via inline functions, for\nexample). The root cause is the optimizer, so disabling it is the\nmost complete fix.\n\n-Peff\n"},{"id":"463505","messageId":"220923.86k05u4hfd.gmgdl@evledraar.gmail.com","threadId":"58407","inReplyTo":"Yxo3HIXYDxutU0wF@coredump.intra.peff.net","subject":"Re: [PATCH] tests: opt \"git --config-env\" test out of SANITIZE=leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-23T08:28:29Z","receivedAt":"2022-09-23T08:35:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 08 2022, Jeff King wrote:\n\n> On Thu, Sep 08, 2022 at 05:42:54PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This is arguably not a compiler issue or bug, as the very notion of a\n>> variable being \"in scope\" as far as the leak checker is concerned is\n>> fuzzy at best in the face of compiler optimizations.\n>> \n>> A similar issue came up before related to the config.c code in\n>> [2]. There the consensus seemed to be to avoid hacks in the C code to\n>> work around these compiler edge cases. In this case however skipping\n>> one test is easy enough. We can deal with these \"!SANITIZE_LEAK\"\n>> issues later, when we have fewer overall leaks.\n>\n> IMHO this is the wrong approach. It is playing whack-a-mole with\n> individual false positives, when we know the root cause is compiler\n> optimizations. We should be invoking the compiler in a way that gives us\n> the most reliable result.\n>\n> In the long run, when all leaks are plugged, we'd want to ditch the\n> whole SANITIZE_LEAK system anyway. So we are better off preventing false\n> positives than trying to gloss over them.\n\nIn the long run when all leaks are plugged I'd prefer to be able to\ncompile a git with CFLAGS=-O3 and -fsanitize=leak, and have it run \"git\nconfig\" without erroring out about a leak.\n\nSo I'd really prefer to keep this patch as-is. I'd agree with you if the\n\"whack-a-mole\" was painful, but in this case it's really not. I think\nit's just this one edge-case in the whole codebase (out of the things\nmarked leak-free).\n\nThat's inching us closer to what I think should be the end goal,\ni.e. this SANITIZE=leak is just a transition mechanism, so having it\nenforce -O0 would mean that we can't have -fsanitize=leak Just Work in\nthe future (both for the tests & when manually compiled).\n\n>> I don't know if we (or the compiler implementors) can call it the\n>> \"wrong\" result. Just as some warnings only pop up depending on\n>> optimization levels, this behavior also depends on it.\n>\n> I think it's different than the case of warnings. In those cases the\n> optimizations let the compiler see more of what's going on in the code,\n> which lets them find a warn-able offense that they wouldn't otherwise\n> see (e.g., inlining a function lets the compiler see an invariant).\n> Those are real problems in the code that -O0 simply doesn't catch.\n>\n> But this is different: there is no problem in the code. It is just that\n> the optimization of removing the variable does not work well with how\n> the leak-checker works. There is nothing we can do in the code to fix\n> it; the C conceptual model of variable scoping is all we have to work\n> with, and according to that, there is no leak. C's \"as if\" rule means\n> that the compiler can change the program as long as the behavior is the\n> same. And it is, from the perspective of the rest of the program. The\n> issue is that the leak-checker is itself a sort of undefined behavior;\n> it expects to poke at random parts of the stack at any moment and find a\n> consistent state, which is not something a normal C program can do.\n\nThis is just from vague recollection, and I couldn't find an example now\nwith some light digging (which was just seeing if any of the tests with\na \"!SANITIZE_LEAK\" prereq passed on -O0, but not -O3 with clang), but I\nthink I've run into cases where higher optimization levels have also\nspotted genuine leaks that I've fixed.\n\n"},{"id":"463551","messageId":"Yy4eo6500C0ijhk+@coredump.intra.peff.net","threadId":"58407","inReplyTo":"220923.86k05u4hfd.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] tests: opt \"git --config-env\" test out of SANITIZE=leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-23T21:01:23Z","receivedAt":"2022-09-23T21:02:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 23, 2022 at 10:28:29AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > In the long run, when all leaks are plugged, we'd want to ditch the\n> > whole SANITIZE_LEAK system anyway. So we are better off preventing false\n> > positives than trying to gloss over them.\n> \n> In the long run when all leaks are plugged I'd prefer to be able to\n> compile a git with CFLAGS=-O3 and -fsanitize=leak, and have it run \"git\n> config\" without erroring out about a leak.\n\nWhy? Do you want to run a leak-checking copy of Git all the time? If so,\nI have bad news for you performance-wise. Running the tests marked as\nleak-passing with -O2 (but not -fsanitize=leak) takes ~101s of CPU.\nRunning with -O0 takes ~111s. Running with -fsanitize=leak takes ~241s.\nSo the improvement from compiler optimizations is not a big win there,\nrelatively speaking.\n\nOr are you thinking that -O3 reveals new information we would not find\nunder other optimization levels? I don't think this is the case. While\nthat does sometimes find new opportunities for static analysis (via\ninlining code, etc), I don't think it helps with run-time analysis. And\nas we've seen here, it actively makes things _worse_ by introducing\nfalse positives.\n\n> So I'd really prefer to keep this patch as-is. I'd agree with you if the\n> \"whack-a-mole\" was painful, but in this case it's really not. I think\n> it's just this one edge-case in the whole codebase (out of the things\n> marked leak-free).\n\nIs it just this one spot? This is already the second one we've discussed\non the list, and I think you indicated there were more spots where you\nintentionally held back on setting TEST_PASSES_SANITIZE_LEAK when you\nsaw hits under higher optimization levels.\n\nIt really is a potential problem anywhere we'd call a NORETURN function,\nbecause the compiler (rightly) realizes there is no point in making sure\nwe can call a later free() that we'll never reach.\n\n> That's inching us closer to what I think should be the end goal,\n> i.e. this SANITIZE=leak is just a transition mechanism, so having it\n> enforce -O0 would mean that we can't have -fsanitize=leak Just Work in\n> the future (both for the tests & when manually compiled).\n\nI think TEST_PASSES_SANITIZE_LEAK is a transition mechanism. But we'll\nalways want SANITIZE=leak, and I don't think it makes sense for ordinary\nbuilds to use it.\n\n> This is just from vague recollection, and I couldn't find an example now\n> with some light digging (which was just seeing if any of the tests with\n> a \"!SANITIZE_LEAK\" prereq passed on -O0, but not -O3 with clang), but I\n> think I've run into cases where higher optimization levels have also\n> spotted genuine leaks that I've fixed.\n\nIf you have a counter-example, I'm all ears, but I have trouble\nimagining how -O3 could actually find a leak not present in -O0. Again,\nbecause we're testing runtime behavior, and the optimizations should not\nbe changing the visible behavior of our program (and the problem is the\nleak checker itself is testing \"invisible\" things that may not actually\nbe important, like the state of a variable on the stack when we call\nexit()).\n\n-Peff\n"},{"id":"463611","messageId":"220926.86tu4u33m2.gmgdl@evledraar.gmail.com","threadId":"58407","inReplyTo":"Yy4eo6500C0ijhk+@coredump.intra.peff.net","subject":"Re: [PATCH] tests: opt \"git --config-env\" test out of SANITIZE=leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-26T08:56:45Z","receivedAt":"2022-09-26T09:08:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 23 2022, Jeff King wrote:\n\n> On Fri, Sep 23, 2022 at 10:28:29AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > In the long run, when all leaks are plugged, we'd want to ditch the\n>> > whole SANITIZE_LEAK system anyway. So we are better off preventing false\n>> > positives than trying to gloss over them.\n>> \n>> In the long run when all leaks are plugged I'd prefer to be able to\n>> compile a git with CFLAGS=-O3 and -fsanitize=leak, and have it run \"git\n>> config\" without erroring out about a leak.\n>\n> Why? Do you want to run a leak-checking copy of Git all the time? \n\nNot all the time, yes, and I've been getting closer to compiling my\ndaily driver with it..\n\n> If so,\n> I have bad news for you performance-wise. Running the tests marked as\n> leak-passing with -O2 (but not -fsanitize=leak) takes ~101s of CPU.\n> Running with -O0 takes ~111s. Running with -fsanitize=leak takes ~241s.\n> So the improvement from compiler optimizations is not a big win there,\n> relatively speaking.\n\nYeah, I know it sucks, but I use it for interactive use, \"git push\" and\nthe like, so I usually don't care if it's ~2x slower. I even run with\nSANITIZE=address sometimes.\n\nWanting to have non-O0 there is less about thinking the higher -On\nhelps, and more to avoid it being a special snowflake, i.e. if I run\nwith -O2 -g usually I'd like to just add -fsanitize=leak to that, and\nnot have to also change the optimization level.\n\n> Or are you thinking that -O3 reveals new information we would not find\n> under other optimization levels? I don't think this is the case. While\n> that does sometimes find new opportunities for static analysis (via\n> inlining code, etc), I don't think it helps with run-time analysis. And\n> as we've seen here, it actively makes things _worse_ by introducing\n> false positives.\n\nI think this (and to your \"conter-example\" below) is correct on your\npart. I.e. I don't see a good reason for why this would happen.\n\nI have been able to reliably reproduce some leaks as being flaky (and\nhave avoided adding them to the tests). I wonder if that explains it,\ni.e. there was another underlying issue, and the optimization level\nhappened to trigger some race (or whatever was going on there, I haven't\nlooked in any depth into those either...).\n\n>> So I'd really prefer to keep this patch as-is. I'd agree with you if the\n>> \"whack-a-mole\" was painful, but in this case it's really not. I think\n>> it's just this one edge-case in the whole codebase (out of the things\n>> marked leak-free).\n>\n> Is it just this one spot? This is already the second one we've discussed\n> on the list, and I think you indicated there were more spots where you\n> intentionally held back on setting TEST_PASSES_SANITIZE_LEAK when you\n> saw hits under higher optimization levels.\n\nProbably not the only spot, but the only spot under the\nTEST_PASSES_SANITIZE_LEAK umbrella.\n\n> It really is a potential problem anywhere we'd call a NORETURN function,\n> because the compiler (rightly) realizes there is no point in making sure\n> we can call a later free() that we'll never reach.\n\nFWIW I'm not sure it's NORETURN, and haven't had time to dig...\n"}]}