Add sanitize flag to dev.py, fix UBSAN issues and run UBSAN check on CI #3493
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ankith26
force-pushed
the
ankith26-sanitizers
branch
from
60cb872 to
333f3bb
Compare
oddbookworm
approved these changes
Jun 10, 2025
Member
oddbookworm
left a comment
oddbookworm
left a comment
There was a problem hiding this comment.
LGTM! 🎉
Spring cleaning time (in June)
gresm
reviewed
Jun 10, 2025
Contributor
gresm
left a comment
gresm
left a comment
There was a problem hiding this comment.
While it's good as it is, I propose a slightly different scheme for --sanitize argument to allow mixing more modes.
Comment on lines
+385
to
+398
| build_parser.add_argument( | ||
| "--sanitize", | ||
| choices=[ | ||
| "address", | ||
| "undefined", | ||
| "address,undefined", | ||
| "leak", | ||
| "thread", | ||
| "memory", | ||
| "none", | ||
| ], | ||
| default="none", | ||
| help="Enable compiler sanitizers. Defaults to 'none'.", | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggested change
| build_parser.add_argument( | |
| "--sanitize", | |
| choices=[ | |
| "address", | |
| "undefined", | |
| "address,undefined", | |
| "leak", | |
| "thread", | |
| "memory", | |
| "none", | |
| ], | |
| default="none", | |
| help="Enable compiler sanitizers. Defaults to 'none'.", | |
| ) | |
| build_parser.add_argument( | |
| "--sanitize", | |
| choices=[ | |
| "address", | |
| "undefined", | |
| "leak", | |
| "thread", | |
| "memory", | |
| "none", | |
| ], | |
| action="append", | |
| help="Enable compiler sanitizers. Defaults to 'none'.", | |
| ) |
Member
Author
There was a problem hiding this comment.
Arbitrary choices cannot be mixed. Meson errors if you try to do that. The only mix that is allowed is address,undefined. Hence I have gone with this approach.
| install_args.extend(COVERAGE_ARGS) | ||
|
|
||
| if sanitize: | ||
| install_args.append(f"-Csetup-args=-Db_sanitize={sanitize}") |
Contributor
There was a problem hiding this comment.
Suggested change
| install_args.append(f"-Csetup-args=-Db_sanitize={sanitize}") | |
| parsed_sanitize = ",".join(sanitize) if sanitize else "none" | |
| install_args.append(f"-Csetup-args=-Db_sanitize={parsed_sanitize}") |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #767
closes #766
In the first commit I added a
--sanitizeflag todev.pyto make it more convenient to enable sanitizers. While I was at it I also made editable rebuilds verbose by default (though one can still disable it with a--quietflag now).In the second commit I fixed all UBSAN issues and also added it to our CI. ASAN however catches too many issues and many of them seem to be from python, SDL and other dependencies of ours. So I don't think adding it to CI is feasible at all. However it would still be supported from
dev.pyincase one wants to go over the wall of errors and filter pygame-ce specific issues xD