Warnings #66
Merged
Warnings #66
Conversation
This file contains 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
binaryfusefilter.h only issues addressed 1. changed some return value and parameter types of (static) functions -- PLEASE CHECK THIS IN REVIEW 2. sprinkled 'U' into bitwise operations to silence warnings 3. casting to avoid "standard integer promotion rules" which resulted in signedness warnings 4. explicitly reducing results to the target type rather than letting it happen implicitly tests still passing
binaryfusefilter.h only issues addressed 1. changed some return value and parameter types of (static) functions -- PLEASE CHECK THIS IN REVIEW 2. sprinkled 'U' into bitwise operations to silence warnings 3. casting to avoid "standard integer promotion rules" which resulted in signedness warnings 4. explicitly reducing results to the target type rather than letting it happen implicitly 5. when and `if` statements ends in break or return, then a following `else if` can be just a new `if` tests still passing
mostly casting size_t down to uint32_t - maybe some internal struct types should have been size_t? also some integer promotion casts
mostly casting blocklengt to uint32_t to fit into keyindex.index should keyindex.index be a size_t?
very repetitive casting of mainly sizes and doubles.
with -Wconversion and -Wsign-conversion so putting these in the Makefile, so during "private" development with the Makefile new warnings will be noticed straight away but not in CMakeLists.txt, because as this is a header-only INTERFACE library, it would force these warning levels on the users.
turned up these additional 'U' tweaks
all sections were indentical except for the call to *contain() and *size_in_bytes some void* and function pointer juggling allowed to make this generic report code reduced by 2/3rds
for all but the special "failure rate test" the large function dispatch table is a litle annoying, but can be removed as well...TBC tests all pass
just a missing free()
remove initialization. it's not needed and compiler now happy
instead of having a wrapper function per action per filter type, we can cast the functions as a generic function pointer on the into the generic test runner, and then cast them as a compatible function pointer type inside the test runner. The generic `filter*` parameter cannot be `void*` and must be a dummy struct because: § 6.2.5..28: "All pointers to structure types shall have the same representation and alignment requirements as each other" (the same is not true for `void*` which may have different representation. This simple change results in a large code reduction and removes the unsightly and hard to remove array of boiler plate function wrappers.
use ubuntu 24.04 run a matrix with both gcc and clang compile tests/unit target with full warnings use -Werror so any new warnings are kept out compile the test with sanitzers to catch runtime UBSAN/ASAN issues execute ctest run with env options to ensure UBSAN/ASAN warning result in CI failure
this CI build should now fail with UBSAN warning on clang (but would pass on gcc, and would pass on ubuntu 22.04 clang)
not on mingw as not supported
this was the motivaing origin of all the CI changes resolve the UBSAN warning due to recent LLVM changes in clang-17 UBSAN, which ap0plies much more strict interpretation of casting of function pointers where one of the parameters is a void* and therefore not "an exact match" for the function ultimately called. this is not a problem in practice at runtime as these pointers are binary compatible, but best to avoid the interpretation of the standard by clang UBSAN - as major projects have also had to do The warning is only generated at runtime with sanatizers from recent clang-17+ compiled in. Hence the fairly wide reaching changes to CI. Solution to the UBSAN warning is macro generated "thunks", ie simple function wrappers, which allow the compiler to be happy about the cast. Much more terse than spelling out the wrapper functions, but with all the benefits of conformance and clangd/IDE help with completion and type checks.
immediately caught by the new -Werror policy, MINGW32 CI build was failing because: - size_t is 32bit on MINGW32 - filter->blockLength is uint64_t so any calculation involving filter->blockLength and being stored in a size_t was causing an implicit cast which this commit makes explicit review whether to just make filter->blockLength a size_t so it auto adjusts to the correct size.
|
Merged. |
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.
Upgrade CI:
Note that none of the above changes affect builds by users including the library, they are changes to test and CI infrastructure only.
The motivating origin of these CI changes:
It turns out that my commit in pull request #65 "removing unnecessary boiler plate in unit.c" has subtle issues with the standard, which can be detected by the latest sanitizers. When I investigated why the testing and CI pipeline was not detecting it, I discovered that tests were compiled without warnings, and without sanitizers and also ctest was not being configured to to tell sanitizers to fail on warnings. Basically necessitating all of the above upgrades to build and CI.
Once all that was set up, and I could detect this subtle problem, I then also committed the fix for the actual issue:
Resolve the UBSAN warning due to recent LLVM changes in clang-17
UBSAN, which ap0plies much more strict interpretation of casting of
function pointers where one of the parameters is a void* and therefore
not "an exact match" for the function ultimately called.
this is not a problem in practice at runtime as these pointers are
binary compatible, but best to avoid the interpretation of the
standard by clang UBSAN - as major projects have also had to do
The warning is only generated at runtime with sanatizers from recent
clang-17+ compiled in. Hence the fairly wide reaching changes to CI.
Solution to the UBSAN warning is macro generated "thunks", ie simple
function wrappers, which allow the compiler to be happy about the
cast. Much more terse than spelling out the wrapper functions, but
with all the benefits of conformance and clangd/IDE help with
completion and type checks.
Plus The new setup then immediately picked up warnings in MINGW32 because of size_t being 32bit on that platform and probably that
filter->blockLengthshould besize_tand notuint64_tThe much stricter tests now passing including the updated sanitizer runs with newer gcc and now also clang.