Skip to content
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

Add periodic checks with sanitizers #2763

Merged
merged 3 commits into from
May 9, 2024
Merged

Add periodic checks with sanitizers #2763

merged 3 commits into from
May 9, 2024

Conversation

daljit46
Copy link
Member

@daljit46 daljit46 commented Nov 30, 2023

Sanitizers are very valuable tools to detect problems by instrumenting code for runtime bug detection. They have already proven useful for us as demonstrated in #2760, #2761 and #2755.
This PR adds a new GitHub Actions workflow that runs our unit and binary tests compiled with sanitizers provided by LLVM each night. It'd be rather computationally expensive to run these checks on pull requests, but my tests show that they are fast enough to be run once a day (with caching enabled).

Note that this PR must be merged into master since scheduled GitHub Actions only run on the default or base branch.

@daljit46 daljit46 requested review from jdtournier and a team November 30, 2023 09:24
@daljit46 daljit46 self-assigned this Nov 30, 2023
@daljit46 daljit46 force-pushed the san_checks branch 2 times, most recently from 54a74fb to 175bb9f Compare November 30, 2023 09:31
@daljit46
Copy link
Member Author

After the discussion today, I think it may make sense to wait for this PR to be merged until most issues spotted by the sanitizers have been fixed on dev.
Also, I think it may make sense to schedule this weekly, instead of daily (although we should try to make sure that CI cache isn't invalidated between consecutive runs).

@daljit46
Copy link
Member Author

daljit46 commented Mar 11, 2024

I think this is mostly ready to be merged. All of issues mentioned in #2772 seems to have fixed apart from #2779. I think that's still waiting a review from @jdtournier.
Once that's merged, I don't see any problem with merging to master.

EDIT: never mind, it seems that TSAN has spotted other bugs, so we should fix those too.

@daljit46 daljit46 changed the title Add nightly checks with sanitizers Add periodic checks with sanitizers May 9, 2024
@daljit46 daljit46 marked this pull request as ready for review May 9, 2024 09:06
@daljit46 daljit46 requested a review from Lestropie May 9, 2024 09:06
@daljit46
Copy link
Member Author

daljit46 commented May 9, 2024

All tests are now passing, so I think this is now mergeable.

@Lestropie Lestropie added this pull request to the merge queue May 9, 2024
Merged via the queue into master with commit fb4dd43 May 9, 2024
5 checks passed
@Lestropie Lestropie deleted the san_checks branch May 9, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants