Skip to content

Add ASAN to CI #5180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add ASAN to CI #5180

wants to merge 2 commits into from

Conversation

KrystalDelusion
Copy link
Member

What are the reasons/motivation for this change?

We have UBSAN running on CI, but sometimes ASAN issues slip through as well (e.g. #5020, #5163).

Explain how this is achieved.

Define the sanitizer as part of the test strategy matrix in the test-build.yml family of CI jobs.

There are still memory leaks being picked up in the Verific tests, but I added a (hopefully temporary) patch to tests/verific/run-test.sh to disable halt_on_error for the tests in that directory. The sanitizer builds are running on the open source runner, so they shouldn't actually hit the Verific tests anyway, but better safe than sorry.

If applicable, please suggest to reviewers how they can test the change.

CI should pass, having run the test suite with ASAN.

New matrix variable for sanitizer, running `undefined` and `address` separately
(because they are mutually exclusive).  Probably don't need to run both
sanitizers on both os targets, but it's probably fine.
@KrystalDelusion
Copy link
Member Author

FYI this is failing CI because of a memory issue introduced by #5152

@widlarizer
Copy link
Collaborator

I have this fixed in my big ownership PR (#5135), specifically this bit but applying an equivalent to the relevant part to main isn't enough, see #5189

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants