On Tue, Jul 26 2022, Derrick Stolee wrote:

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

quoted

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.

quoted

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

quoted

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