Skip to content

Commit

Permalink
Warnings (#66)
Browse files Browse the repository at this point in the history
* first cut at reducing warnings

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

* first cut at reducing warnings

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

* starting work on xofilter.h

* binclude/binaryfusefilter.h apparently clean for first time

* formatting

* first cut on xofilter.h

mostly casting size_t down to uint32_t - maybe some internal struct
types should have been size_t?

also some integer promotion casts

* round2 on xorfilter.h

mostly casting blocklengt to uint32_t to fit into keyindex.index

should keyindex.index be a size_t?

* bench.c and unit.c

very repetitive casting of mainly sizes and doubles.

* all silent now on a clean compile

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.

* another sweep from including c++ project

turned up these additional 'U' tweaks

* mistaken cast which broke test

* factoring out the report functionality

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

* iron out slight inconsistencies between tests

* abstracting away the rest of the test logic

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

* fixing a memory leak caught by sanitizer

just a missing free()

* _duplicates test cases can be convered by the same code

remove initialization. it's not needed and compiler now happy

* removing the need for large array of boiler plate function wrappers

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.

* upgrade CI

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

* change options only

* correct compile and link options for test

this CI build should now fail with UBSAN warning on clang (but would
pass on gcc, and would pass on ubuntu 22.04 clang)

* syntax

* remove whitespce after \

* use sanitizers only on *nix

not on mingw as not supported

* fix the UBSAN warning

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.

* really skip saninitizers unless not mingw

* fixing warnings on MINGW32

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.
  • Loading branch information
oschonrock authored Dec 16, 2024
1 parent 872a8bb commit 9f5b776
Show file tree
Hide file tree
Showing 5 changed files with 157 additions and 112 deletions.
25 changes: 20 additions & 5 deletions .github/workflows/ubuntu.yml
Original file line number Diff line number Diff line change
@@ -1,19 +1,34 @@
name: Ubuntu 22.04 CI (GCC 11)
name: Ubuntu 24.04 (gcc-13, clang-18)

on: [push, pull_request]

jobs:
ubuntu-build:
runs-on: ubuntu-22.04
runs-on: ubuntu-24.04
strategy:
matrix:
compiler: [gcc, clang]
steps:
- uses: actions/checkout@v3
- name: Use cmake
- name: checkout code
uses: actions/checkout@v4
- name: build with cmake
run: |
if [ "${{ matrix.compiler }}" == "gcc" ]; then
export CC=gcc
export CXX=g++
elif [ "${{ matrix.compiler }}" == "clang" ]; then
export CC=clang
export CXX=clang++
fi
mkdir build &&
cd build &&
cmake .. -DCMAKE_INSTALL_PREFIX:PATH=destination &&
cmake --build . &&
ctest --output-on-failure &&
# force failure if sanitizers report any warnings
env \
ASAN_OPTIONS='halt_on_error=1:abort_on_error=1:print_summary=1' \
UBSAN_OPTIONS='halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1' \
ctest --output-on-failure &&
cmake --install . &&
cd ../tests/installation_tests/find &&
mkdir build && cd build && cmake -DCMAKE_INSTALL_PREFIX:PATH=../../../build/destination .. && cmake --build .
9 changes: 5 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
all: unit bench

unit : tests/unit.c include/xorfilter.h include/binaryfusefilter.h
cc -std=c99 -O3 -o unit tests/unit.c -lm -Iinclude -Wall -Wextra -Wshadow -Wcast-qual -Wconversion -Wsign-conversion

${CC} -std=c99 -g -O2 -fsanitize=address,leak,undefined -o unit tests/unit.c -lm -Iinclude -Wall -Wextra -Wshadow -Wcast-qual -Wconversion -Wsign-conversion -Werror

ab : tests/a.c tests/b.c
cc -std=c99 -o c tests/a.c tests/b.c -lm -Iinclude -Wall -Wextra -Wshadow -Wcast-qual -Wconversion -Wsign-conversion
${CC} -std=c99 -o c tests/a.c tests/b.c -lm -Iinclude -Wall -Wextra -Wshadow -Wcast-qual -Wconversion -Wsign-conversion

bench : benchmarks/bench.c include/xorfilter.h include/binaryfusefilter.h
cc -std=c99 -O3 -o bench benchmarks/bench.c -lm -Iinclude -Wall -Wextra -Wshadow -Wcast-qual -Wconversion -Wsign-conversion
${CC} -std=c99 -O3 -o bench benchmarks/bench.c -lm -Iinclude -Wall -Wextra -Wshadow -Wcast-qual -Wconversion -Wsign-conversion

test: unit ab
ASAN_OPTIONS='halt_on_error=1:abort_on_error=1:print_summary=1' \
UBSAN_OPTIONS='halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1' \
./unit

clean:
Expand Down
36 changes: 18 additions & 18 deletions include/xorfilter.h
Original file line number Diff line number Diff line change
Expand Up @@ -157,12 +157,12 @@ static inline bool xor16_allocate(uint32_t size, xor16_t *filter) {

// report memory usage
static inline size_t xor8_size_in_bytes(const xor8_t *filter) {
return 3 * filter->blockLength * sizeof(uint8_t) + sizeof(xor8_t);
return 3 * (size_t)(filter->blockLength) * sizeof(uint8_t) + sizeof(xor8_t);
}

// report memory usage
static inline size_t xor16_size_in_bytes(const xor16_t *filter) {
return 3 * filter->blockLength * sizeof(uint16_t) + sizeof(xor16_t);
return 3 * (size_t)(filter->blockLength) * sizeof(uint16_t) + sizeof(xor16_t);
}

// release memory
Expand Down Expand Up @@ -449,9 +449,9 @@ static inline bool xor8_buffered_populate(uint64_t *keys, uint32_t size, xor8_t
if(size == 0) { return false; }
uint64_t rng_counter = 1;
filter->seed = xor_rng_splitmix64(&rng_counter);
size_t arrayLength = filter->blockLength * 3; // size of the backing array
size_t arrayLength = (size_t)(filter->blockLength) * 3; // size of the backing array
xor_setbuffer_t buffer0, buffer1, buffer2;
size_t blockLength = filter->blockLength;
size_t blockLength = (size_t)(filter->blockLength);
bool ok0 = xor_init_buffer(&buffer0, blockLength);
bool ok1 = xor_init_buffer(&buffer1, blockLength);
bool ok2 = xor_init_buffer(&buffer2, blockLength);
Expand Down Expand Up @@ -660,8 +660,8 @@ static inline bool xor8_populate(uint64_t *keys, uint32_t size, xor8_t *filter)
if(size == 0) { return false; }
uint64_t rng_counter = 1;
filter->seed = xor_rng_splitmix64(&rng_counter);
size_t arrayLength = filter->blockLength * 3; // size of the backing array
size_t blockLength = filter->blockLength;
size_t arrayLength = (size_t)(filter->blockLength) * 3; // size of the backing array
size_t blockLength = (size_t)(filter->blockLength);

xor_xorset_t *sets =
(xor_xorset_t *)malloc(arrayLength * sizeof(xor_xorset_t));
Expand Down Expand Up @@ -867,9 +867,9 @@ static inline bool xor16_buffered_populate(uint64_t *keys, uint32_t size, xor16_
if(size == 0) { return false; }
uint64_t rng_counter = 1;
filter->seed = xor_rng_splitmix64(&rng_counter);
size_t arrayLength = filter->blockLength * 3; // size of the backing array
size_t arrayLength = (size_t)(filter->blockLength) * 3; // size of the backing array
xor_setbuffer_t buffer0, buffer1, buffer2;
size_t blockLength = filter->blockLength;
size_t blockLength = (size_t)(filter->blockLength);
bool ok0 = xor_init_buffer(&buffer0, blockLength);
bool ok1 = xor_init_buffer(&buffer1, blockLength);
bool ok2 = xor_init_buffer(&buffer2, blockLength);
Expand Down Expand Up @@ -1081,8 +1081,8 @@ static inline bool xor16_populate(uint64_t *keys, uint32_t size, xor16_t *filter
if(size == 0) { return false; }
uint64_t rng_counter = 1;
filter->seed = xor_rng_splitmix64(&rng_counter);
size_t arrayLength = filter->blockLength * 3; // size of the backing array
size_t blockLength = filter->blockLength;
size_t arrayLength = (size_t)(filter->blockLength) * 3; // size of the backing array
size_t blockLength = (size_t)(filter->blockLength);

xor_xorset_t *sets =
(xor_xorset_t *)malloc(arrayLength * sizeof(xor_xorset_t));
Expand Down Expand Up @@ -1282,12 +1282,12 @@ static inline bool xor16_populate(uint64_t *keys, uint32_t size, xor16_t *filter

static inline size_t xor16_serialization_bytes(xor16_t *filter) {
return sizeof(filter->seed) + sizeof(filter->blockLength) +
sizeof(uint16_t) * 3 * filter->blockLength;
sizeof(uint16_t) * 3 * (size_t)(filter->blockLength);
}

static inline size_t xor8_serialization_bytes(const xor8_t *filter) {
return sizeof(filter->seed) + sizeof(filter->blockLength) +
sizeof(uint8_t) * 3 * filter->blockLength;
sizeof(uint8_t) * 3 * (size_t)(filter->blockLength);
}

// serialize a filter to a buffer, the buffer should have a capacity of at least
Expand All @@ -1298,7 +1298,7 @@ static inline void xor16_serialize(const xor16_t *filter, char *buffer) {
buffer += sizeof(filter->seed);
memcpy(buffer, &filter->blockLength, sizeof(filter->blockLength));
buffer += sizeof(filter->blockLength);
memcpy(buffer, filter->fingerprints, filter->blockLength * 3 * sizeof(uint16_t));
memcpy(buffer, filter->fingerprints, (size_t)(filter->blockLength) * 3 * sizeof(uint16_t));
}

// serialize a filter to a buffer, the buffer should have a capacity of at least
Expand All @@ -1309,7 +1309,7 @@ static inline void xor8_serialize(const xor8_t *filter, char *buffer) {
buffer += sizeof(filter->seed);
memcpy(buffer, &filter->blockLength, sizeof(filter->blockLength));
buffer += sizeof(filter->blockLength);
memcpy(buffer, filter->fingerprints, filter->blockLength * 3 * sizeof(uint8_t));
memcpy(buffer, filter->fingerprints, (size_t)(filter->blockLength) * 3 * sizeof(uint8_t));
}

// deserialize a filter from a buffer, returns true on success, false on failure.
Expand All @@ -1322,11 +1322,11 @@ static inline bool xor16_deserialize(xor16_t * filter, const char *buffer) {
buffer += sizeof(filter->seed);
memcpy(&filter->blockLength, buffer, sizeof(filter->blockLength));
buffer += sizeof(filter->blockLength);
filter->fingerprints = (uint16_t*)malloc(filter->blockLength * 3 * sizeof(uint16_t));
filter->fingerprints = (uint16_t*)malloc((size_t)(filter->blockLength) * 3 * sizeof(uint16_t));
if(filter->fingerprints == NULL) {
return false;
}
memcpy(filter->fingerprints, buffer, filter->blockLength * 3 * sizeof(uint16_t));
memcpy(filter->fingerprints, buffer, (size_t)(filter->blockLength) * 3 * sizeof(uint16_t));
return true;
}

Expand All @@ -1341,11 +1341,11 @@ static inline bool xor8_deserialize(xor8_t * filter, const char *buffer) {
buffer += sizeof(filter->seed);
memcpy(&filter->blockLength, buffer, sizeof(filter->blockLength));
buffer += sizeof(filter->blockLength);
filter->fingerprints = (uint8_t*)malloc(filter->blockLength * 3 * sizeof(uint8_t));
filter->fingerprints = (uint8_t*)malloc((size_t)(filter->blockLength) * 3 * sizeof(uint8_t));
if(filter->fingerprints == NULL) {
return false;
}
memcpy(filter->fingerprints, buffer, filter->blockLength * 3 * sizeof(uint8_t));
memcpy(filter->fingerprints, buffer, (size_t)(filter->blockLength) * 3 * sizeof(uint8_t));
return true;
}

Expand Down
23 changes: 22 additions & 1 deletion tests/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,24 @@
add_executable(unit unit.c)
add_test(unit unit)
target_link_libraries(unit PUBLIC xor_singleheader)
target_link_libraries(unit PRIVATE xor_singleheader)


# full warnings with sanitizers for tests. Include debug symbols and
# only -O2 to maintain some debugability. -Werror to
# prevent new warning creeping in Matches Makefile
if (MSVC)
# limited support for MSVC, this is not tested
list(APPEND TEST_COMPILE_OPTIONS /W4 /fsanitize=address)
else() # *nix
list(APPEND TEST_COMPILE_OPTIONS -g -O2
-Wall -Wextra -Wshadow -Wcast-qual -Wconversion -Wsign-conversion -Werror)

if (NOT MINGW) # sanitizers are not supported under mingw
list(APPEND TEST_COMPILE_OPTIONS -fsanitize=address,undefined,leak)
# sanitsizers need to be specified at link time as well
target_link_options(unit PRIVATE -fsanitize=address,leak,undefined)
endif()
endif()

target_compile_options(unit PRIVATE ${TEST_COMPILE_OPTIONS})

Loading

0 comments on commit 9f5b776

Please sign in to comment.