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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jul 26, 2022, 13:30 UTC
Message-ID: <d4dcb1f6-6076-3725-d479-7e9f1fece2a3@github.com>
In-Reply-To: <patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com>

On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

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

Show 14 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

Show 10 quoted lines

> @@ -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, since the point is not to test the Linux platform but to use the additional runtime checks (and Linux is the fasted CI platform).

Thanks, -Stolee