{"thread":{"id":"59822","subject":"[PATCH 0/3] ci sanitizer cleanups and performance improvements","startedAt":"2023-06-01T18:02:26Z","lastAt":"2023-06-03T01:35:46Z","messageCount":5,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"477893","messageId":"20230601180220.GA4167745@coredump.intra.peff.net","threadId":"59822","inReplyTo":null,"subject":"[PATCH 0/3] ci sanitizer cleanups and performance improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-01T18:02:20Z","receivedAt":"2023-06-01T18:02:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are a few patches to tweak our CI sanitizer setup. The first one\nhopefully increases coverage. The other two just aim to reduce the\namount of CPU we use.\n\n  [1/3]: ci: use clang for ASan/UBSan checks\n  [2/3]: ci: run ASan/UBSan in a single job\n  [3/3]: ci: drop linux-clang job\n\n .github/workflows/main.yml | 10 ++--------\n ci/lib.sh                  |  7 ++-----\n 2 files changed, 4 insertions(+), 13 deletions(-)\n\n-Peff\n"},{"id":"477894","messageId":"20230601180351.GA4167886@coredump.intra.peff.net","threadId":"59822","inReplyTo":"20230601180220.GA4167745@coredump.intra.peff.net","subject":"[PATCH 1/3] ci: use clang for ASan/UBSan checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-01T18:03:51Z","receivedAt":"2023-06-01T18:03:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Both gcc and clang support the \"address\" and \"undefined\" sanitizers.\nHowever, they may produce different results. We've seen at least two\nreal world cases where gcc missed a UBSan problem but clang found it:\n\n  1. Clang's UBSan (using clang 14.0.6) found a string index that was\n     subtracted to \"-1\", causing an out-of-bounds read (curiously this\n     didn't trigger ASan, but that may be because the string was in the\n     argv memory, not stack or heap). Using gcc (version 12.2.0) didn't\n     find the same problem.\n\n     Original thread:\n     https://lore.kernel.org/git/20230519005447.GA2955320@coredump.intra.peff.net/\n\n  2. Clang's UBSan (using clang 4.0.1) complained about pointer\n     arithmetic with NULL, but gcc at the time did not. This was in\n     2017, and modern gcc does seem to find the issue, though.\n\n     Original thread:\n     https://lore.kernel.org/git/32a8b949-638a-1784-7fba-948ae32206fc@web.de/\n\nSince we don't otherwise have a particular preference for one compiler\nover the other for this test, let's switch to the one that we think may\nbe more thorough.\n\nNote that it's entirely possible that the two are simply _different_,\nand we are trading off problems that gcc would find that clang wouldn't.\nHowever, my subjective and anecdotal experience has been that clang's\nsanitizer support is a bit more mature (e.g., I recall other oddities\naround leak-checking where clang performed more sensibly).\n\nObviously running both and cross-checking the results would give us the\nbest coverage, but that's very expensive to run (and these are already\nsome of our most expensive CI jobs). So let's use clang as our best\nguess, and we can re-evaluate if we get more data points.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nYou can see that this catches the recent problem when merged with\n'next':\n\n  https://github.com/peff/git/actions/runs/5102807385/jobs/9172605638\n\n .github/workflows/main.yml | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 30492eacdd..487ad31e66 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -274,10 +274,10 @@ jobs:\n             cc: gcc\n             pool: ubuntu-latest\n           - jobname: linux-asan\n-            cc: gcc\n+            cc: clang\n             pool: ubuntu-latest\n           - jobname: linux-ubsan\n-            cc: gcc\n+            cc: clang\n             pool: ubuntu-latest\n     env:\n       CC: ${{matrix.vector.cc}}\n-- \n2.41.0.346.g8d12207a4f\n\n"},{"id":"477895","messageId":"20230601180947.GB4167886@coredump.intra.peff.net","threadId":"59822","inReplyTo":"20230601180220.GA4167745@coredump.intra.peff.net","subject":"[PATCH 2/3] ci: run ASan/UBSan in a single job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-01T18:09:47Z","receivedAt":"2023-06-01T18:10:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we started running sanitizers in CI via 1c0962c0c4 (ci: add address\nand undefined sanitizer tasks, 2022-10-20), we ran them as two separate\nCI jobs, since as that commit notes, the combination \"seems to take\nforever\".\n\nAnd indeed, it does with gcc. However, since the previous commit\nswitched to using clang, the situation is different, and we can save\nsome CPU by using a single job for both. Comparing before/after CI runs,\nthis saved about 14 minutes (the single combined job took 54m, versus\n44m plus 24m for ASan and UBSan jobs, respectively). That's wall-clock\nand not CPU, but since our jobs are mostly CPU-bound, the two should be\nclosely proportional.\n\nThis does increase the end-to-end time of a CI run, though, since before\nthis patch the two jobs could run in parallel, and the sanitizer job is\nour longest single job. It also means that we won't get a separate\nresult for \"this passed with UBSan but not with ASan\" or vice versa).\nBut as 1c0962c0c4 noted, that is not a very useful signal in practice.\n\nBelow are some more detailed timings of gcc vs clang that I measured by\nrunning the test suite on my local workstation. Each measurement counts\nonly the time to run the test suite with each compiler (not the compile\ntime itself). We'll focus on the wall-clock times for simplicity, though\nthe CPU times follow roughly similar trends.\n\nHere's a run with CC=gcc as a baseline:\n\n  real\t1m12.931s\n  user\t9m30.566s\n  sys\t8m9.538s\n\nRunning with SANITIZE=address increases the time by a factor of ~4.7x:\n\n  real\t5m40.352s\n  user\t49m37.044s\n  sys\t36m42.950s\n\nRunning with SANITIZE=undefined increases the time by a factor of ~1.7x:\n\n  real\t2m5.956s\n  user\t12m42.847s\n  sys\t19m27.067s\n\nSo let's call that 6.4 time units to run them separately (where a unit\nis the time it takes to run the test suite with no sanitizers). As a\nsimplistic model, we might imagine that running them together would take\n5.4 units (we save 1 unit because we are no longer running the test\nsuite twice, but just paying the sanitizer overhead on top of a single\nrun).\n\nBut that's not what happens. Running with SANITIZE=address,undefined\nresults in a factor of 9.3x:\n\n  real\t11m9.817s\n  user\t77m31.284s\n  sys\t96m40.454s\n\nSo not only did we not get faster when doing them together, we actually\nspent 1.5x as much CPU as doing them separately! And while those\nwall-clock numbers might not look too terrible, keep in mind that this\nis on an unloaded 8-core machine. In the CI environment, wall-clock\ntimes will be much closer to CPU times. So not only are we wasting CPU,\nbut we risk hitting timeouts.\n\nNow let's try the same thing with clang. Here's our no-sanitizer\nbaseline run, which is almost identical to the gcc one (which is quite\nconvenient, because we can keep using the same \"time units\" to get an\napples-to-apples comparison):\n\n  real\t1m11.844s\n  user\t9m28.313s\n  sys\t8m8.240s\n\nAnd now again with SANITIZE=address, we get a 5x factor (so slightly\nworse than gcc's 4.7x, though I wouldn't read too much into it; there is\na fair bit of run-to-run noise):\n\n  real\t6m7.662s\n  user\t49m24.330s\n  sys\t44m13.846s\n\nAnd with SANITIZE=undefined, we are at 1.5x, slightly outperforming gcc\n(though again, that's probably mostly noise):\n\n  real\t1m50.028s\n  user\t11m0.973s\n  sys\t16m42.731s\n\nSo running them separately, our total cost is 6.5x. But if we combine\nthem in a single run (SANITIZE=address,undefined), we get:\n\n  real\t6m51.804s\n  user\t52m32.049s\n  sys\t51m46.711s\n\nwhich is a factor of 5.7x. That's along the lines we'd hoped for!\nRunning them together saves us almost a whole time unit. And that's not\ncounting any time spent outside the test suite itself (starting the job,\nsetting up the environment, compiling) that we're no longer duplicating\nby having two jobs.\n\nSo clang behaves like we'd hope: the overhead to run the sanitizers is\nadditive as you add more sanitizers. Whereas gcc's numbers seem very\nclose to multiplicative, almost as if the sanitizers were enforcing\ntheir overheads on each other (though that is purely a guess on what is\ngoing on; ultimately what matters to us is the amount of time it takes).\n\nAnd that roughly matches the CI improvement I saw. A \"time unit\" there\nis more like 12 minutes, and the observed time savings was 14 minutes\n(with the extra presumably coming from avoiding duplicated setup, etc).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm not excited about increasing the total run-time. However, if we want\nto deal with that, I suspect a better solution is to split the tests up\ninto several parallel jobs the way the Windows ones do. That's a lot\nmore work to set up, but it would let us bring the runtime down to match\nother tests (which seem to take 15-20 minutes, versus 44 minutes even\nbefore this patch).\n\n .github/workflows/main.yml | 5 +----\n ci/lib.sh                  | 7 ++-----\n 2 files changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 487ad31e66..2114303b7d 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -273,10 +273,7 @@ jobs:\n           - jobname: linux-leaks\n             cc: gcc\n             pool: ubuntu-latest\n-          - jobname: linux-asan\n-            cc: clang\n-            pool: ubuntu-latest\n-          - jobname: linux-ubsan\n+          - jobname: linux-asan-ubsan\n             cc: clang\n             pool: ubuntu-latest\n     env:\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex db7105e8a8..369d462f13 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -278,11 +278,8 @@ linux-leaks)\n \texport GIT_TEST_PASSING_SANITIZE_LEAK=true\n \texport GIT_TEST_SANITIZE_LEAK_LOG=true\n \t;;\n-linux-asan)\n-\texport SANITIZE=address\n-\t;;\n-linux-ubsan)\n-\texport SANITIZE=undefined\n+linux-asan-ubsan)\n+\texport SANITIZE=address,undefined\n \t;;\n esac\n \n-- \n2.41.0.346.g8d12207a4f\n\n"},{"id":"477896","messageId":"20230601181040.GC4167886@coredump.intra.peff.net","threadId":"59822","inReplyTo":"20230601180220.GA4167745@coredump.intra.peff.net","subject":"[PATCH 3/3] ci: drop linux-clang job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-01T18:10:40Z","receivedAt":"2023-06-01T18:10:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the linux-asan-ubsan job runs using clang under Linux, there is\nnot much point in running a separate clang job. Any errors that a normal\nclang compile-and-test cycle would find are likely to be a subset of\nwhat the sanitizer job will find. Since this job takes ~14 minutes to\nrun in CI, this shaves off some of our CPU load (though it does not\naffect end-to-end runtime, since it's typically run in parallel and is\nnot the longest job).\n\nTechnically this provides us with slightly less signal for a given run,\nsince you won't immediately know if a failure in the sanitizer job is\nfrom using clang or from the sanitizers themselves. But it's generally\nobvious from the logs, and anyway your next step would be to fix the\nprobvlem and re-run CI, since we expect all of these jobs to pass\nnormally.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously not necessary, but just seemed like another easy way to trim\nour resource usage. Really, the same logic could apply before my series\nto the linux-gcc job, which is similarly redundant.\n\n .github/workflows/main.yml | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 2114303b7d..079645b776 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -246,9 +246,6 @@ jobs:\n       fail-fast: false\n       matrix:\n         vector:\n-          - jobname: linux-clang\n-            cc: clang\n-            pool: ubuntu-latest\n           - jobname: linux-sha256\n             cc: clang\n             pool: ubuntu-latest\n-- \n2.41.0.346.g8d12207a4f\n"},{"id":"478000","messageId":"xmqq1qit8h06.fsf@gitster.g","threadId":"59822","inReplyTo":"20230601180220.GA4167745@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] ci sanitizer cleanups and performance improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-03T01:35:37Z","receivedAt":"2023-06-03T01:35:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here are a few patches to tweak our CI sanitizer setup. The first one\n> hopefully increases coverage. The other two just aim to reduce the\n> amount of CPU we use.\n>\n>   [1/3]: ci: use clang for ASan/UBSan checks\n>   [2/3]: ci: run ASan/UBSan in a single job\n>   [3/3]: ci: drop linux-clang job\n>\n>  .github/workflows/main.yml | 10 ++--------\n>  ci/lib.sh                  |  7 ++-----\n>  2 files changed, 4 insertions(+), 13 deletions(-)\n\nMakes sense.  Will queue.  Thanks.\n"}]}