Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- Jul 27, 2022, 11:34 UTC
- Message-ID
- <483f9183-d7bb-4391-4299-d89f83d481d2@github.com>
- In-Reply-To
- <220726.867d40ng6j.gmgdl@evledraar.gmail.com>
On 7/26/22 9:33 AM, Ævar Arnfjörð Bjarmason wrote:
> > On Tue, Jul 26 2022, Derrick Stolee wrote: > >> On 7/26/2022 7:09 AM, Ævar Arnfjörð Bjarmason wrote:
Show 36 quoted lines
>>> - 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".
Yes, SANITIZE=leak requires extra steps to make tests pass, but as far as setting up the CI job it is extremely similar to the two you are adding here. Their purpose is also very similar: add compiler flags that would cause our test suite to fail when certain memory issues occur. We just expect the SANITIZE=address and SANITIZE=undefined to pass for every test in the test suite.
That's why I think renaming linux-leaks to SANITIZE=leak is the best option.
Thanks, -Stolee