Add Wformat=2 and sanitize=undefined to the CI system#2101
Merged
Conversation
daviesrob
reviewed
Aug 19, 2024
| // there is little we can do other than elide this warning for this piece of | ||
| // code. This is probably the lesser of two evils. | ||
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wformat-nonliteral" |
Member
There was a problem hiding this comment.
An easier solution here would be to always pass the full format list to sscanf() and then use the return value to see how many of them it managed to fill out. Checking against num_columns should ensure that it hasn't been confused by whitespace other than tabs.
c57c6c1 to
c6ef58e
Compare
Contributor
Author
|
I think I've finally beaten it into submission, and fixed several ampliconclip bugs related to parsing of BED files too which were instigated by attempting to work around some of the more aggressive warnings. So not found as errors due to the warnings, but serendipity at play. |
This is enabled via -Wformat=2, but it causes a number of warnings. Some are trivial to fix, needing simply a format attribute, others less so. Specifically ampliconclip constructs a format string based on the number of columns detected in the bed file. Fundamentally this isn't compatible with the format checking. However Rob points out an easier way of doing this by looking at sscanf return values, and it turns out this also fixes a whole swathe of unrelated recent ampliconclip bugs. Also fixed a newly detected formatting problem (albeit 32-bit platforms only and only in error reporting) in tmp_file.c.
The size check for "num_columns > sizeof(scanf_components)" is wrong, meaning with more columns than 6 we had a read buffer overrun. The code also required tabs, but the BED format does not strictly state this. The original code prior to samtools#2033 just uses sscanf and no tab counting, which is better as it supports any white space and matches the BED spec (bar the weird oddity of permitting spaces in names if only single tabs are used, but that's a nasty case we have no desire to support). Instead we rely on the return value from sscanf to inform us of the (minimum) number of columns detected. Also amended the test data to incorporate up to 12 columns and down to 3 to validate the code can still parse correctly, plus spaces instead of tabs.
The -Wimplicit-fallthrough one spotted a genuine error too where the new "samtools split -p" accidentally also enabled --no-PG.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
As requested.
Wformat=2 is a bit of a pain, but undefined behaviour sanitizer seems valid to use. non-literal format checking is more questionable. It is finding bugs, although oddly in things that it ought to have been spotting before already (so maybe this coincided with a compiler version update on mingw), but it also has false positives to work around: i.e. places that aren't demonstrably the wrong arguments but cannot be validated at compile time are still considered to be wrong.