Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jul 26 2022, Derrick Stolee wrote:

> 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.

>> 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 :)