Enable gcc undefined behavior sanitizer during coverage build #17409
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
Signed-off-by: Jeff Epler <jepler@gmail.com>
.. by ensuring the value to be shifted is an unsigned of the appropriate type. This fixes several runtime diagnostics such as ``` ../../py/binary.c:199:28: runtime error: left shift of 32768 by 16 places cannot be represented in type 'int' ``` Signed-off-by: Jeff Epler <jepler@gmail.com>
When an mpz number is empty, its digit pointer `dig` is NULL. If it is, then the result of decrementing it `d--` results in undefined behavior. When micropython is built to be "non-null compliant" (which is now a bit of a misnomer!), add an extra check for `d` to be non-NULL to prevent the diagnostic. Signed-off-by: Jeff Epler <jepler@gmail.com>
These sites all caused runtime diagnostics during the
unix coverage test. Fix them by calling the "0" variant
of the mem* function.
Additionally, in `mp_seq_replace_slice_no_grow` a
`do {} while(0)` construct is added to the macro, and a void cast
is added to explicitly denote that the return value of memmove0
is unused.
Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17409 +/- ##
==========================================
- Coverage 98.54% 98.53% -0.01%
==========================================
Files 169 169
Lines 21898 21626 -272
==========================================
- Hits 21579 21309 -270
+ Misses 319 317 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Code size report: |
|
Having done this I don't know how it contrasts with #15303 which I didn't take the time to properly review at the time and had forgotten about until I went looking just now. Duplicated effort? nah, never. |
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.
Summary
gcc's "undefined behavior" sanitizer can catch a range of misbehaviors at runtime that normally go unnoticed. These include integer and pointer operations that are "undefined" per the relevant C specification.
This PR fixes current undefined behavior detected under gcc 12.2.0 (debian stable/bookworm) on an x64 system, then enables it during unix coverage builds.
Testing
I built and ran the unix tests locally, iterating until there were no remaining diagnostics.
I double checked the places I used the
mem*0alternative functions for "passing the sniff test" (i.e., is it reasonably expected that zero-lengths would occur here, etc)Trade-offs and Alternatives
I believe that the core changes (memcpy0 etc) are implemented so that they avoid code growth except in the coverage build. (but the CI will tell us for sure)
Not all gcc sanitizers can be enabled simultaneously. So, a choice has to be mad (mainly between -fsanitize=undefined and -fsanitize=memory). I chose the undefined checker, but implemented it so that an override of the makefile variable is possible.
-fsanitize=memory is also nearly error free except for micropython/berkeley-db-1.xx#1
I read that a future C specifiction will make e.g.,
memset(NULL, 0, 0)(setting zero bytes of a NULL pointer) not-undefined-behavior. When this comes to pass, themem*0macros can possibly be removed.Different gcc versions might be too different in what they affect, as any diagnostics will make
make testfail due to the unexpected output.Well, except pattern-based tests which might inadvertently skip over output text that comes from the sanitizer, giving false negatives.