Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jul 26, 2022, 13:33 UTC
- Message-ID
- <220726.867d40ng6j.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <d4dcb1f6-6076-3725-d479-7e9f1fece2a3@github.com>
On Tue, Jul 26 2022, Derrick Stolee wrote:
Show 11 quoted lines
> On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote: >> Add CI targets for SANITIZE=address and SANITIZE=undefined. The former >> would have caught a regression in 18bbc795fc5 (Merge branch >> 'gc/bare-repo-discovery', 2022-07-22) which made its way to >> "master"[1]. >> >> Per [2] the GitHub fork of git.git runs with these in CI, so it's >> already useful to some forks of this repository. > > I'm a fan of adding additional sanitizer checks in our CI. Let's let > computers do the work for us here instead of relying on humans.
Thanks, good to have agreement on adding these CI runs.
Show 31 quoted lines
>> Also per [2] we could use SANITIZE=address with some ASAN_OPTIONS >> instead of our SANITIZE=leak job added in 956d2e4639b (tests: add a >> test mode for SANITIZE=leak, run it in CI, 2021-09-23), but unifying >> those two with these new jobs would be a lot harder, so let's leave >> that for now. >> - jobname: linux-leaks >> cc: gcc >> pool: ubuntu-latest >> + - jobname: SANITIZE=address >> + cc: gcc >> + pool: ubuntu-latest >> + - jobname: SANITIZE=undefined >> + cc: gcc >> + pool: ubuntu-latest > >> @@ -277,6 +277,12 @@ linux-leaks) >> export SANITIZE=leak >> export GIT_TEST_PASSING_SANITIZE_LEAK=true >> ;; >> +SANITIZE=address) >> + export SANITIZE=address >> + ;; >> +SANITIZE=undefined) >> + export SANITIZE=undefined >> + ;; > > In both of these cases, we are breaking from the nearby pattern. These > jobs could be renamed to linux-address and linux-undefined to match the > linux-leaks job. > > Alternatively, we could rename linux-leaks to SANITIZE=leak[...]
I deliberately deviated from the "linux-leaks" pattern since it's a lot more than just:
make SANITIZE=leak test
I.e. we instrument what tests we run, skip some individual ones etc. These are different in that we can run the entire set. I'd think the reverse would make sense, i.e. one day if we run fully with SANITIZE=leak enabled to rename that job to "SANITIZE=leak".
> [...], since the > point is not to test the Linux platform but to use the additional runtime > checks (and Linux is the fasted CI platform).
Strictly speaking these tests are platform-specific in that they require us to take certain codepaths at runtime, so if we have any platform-specific code, or code that's affected by compilation options (say NO_REGEX=Y v.s. using the libc's) we might see failures on one platform, but not another. Compilation flags also matter (e.g. -O0 v.s. -O3).
But I think for all of [leak,address,undefined] it's a sensible trade-off for now to just pick one specific platform.
Since it's very unlikely that the resulting issues are OS-specific I thought it made sense to leave "linux" out of it, just like we have "pedantic", not "linux-pedantic", ditto "sparse" which is also platform-specific around the edges.
Having said all that I really don't care much what we call these as long as we get the test coverage, but I'll hold off on any possible re-roll to see if others chime in about the bikeshed :)