{"thread":{"id":"58224","subject":"[PATCH] CI: add SANITIZE=[address|undefined] jobs","startedAt":"2022-07-26T11:09:22Z","lastAt":"2022-07-28T23:18:25Z","messageCount":12,"participants":["Ævar Arnfjörð Bjarmason","Derrick Stolee","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"459965","messageId":"patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com","threadId":"58224","inReplyTo":null,"subject":"[PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-26T11:09:13Z","receivedAt":"2022-07-26T11:09:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add CI targets for SANITIZE=address and SANITIZE=undefined. The former\nwould have caught a regression in 18bbc795fc5 (Merge branch\n'gc/bare-repo-discovery', 2022-07-22) which made its way to\n\"master\"[1].\n\nPer [2] the GitHub fork of git.git runs with these in CI, so it's\nalready useful to some forks of this repository.\n\nAlso per [2] we could use SANITIZE=address with some ASAN_OPTIONS\ninstead of our SANITIZE=leak job added in 956d2e4639b (tests: add a\ntest mode for SANITIZE=leak, run it in CI, 2021-09-23), but unifying\nthose two with these new jobs would be a lot harder, so let's leave\nthat for now.\n\nOn my system a \"make test\" takes around 12m of user time,\nSANITIZE=address around 44m, and SANITIZE=undefined around 18m.\n\nIn practice this doesn't seem to slow down the wallclock time of the\nthe GitHub CI by much This runs in about 50m, but getting through some\nof the \"win build\" and associated tests can take around 40m, and the\n\"OSX\" tests are on the order of 40m (all of this may vary with the\navailable workers etc.).\n\nThe \"address\" job will fail due to the issue reported in [1]. the\n\"undefined\" one succeeds, but note that \"t4058-diff-duplicates.sh\"\ntriggers its assertions, they're just hidden by\n\"test_expect_failure\" (see [3] for why).\n\n1. https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com/\n2. https://lore.kernel.org/git/YPClS0fj2HOJE5nH@coredump.intra.peff.net/\n3. https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThis CI wil catch the memory corruption that made it to \"master\", see:\nhttps://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com/\n\n .github/workflows/main.yml | 6 ++++++\n ci/lib.sh                  | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex cd1f52692a5..4f59a7aa44c 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -251,6 +251,12 @@ jobs:\n           - jobname: linux-leaks\n             cc: gcc\n             pool: ubuntu-latest\n+          - jobname: SANITIZE=address\n+            cc: gcc\n+            pool: ubuntu-latest\n+          - jobname: SANITIZE=undefined\n+            cc: gcc\n+            pool: ubuntu-latest\n     env:\n       CC: ${{matrix.vector.cc}}\n       CC_PACKAGE: ${{matrix.vector.cc_package}}\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex f095519f8db..5ad60af2e7c 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -277,6 +277,12 @@ linux-leaks)\n \texport SANITIZE=leak\n \texport GIT_TEST_PASSING_SANITIZE_LEAK=true\n \t;;\n+SANITIZE=address)\n+\texport SANITIZE=address\n+\t;;\n+SANITIZE=undefined)\n+\texport SANITIZE=undefined\n+\t;;\n esac\n \n MAKEFLAGS=\"$MAKEFLAGS CC=${CC:-cc}\"\n-- \n2.37.1.1107.g76fe5d1ed7c\n\n"},{"id":"459968","messageId":"d4dcb1f6-6076-3725-d479-7e9f1fece2a3@github.com","threadId":"58224","inReplyTo":"patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-26T13:30:07Z","receivedAt":"2022-07-26T13:30:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:\n> Add CI targets for SANITIZE=address and SANITIZE=undefined. The former\n> would have caught a regression in 18bbc795fc5 (Merge branch\n> 'gc/bare-repo-discovery', 2022-07-22) which made its way to\n> \"master\"[1].\n> \n> Per [2] the GitHub fork of git.git runs with these in CI, so it's\n> already useful to some forks of this repository.\n\nI'm a fan of adding additional sanitizer checks in our CI. Let's let\ncomputers do the work for us here instead of relying on humans.\n\n> Also per [2] we could use SANITIZE=address with some ASAN_OPTIONS\n> instead of our SANITIZE=leak job added in 956d2e4639b (tests: add a\n> test mode for SANITIZE=leak, run it in CI, 2021-09-23), but unifying\n> those two with these new jobs would be a lot harder, so let's leave\n> that for now.\n>            - jobname: linux-leaks\n>              cc: gcc\n>              pool: ubuntu-latest\n> +          - jobname: SANITIZE=address\n> +            cc: gcc\n> +            pool: ubuntu-latest\n> +          - jobname: SANITIZE=undefined\n> +            cc: gcc\n> +            pool: ubuntu-latest\n\n> @@ -277,6 +277,12 @@ linux-leaks)\n>  \texport SANITIZE=leak\n>  \texport GIT_TEST_PASSING_SANITIZE_LEAK=true\n>  \t;;\n> +SANITIZE=address)\n> +\texport SANITIZE=address\n> +\t;;\n> +SANITIZE=undefined)\n> +\texport SANITIZE=undefined\n> +\t;;\n\nIn both of these cases, we are breaking from the nearby pattern. These\njobs could be renamed to linux-address and linux-undefined to match the\nlinux-leaks job.\n\nAlternatively, we could rename linux-leaks to SANITIZE=leak, since the\npoint is not to test the Linux platform but to use the additional runtime\nchecks (and Linux is the fasted CI platform).\n\nThanks,\n-Stolee\n"},{"id":"459970","messageId":"220726.867d40ng6j.gmgdl@evledraar.gmail.com","threadId":"58224","inReplyTo":"d4dcb1f6-6076-3725-d479-7e9f1fece2a3@github.com","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-26T13:33:50Z","receivedAt":"2022-07-26T13:45:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 26 2022, Derrick Stolee wrote:\n\n> On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:\n>> Add CI targets for SANITIZE=address and SANITIZE=undefined. The former\n>> would have caught a regression in 18bbc795fc5 (Merge branch\n>> 'gc/bare-repo-discovery', 2022-07-22) which made its way to\n>> \"master\"[1].\n>> \n>> Per [2] the GitHub fork of git.git runs with these in CI, so it's\n>> already useful to some forks of this repository.\n>\n> I'm a fan of adding additional sanitizer checks in our CI. Let's let\n> computers do the work for us here instead of relying on humans.\n\nThanks, good to have agreement on adding these CI runs.\n\n>> Also per [2] we could use SANITIZE=address with some ASAN_OPTIONS\n>> instead of our SANITIZE=leak job added in 956d2e4639b (tests: add a\n>> test mode for SANITIZE=leak, run it in CI, 2021-09-23), but unifying\n>> those two with these new jobs would be a lot harder, so let's leave\n>> that for now.\n>>            - jobname: linux-leaks\n>>              cc: gcc\n>>              pool: ubuntu-latest\n>> +          - jobname: SANITIZE=address\n>> +            cc: gcc\n>> +            pool: ubuntu-latest\n>> +          - jobname: SANITIZE=undefined\n>> +            cc: gcc\n>> +            pool: ubuntu-latest\n>\n>> @@ -277,6 +277,12 @@ linux-leaks)\n>>  \texport SANITIZE=leak\n>>  \texport GIT_TEST_PASSING_SANITIZE_LEAK=true\n>>  \t;;\n>> +SANITIZE=address)\n>> +\texport SANITIZE=address\n>> +\t;;\n>> +SANITIZE=undefined)\n>> +\texport SANITIZE=undefined\n>> +\t;;\n>\n> In both of these cases, we are breaking from the nearby pattern. These\n> jobs could be renamed to linux-address and linux-undefined to match the\n> linux-leaks job.\n>\n> Alternatively, we could rename linux-leaks to SANITIZE=leak[...]\n\nI deliberately deviated from the \"linux-leaks\" pattern since it's a lot\nmore than just:\n\n\tmake SANITIZE=leak test\n\nI.e. we instrument what tests we run, skip some individual ones\netc. These are different in that we can run the entire set. I'd think\nthe reverse would make sense, i.e. one day if we run fully with\nSANITIZE=leak enabled to rename that job to \"SANITIZE=leak\".\n\n> [...], since the\n> point is not to test the Linux platform but to use the additional runtime\n> checks (and Linux is the fasted CI platform).\n\nStrictly speaking these tests are platform-specific in that they require\nus to take certain codepaths at runtime, so if we have any\nplatform-specific code, or code that's affected by compilation options\n(say NO_REGEX=Y v.s. using the libc's) we might see failures on one\nplatform, but not another. Compilation flags also matter (e.g. -O0\nv.s. -O3).\n\nBut I think for all of [leak,address,undefined] it's a sensible\ntrade-off for now to just pick one specific platform.\n\nSince it's very unlikely that the resulting issues are OS-specific I\nthought it made sense to leave \"linux\" out of it, just like we have\n\"pedantic\", not \"linux-pedantic\", ditto \"sparse\" which is also\nplatform-specific around the edges.\n\nHaving said all that I really don't care much what we call these as long\nas we get the test coverage, but I'll hold off on any possible re-roll\nto see if others chime in about the bikeshed :)\n"},{"id":"460019","messageId":"483f9183-d7bb-4391-4299-d89f83d481d2@github.com","threadId":"58224","inReplyTo":"220726.867d40ng6j.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-27T11:34:31Z","receivedAt":"2022-07-27T11:34:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/26/22 9:33 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Jul 26 2022, Derrick Stolee wrote:\n> \n>> On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:\n\n>>>            - jobname: linux-leaks\n>>>              cc: gcc\n>>>              pool: ubuntu-latest\n>>> +          - jobname: SANITIZE=address\n>>> +            cc: gcc\n>>> +            pool: ubuntu-latest\n>>> +          - jobname: SANITIZE=undefined\n>>> +            cc: gcc\n>>> +            pool: ubuntu-latest\n>>\n>>> @@ -277,6 +277,12 @@ linux-leaks)\n>>>  \texport SANITIZE=leak\n>>>  \texport GIT_TEST_PASSING_SANITIZE_LEAK=true\n>>>  \t;;\n>>> +SANITIZE=address)\n>>> +\texport SANITIZE=address\n>>> +\t;;\n>>> +SANITIZE=undefined)\n>>> +\texport SANITIZE=undefined\n>>> +\t;;\n>>\n>> In both of these cases, we are breaking from the nearby pattern. These\n>> jobs could be renamed to linux-address and linux-undefined to match the\n>> linux-leaks job.\n>>\n>> Alternatively, we could rename linux-leaks to SANITIZE=leak[...]\n> \n> I deliberately deviated from the \"linux-leaks\" pattern since it's a lot\n> more than just:\n> \n> \tmake SANITIZE=leak test\n> \n> I.e. we instrument what tests we run, skip some individual ones\n> etc. These are different in that we can run the entire set. I'd think\n> the reverse would make sense, i.e. one day if we run fully with\n> SANITIZE=leak enabled to rename that job to \"SANITIZE=leak\".\n\nYes, SANITIZE=leak requires extra steps to make tests pass, but\nas far as setting up the CI job it is extremely similar to the\ntwo you are adding here. Their purpose is also very similar:\nadd compiler flags that would cause our test suite to fail when\ncertain memory issues occur. We just expect the SANITIZE=address\nand SANITIZE=undefined to pass for every test in the test suite.\n\nThat's why I think renaming linux-leaks to SANITIZE=leak is the\nbest option.\n\nThanks,\n-Stolee\n"},{"id":"460024","messageId":"xmqqilnityp0.fsf@gitster.g","threadId":"58224","inReplyTo":"d4dcb1f6-6076-3725-d479-7e9f1fece2a3@github.com","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-27T14:30:51Z","receivedAt":"2022-07-27T14:31:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n>>            - jobname: linux-leaks\n>>              cc: gcc\n>>              pool: ubuntu-latest\n>> +          - jobname: SANITIZE=address\n>> +            cc: gcc\n>> +            pool: ubuntu-latest\n>> +          - jobname: SANITIZE=undefined\n>> +            cc: gcc\n>> +            pool: ubuntu-latest\n>\n>> @@ -277,6 +277,12 @@ linux-leaks)\n>>  \texport SANITIZE=leak\n>>  \texport GIT_TEST_PASSING_SANITIZE_LEAK=true\n>>  \t;;\n>> +SANITIZE=address)\n>> +\texport SANITIZE=address\n>> +\t;;\n>> +SANITIZE=undefined)\n>> +\texport SANITIZE=undefined\n>> +\t;;\n>\n> In both of these cases, we are breaking from the nearby pattern. These\n> jobs could be renamed to linux-address and linux-undefined to match the\n> linux-leaks job.\n>\n> Alternatively, we could rename linux-leaks to SANITIZE=leak, since the\n> point is not to test the Linux platform but to use the additional runtime\n> checks (and Linux is the fasted CI platform).\n\nI tend to agree that in the existing linux-leaks job, the\n\"linux\"-ness is much less important than the \"leaks\"-ness, so the\n\"alternative\" might be slightly more preferable, but I do not mind\nthe renaming goes the other way, either.\n\nThanks for your good eyes and good taste.\n\n"},{"id":"460033","messageId":"YuGPeHn9wfF6tWA5@coredump.intra.peff.net","threadId":"58224","inReplyTo":"patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-27T19:18:16Z","receivedAt":"2022-07-27T19:22:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 26, 2022 at 01:09:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Per [2] the GitHub fork of git.git runs with these in CI, so it's\n> already useful to some forks of this repository.\n\nYeah, it has been helpful there and I think this is worth doing as part\nof our CI. It's a lot of CPU versus running the test suite once on\nLinux, but probably not compared to the overall cost of our current CI.\n\nFor the GitHub fork, the code-coverage issues you noticed were easy: we\nonly built one variant, so we could just test with those knobs. ;) But I\ntend to agree with your approach here to just test on one platform,\nwhich covers _most_ of the code. It's certainly better than the status\nquo, and it strikes a nice balance of CPU versus coverage.\n\nOne thing I'd say...\n\n> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n> index cd1f52692a5..4f59a7aa44c 100644\n> --- a/.github/workflows/main.yml\n> +++ b/.github/workflows/main.yml\n> @@ -251,6 +251,12 @@ jobs:\n>            - jobname: linux-leaks\n>              cc: gcc\n>              pool: ubuntu-latest\n> +          - jobname: SANITIZE=address\n> +            cc: gcc\n> +            pool: ubuntu-latest\n> +          - jobname: SANITIZE=undefined\n> +            cc: gcc\n> +            pool: ubuntu-latest\n\nThere's really no reason to split the \"address\" and \"undefined\" builds\ninto two jobs. We expect them to pass, and if one fails, having the\nresults split is not likely to give any extra information. So I think\none job with SANITIZE=address,undefined is fine, and reclaims some of\nthe extra CPU we're spending.\n\n-Peff\n"},{"id":"460127","messageId":"220728.86sfmljhyx.gmgdl@evledraar.gmail.com","threadId":"58224","inReplyTo":"xmqqilnityp0.fsf@gitster.g","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-28T16:52:25Z","receivedAt":"2022-07-28T16:54:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 27 2022, Junio C Hamano wrote:\n\n> Derrick Stolee <derrickstolee@github.com> writes:\n>\n>>>            - jobname: linux-leaks\n>>>              cc: gcc\n>>>              pool: ubuntu-latest\n>>> +          - jobname: SANITIZE=address\n>>> +            cc: gcc\n>>> +            pool: ubuntu-latest\n>>> +          - jobname: SANITIZE=undefined\n>>> +            cc: gcc\n>>> +            pool: ubuntu-latest\n>>\n>>> @@ -277,6 +277,12 @@ linux-leaks)\n>>>  \texport SANITIZE=leak\n>>>  \texport GIT_TEST_PASSING_SANITIZE_LEAK=true\n>>>  \t;;\n>>> +SANITIZE=address)\n>>> +\texport SANITIZE=address\n>>> +\t;;\n>>> +SANITIZE=undefined)\n>>> +\texport SANITIZE=undefined\n>>> +\t;;\n>>\n>> In both of these cases, we are breaking from the nearby pattern. These\n>> jobs could be renamed to linux-address and linux-undefined to match the\n>> linux-leaks job.\n>>\n>> Alternatively, we could rename linux-leaks to SANITIZE=leak, since the\n>> point is not to test the Linux platform but to use the additional runtime\n>> checks (and Linux is the fasted CI platform).\n>\n> I tend to agree that in the existing linux-leaks job, the\n> \"linux\"-ness is much less important than the \"leaks\"-ness, so the\n> \"alternative\" might be slightly more preferable, but I do not mind\n> the renaming goes the other way, either.\n\nI'm fine with it either way, but to add a prep patch to this series to\ndo s/linux-leaks/SANITIZE=leak/g would cause a semantic conflict with my\nin-flight\nhttps://lore.kernel.org/git/cover-v3-00.15-00000000000-20220727T230800Z-avarab@gmail.com/;\nIt refers to \"linux-leaks\" in newly added documentation.\n\nSo what would you prefer here, to take a lightly altered re-roll of this\n(I think Jeff's suggestion of bundling \"undefined\" into one job makes\nsense), or have it build on top of that otherwise unrelated series, or\njust do the s/linux-leaks/.../g as an eventual follow-up?\n"},{"id":"460129","messageId":"220728.86o7x9jhrp.gmgdl@evledraar.gmail.com","threadId":"58224","inReplyTo":"YuGPeHn9wfF6tWA5@coredump.intra.peff.net","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-28T16:54:37Z","receivedAt":"2022-07-28T16:59:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 27 2022, Jeff King wrote:\n\n> On Tue, Jul 26, 2022 at 01:09:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Per [2] the GitHub fork of git.git runs with these in CI, so it's\n>> already useful to some forks of this repository.\n>\n> Yeah, it has been helpful there and I think this is worth doing as part\n> of our CI. It's a lot of CPU versus running the test suite once on\n> Linux, but probably not compared to the overall cost of our current CI.\n>\n> For the GitHub fork, the code-coverage issues you noticed were easy: we\n> only built one variant, so we could just test with those knobs. ;) But I\n> tend to agree with your approach here to just test on one platform,\n> which covers _most_ of the code. It's certainly better than the status\n> quo, and it strikes a nice balance of CPU versus coverage.\n\n*nod*\n\n> One thing I'd say...\n>\n>> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n>> index cd1f52692a5..4f59a7aa44c 100644\n>> --- a/.github/workflows/main.yml\n>> +++ b/.github/workflows/main.yml\n>> @@ -251,6 +251,12 @@ jobs:\n>>            - jobname: linux-leaks\n>>              cc: gcc\n>>              pool: ubuntu-latest\n>> +          - jobname: SANITIZE=address\n>> +            cc: gcc\n>> +            pool: ubuntu-latest\n>> +          - jobname: SANITIZE=undefined\n>> +            cc: gcc\n>> +            pool: ubuntu-latest\n>\n> There's really no reason to split the \"address\" and \"undefined\" builds\n> into two jobs. We expect them to pass, and if one fails, having the\n> results split is not likely to give any extra information. So I think\n> one job with SANITIZE=address,undefined is fine, and reclaims some of\n> the extra CPU we're spending.\n\nI'll do that in a re-roll, pending a resolution of the naming discussion\nat:\nhttps://lore.kernel.org/git/220728.86sfmljhyx.gmgdl@evledraar.gmail.com/\n\nBut note that it *does* give you extra information to split them up\ncurrently, i.e. the \"test_expect_failure\" that you get with \"undefined\"\nisn't conflated with the non-changes that SANITIZE=address flags (sans\noutstanding recent breakage) in the test report.\n\nBut just having that \"TODO\" test sitting there will suck less than\npotentially having CI run much longer, or taking up resources from\nconcurrent CI runs, so I'll do this.\n\nWe also leave a lot of CI performance on the table by e.g. doing \"chain\nlint\" in every single test run (except Windows), there *are* platform\nedge-cases there like with SANITIZE=address, but I wonder if we should\njust declare it good enough to do it in 1-2 jobs.\n\nDitto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about\nall of those some other time....\n\nThanks for reviewing this.\n"},{"id":"460134","messageId":"xmqq4jz1cexq.fsf@gitster.g","threadId":"58224","inReplyTo":"220728.86sfmljhyx.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-28T17:41:53Z","receivedAt":"2022-07-28T17:42:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I'm fine with it either way, but to add a prep patch to this series to\n> do s/linux-leaks/SANITIZE=leak/g would cause a semantic conflict with my\n> in-flight\n> https://lore.kernel.org/git/cover-v3-00.15-00000000000-20220727T230800Z-avarab@gmail.com/;\n> It refers to \"linux-leaks\" in newly added documentation.\n\nIf you mean \"in-flight\" something that is not in 'next', it may have\nto be rerolled on top of this new topic.\n\nIf something is already in 'next' and we are committed to see it\nthrough down to 'master', then this new topic may need to depend on\nit.\n"},{"id":"460150","messageId":"YuL7EotrIpnOn5BT@coredump.intra.peff.net","threadId":"58224","inReplyTo":"220728.86o7x9jhrp.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-28T21:09:38Z","receivedAt":"2022-07-28T21:09:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 28, 2022 at 06:54:37PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > There's really no reason to split the \"address\" and \"undefined\" builds\n> > into two jobs. We expect them to pass, and if one fails, having the\n> > results split is not likely to give any extra information. So I think\n> > one job with SANITIZE=address,undefined is fine, and reclaims some of\n> > the extra CPU we're spending.\n> \n> I'll do that in a re-roll, pending a resolution of the naming discussion\n> at:\n> https://lore.kernel.org/git/220728.86sfmljhyx.gmgdl@evledraar.gmail.com/\n\nThanks. FWIW, I have no opinion on the job naming. ;)\n\n> But note that it *does* give you extra information to split them up\n> currently, i.e. the \"test_expect_failure\" that you get with \"undefined\"\n> isn't conflated with the non-changes that SANITIZE=address flags (sans\n> outstanding recent breakage) in the test report.\n> \n> But just having that \"TODO\" test sitting there will suck less than\n> potentially having CI run much longer, or taking up resources from\n> concurrent CI runs, so I'll do this.\n\nMy hope would be that we'd identify and fix cases where this flagged,\neven in expect_failure, and so any state like you describe would be\ntemporary. In the long term, we should be able to assume that these\ndon't trigger warnings in the existing code base, and optimize in that\ndirection.\n\n> We also leave a lot of CI performance on the table by e.g. doing \"chain\n> lint\" in every single test run (except Windows), there *are* platform\n> edge-cases there like with SANITIZE=address, but I wonder if we should\n> just declare it good enough to do it in 1-2 jobs.\n\nI'd be fine with that, but I think chain lint isn't actually that\nexpensive. The original in-shell bits are super cheap. The extra\nsed process is measurable, but I think I blunted the worst of it in the\n2d86a96220 (t: avoid sed-based chain-linting in some expensive cases,\n2021-05-13).\n\nStill, that patch should make it easy to time things just by setting\nGIT_TEST_CHAIN_LINT_HARDER=0 in various jobs. It does seem to buy ~100s\nof CPU time per test run on my Linux box. That's not a lot in the grand\nscheme, but perhaps adds up. And I could believe it's much worse on\nWindows. Maybe worth seeing how it performs in the actual CI\nenvironments.\n\n> Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about\n> all of those some other time....\n\nI'd be surprised if the malloc checking itself is all that expensive,\nthough it does look like we call getconf and expr once per test there\nfor setup. We could almost certainly hoist that out and call it once per\nscript.\n\n-Peff\n"},{"id":"460154","messageId":"YuMAIfM3jv6lI9SA@coredump.intra.peff.net","threadId":"58224","inReplyTo":"YuL7EotrIpnOn5BT@coredump.intra.peff.net","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-28T21:31:13Z","receivedAt":"2022-07-28T21:31:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 28, 2022 at 05:09:38PM -0400, Jeff King wrote:\n\n> > Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about\n> > all of those some other time....\n> \n> I'd be surprised if the malloc checking itself is all that expensive,\n> though it does look like we call getconf and expr once per test there\n> for setup. We could almost certainly hoist that out and call it once per\n> script.\n\nNope, I was totally wrong here. Running with TEST_NO_MALLOC_CHECK=1 does\nindeed make a big difference:\n\n  Benchmark 1: make test\n    Time (mean ± σ):     67.486 s ±  3.339 s    [User: 622.643 s, System: 409.951 s]\n    Range (min … max):   63.634 s … 69.550 s    3 runs\n   \n  Benchmark 2: TEST_NO_MALLOC_CHECK=1 make test\n    Time (mean ± σ):     57.596 s ±  0.899 s    [User: 577.273 s, System: 291.627 s]\n    Range (min … max):   57.072 s … 58.634 s    3 runs\n\nEliminating the extra per-test processes with the patch below helps a\nlittle, but most of the effort really is (presumably) going to the\nactual malloc checking code:\n\n  Benchmark 1: make test [nb: after applying patch...]\n  Time (mean ± σ):     67.091 s ±  0.385 s    [User: 599.382 s, System: 410.781 s]\n  Range (min … max):   66.648 s … 67.343 s    3 runs\n\nCurious that most of the CPU time seems to go into system time. I'd have\nthought it was extra internal malloc debugging bits, but maybe it is\nallocating mmap/brk calls in a less efficient way. I dunno how any of it\nactually works.\n\n-Peff\n\n---\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 55857af601..303fbe8ecc 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -557,14 +557,20 @@ then\n \t\t: nothing\n \t}\n else\n+\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n+\t   _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n+\t    expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n+\tthen\n+\t\tUSE_LIBC_MALLOC_DEBUG=t\n+\telse\n+\t\tUSE_LIBC_MALLOC_DEBUG=\n+\tfi\n \tsetup_malloc_check () {\n \t\tlocal g\n \t\tlocal t\n \t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n \t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n-\t\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n-\t\t   _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n-\t\t   expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n+\t\tif test -n \"$USE_LIBC_MALLOC_DEBUG\"\n \t\tthen\n \t\t\tg=\n \t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\"\n"},{"id":"460162","messageId":"220729.8635ekkerp.gmgdl@evledraar.gmail.com","threadId":"58224","inReplyTo":"YuL7EotrIpnOn5BT@coredump.intra.peff.net","subject":"Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-28T23:10:21Z","receivedAt":"2022-07-28T23:18:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 28 2022, Jeff King wrote:\n\n> On Thu, Jul 28, 2022 at 06:54:37PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> [...]\n>> We also leave a lot of CI performance on the table by e.g. doing \"chain\n>> lint\" in every single test run (except Windows), there *are* platform\n>> edge-cases there like with SANITIZE=address, but I wonder if we should\n>> just declare it good enough to do it in 1-2 jobs.\n>\n> I'd be fine with that, but I think chain lint isn't actually that\n> expensive. The original in-shell bits are super cheap. The extra\n> sed process is measurable, but I think I blunted the worst of it in the\n> 2d86a96220 (t: avoid sed-based chain-linting in some expensive cases,\n> 2021-05-13).\n>\n> Still, that patch should make it easy to time things just by setting\n> GIT_TEST_CHAIN_LINT_HARDER=0 in various jobs. It does seem to buy ~100s\n> of CPU time per test run on my Linux box. That's not a lot in the grand\n> scheme, but perhaps adds up. And I could believe it's much worse on\n> Windows. Maybe worth seeing how it performs in the actual CI\n> environments.\n>\n>> Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about\n>> all of those some other time....\n>\n> I'd be surprised if the malloc checking itself is all that expensive,\n> though it does look like we call getconf and expr once per test there\n> for setup. We could almost certainly hoist that out and call it once per\n> script.\n\nI posted some benchmarks for these a while ago:\nhttps://lore.kernel.org/git/220405.86k0c3lt2l.gmgdl@evledraar.gmail.com/\n\nYMMV, but these make a big difference for some tests, although less for\na whole run, as you point out disabling chain lint least impact.\n\nFor me a full test suite run is:\n\n* TEST_NO_MALLOC_CHECK=1 --no-bin-wrappers --no-chain-lint:\n\treal    1m55.600s\n\tuser    8m48.039s\n\tsys     3m17.776s\n* [no opts, TEST_NO_MALLOC_CHECK=0 & --bin-wrappers && --chain-lint are on]\n\treal    2m18.126s\n\tuser    10m40.230s\n\tsys     4m7.152s\n\nSo far from the >2x speedup you can get with some tests, but a big\ndifference nonetheless.\n"}]}