diff --git a/.github/workflows/daily.yml b/.github/workflows/daily.yml index efa7d6289..fd067686c 100644 --- a/.github/workflows/daily.yml +++ b/.github/workflows/daily.yml @@ -1398,17 +1398,9 @@ jobs: repository: ${{ env.GITHUB_REPOSITORY }} ref: ${{ env.GITHUB_HEAD_REF }} - name: make - run: make SANITIZER=address DEBUG_DEFRAG=force REDIS_CFLAGS='-Werror -fsanitize=pointer-subtract' + run: make SANITIZER=address DEBUG_DEFRAG=force REDIS_CFLAGS='-Werror' - name: testprep run: sudo apt-get install tcl8.6 tclx - - name: test bitmap bitset defrag pointer safety - if: true && !contains(github.event.inputs.skiptests, 'redis') - env: - ASAN_OPTIONS: detect_invalid_pointer_pairs=1 - run: > - ./runtest --debug-defrag --verbose --clients 1 - --single unit/bitmap-native - --only "forced active defrag relocates dense bitmap BITSET storage safely" - name: test if: true && !contains(github.event.inputs.skiptests, 'redis') run: ./runtest --debug-defrag --verbose --clients 1 ${{github.event.inputs.test_args}} diff --git a/runtest-moduleapi b/runtest-moduleapi index 69ac77619..bcb53c653 100755 --- a/runtest-moduleapi +++ b/runtest-moduleapi @@ -22,7 +22,6 @@ $MAKE -C tests/modules && \ $TCLSH tests/test_helper.tcl \ --single unit/moduleapi/commandfilter \ --single unit/moduleapi/basics \ ---single unit/moduleapi/croaring-symbols \ --single unit/moduleapi/fork \ --single unit/moduleapi/testrdb \ --single unit/moduleapi/infotest \ diff --git a/src/bitmap_roaring.c b/src/bitmap_roaring.c index 1aa9932bd..e51aa49b4 100644 --- a/src/bitmap_roaring.c +++ b/src/bitmap_roaring.c @@ -37,9 +37,6 @@ #include "bitmap_roaring.h" #include "lzf.h" #include "rdb.h" -#ifdef REDIS_TEST -#include "testhelp.h" -#endif /* Reuse the established string BITOP vector kernels after bounded native * operands have been materialized. Their scalar tail remains local below. */ @@ -618,28 +615,11 @@ static bitmapObject *bitmapObjectActiveDefragSelf(bitmapObject *bitmap) { return newbitmap; } -static size_t bitmapObjectBitsetWordsOffset(const void *base) { - uintptr_t aligned = ((uintptr_t)base + sizeof(void *) + - (BITMAP_ROARING_BITSET_WORDS_ALIGNMENT - 1)) & - ~((uintptr_t)BITMAP_ROARING_BITSET_WORDS_ALIGNMENT - 1); - return (size_t)(aligned - (uintptr_t)base); -} - -static uint64_t *bitmapObjectRelocateBitsetWords(void *newbase, size_t old_off) { - size_t new_off = bitmapObjectBitsetWordsOffset(newbase); - uint64_t *words = (uint64_t *)((char *)newbase + new_off); - - if (new_off != old_off) - memmove(words, (char *)newbase + old_off, - sizeof(uint64_t) * BITSET_CONTAINER_SIZE_IN_WORDS); - ((void **)words)[-1] = newbase; - return words; -} - static void bitmapObjectDefragBitsetWords(bitmapObject *bitmap, bitset_container_t *bitset) { void *base, *newbase; - size_t old_off; + uintptr_t aligned; + size_t old_off, new_off; if (bitset->words == NULL) return; base = bitmapRoaringAlignedAllocBase(bitset->words); @@ -652,7 +632,15 @@ static void bitmapObjectDefragBitsetWords(bitmapObject *bitmap, /* The relocation copied the block verbatim, so the words still sit at * their old offset; re-derive the aligned offset for the new base address * and slide the words when the two differ. */ - bitset->words = bitmapObjectRelocateBitsetWords(newbase, old_off); + aligned = ((uintptr_t)newbase + sizeof(void *) + + (BITMAP_ROARING_BITSET_WORDS_ALIGNMENT - 1)) & + ~((uintptr_t)BITMAP_ROARING_BITSET_WORDS_ALIGNMENT - 1); + new_off = (size_t)(aligned - (uintptr_t)newbase); + if (new_off != old_off) + memmove((char *)newbase + new_off, (char *)newbase + old_off, + sizeof(uint64_t) * BITSET_CONTAINER_SIZE_IN_WORDS); + ((void **)aligned)[-1] = newbase; + bitset->words = (uint64_t *)aligned; } /* Defrag one container; returns the moved container pointer, or NULL if the @@ -1663,60 +1651,3 @@ bitop_result: o->encoding = OBJ_ENCODING_BITMAP_ROARING; return o; } - -#ifdef REDIS_TEST -static int bitmapRoaringTestBitsetRelocation(size_t old_off, - size_t expected_new_off) { - const size_t words_size = - sizeof(uint64_t) * BITSET_CONTAINER_SIZE_IN_WORDS; - const size_t extra = BITMAP_ROARING_BITSET_WORDS_ALIGNMENT - 1 + - sizeof(void *); - const size_t storage_size = words_size + extra + - BITMAP_ROARING_BITSET_WORDS_ALIGNMENT; - unsigned char *storage = zcalloc(storage_size); - unsigned char *newbase = NULL; - uint64_t *words; - int valid = 1; - - for (size_t i = 0; i < BITMAP_ROARING_BITSET_WORDS_ALIGNMENT; i++) { - unsigned char *candidate = storage + i; - if (bitmapObjectBitsetWordsOffset(candidate) == expected_new_off) { - newbase = candidate; - break; - } - } - if (newbase == NULL) { - zfree(storage); - return 0; - } - - for (size_t i = 0; i < words_size; i++) - newbase[old_off + i] = (unsigned char)((i * 37 + 11) & 0xff); - - words = bitmapObjectRelocateBitsetWords(newbase, old_off); - valid = valid && (unsigned char *)words == newbase + expected_new_off; - valid = valid && ((uintptr_t)words % - BITMAP_ROARING_BITSET_WORDS_ALIGNMENT) == 0; - valid = valid && bitmapRoaringAlignedAllocBase(words) == newbase; - for (size_t i = 0; valid && i < words_size; i++) - valid = ((unsigned char *)words)[i] == - (unsigned char)((i * 37 + 11) & 0xff); - - zfree(storage); - return valid; -} - -int bitmapRoaringTest(int argc, char **argv, int flags) { - UNUSED(argc); - UNUSED(argv); - UNUSED(flags); - - test_cond("BITSET relocation preserves words when alignment moves left", - bitmapRoaringTestBitsetRelocation(32, 16)); - test_cond("BITSET relocation preserves words when alignment moves right", - bitmapRoaringTestBitsetRelocation(16, 32)); - test_cond("BITSET relocation preserves words when alignment is unchanged", - bitmapRoaringTestBitsetRelocation(16, 16)); - return 0; -} -#endif diff --git a/src/bitmap_roaring.h b/src/bitmap_roaring.h index 395c119c8..9e3a3a120 100644 --- a/src/bitmap_roaring.h +++ b/src/bitmap_roaring.h @@ -59,8 +59,4 @@ sds bitmapObjectMaterialize(const robj *o); sds bitmapObjectMaterializeForRDB(const robj *o); robj *bitmapObjectsBitop(bitmapBitop op, robj **objects, size_t numkeys, uint64_t maxlen); -#ifdef REDIS_TEST -int bitmapRoaringTest(int argc, char **argv, int flags); -#endif - #endif /* __BITMAP_ROARING_H */ diff --git a/src/server.c b/src/server.c index eef566eaf..ec586b395 100644 --- a/src/server.c +++ b/src/server.c @@ -7932,7 +7932,6 @@ struct redisTest { {"ebuckets", ebucketsTest}, {"vector", vectorTest}, {"bitmap", bitopsTest}, - {"bitmap-roaring", bitmapRoaringTest}, {"rax", raxTest}, {"zset", zsetTest}, {"topk", chkTopKTest}, diff --git a/tests/modules/Makefile b/tests/modules/Makefile index 4341708e7..a20d9530a 100644 --- a/tests/modules/Makefile +++ b/tests/modules/Makefile @@ -89,10 +89,6 @@ TEST_MODULES = \ postnotifications_perkey_metadata.so \ atomicslotmigration.so -ifeq ($(uname_S),Linux) -TEST_MODULES += croaring_collision.so -endif - .PHONY: all all: $(TEST_MODULES) diff --git a/tests/modules/croaring_collision.c b/tests/modules/croaring_collision.c deleted file mode 100644 index 1f27a5c27..000000000 --- a/tests/modules/croaring_collision.c +++ /dev/null @@ -1,58 +0,0 @@ -/* Module used to verify that symbols from Redis' vendored CRoaring archive do - * not interpose on a module that defines the same symbol. */ - -#include "redismodule.h" - -typedef struct roaring64_bitmap_s roaring64_bitmap_t; - -static unsigned char module_bitmap_sentinel; -static int module_bitmap_free_count; - -/* These are intentionally named after CRoaring API symbols linked into Redis. - * The volatile function pointers below keep the module references eligible - * for ELF symbol interposition, matching a module that embeds CRoaring. */ -roaring64_bitmap_t *roaring64_bitmap_create(void) { - return (roaring64_bitmap_t *)&module_bitmap_sentinel; -} - -void roaring64_bitmap_free(roaring64_bitmap_t *bitmap) { - if (bitmap == (roaring64_bitmap_t *)&module_bitmap_sentinel) - module_bitmap_free_count++; -} - -static roaring64_bitmap_t *(*volatile module_bitmap_create)(void) = - roaring64_bitmap_create; -static void (*volatile module_bitmap_free)(roaring64_bitmap_t *) = - roaring64_bitmap_free; - -static int CroaringCollisionResolvesLocally(RedisModuleCtx *ctx, - RedisModuleString **argv, - int argc) { - int free_count_before = module_bitmap_free_count; - roaring64_bitmap_t *bitmap = module_bitmap_create(); - int resolves_locally; - - REDISMODULE_NOT_USED(argv); - REDISMODULE_NOT_USED(argc); - - resolves_locally = - bitmap == (roaring64_bitmap_t *)&module_bitmap_sentinel; - module_bitmap_free(bitmap); - resolves_locally = resolves_locally && - module_bitmap_free_count == free_count_before + 1; - return RedisModule_ReplyWithLongLong(ctx, resolves_locally); -} - -int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { - REDISMODULE_NOT_USED(argv); - REDISMODULE_NOT_USED(argc); - - if (RedisModule_Init(ctx, "croaring_collision", 1, - REDISMODULE_APIVER_1) == REDISMODULE_ERR) - return REDISMODULE_ERR; - if (RedisModule_CreateCommand(ctx, - "croaring_collision.resolves-locally", - CroaringCollisionResolvesLocally, "", 0, 0, 0) == REDISMODULE_ERR) - return REDISMODULE_ERR; - return REDISMODULE_OK; -} diff --git a/tests/support/server.tcl b/tests/support/server.tcl index 0bedf20e0..c4e24aa54 100644 --- a/tests/support/server.tcl +++ b/tests/support/server.tcl @@ -436,12 +436,8 @@ proc spawn_server {config_file stdout stderr args} { # ASAN_OPTIONS environment variable is for address sanitizer. If a test # tries to allocate huge memory area and expects allocator to return # NULL, address sanitizer throws an error without this setting. - set asan_options "allocator_may_return_null=1" - if {[info exists ::env(ASAN_OPTIONS)] && $::env(ASAN_OPTIONS) ne ""} { - set asan_options "$::env(ASAN_OPTIONS):$asan_options" - } set env [list \ - "ASAN_OPTIONS=$asan_options" \ + "ASAN_OPTIONS=allocator_may_return_null=1" \ "MSAN_OPTIONS=allocator_may_return_null=1" \ "TSAN_OPTIONS=allocator_may_return_null=1,detect_deadlocks=0,suppressions=src/tsan.sup" \ ] diff --git a/tests/unit/bitmap-native.tcl b/tests/unit/bitmap-native.tcl index 0c3d175d0..8e5235b75 100644 --- a/tests/unit/bitmap-native.tcl +++ b/tests/unit/bitmap-native.tcl @@ -969,15 +969,24 @@ start_server {tags {"bitmap" "bitmap-native" "needs:debug" "cluster:skip"}} { wait_for_bitmap_defrag_stop 500 10 r config resetstat - # Alternating bits exceed ARRAY capacity and cannot be compressed - # usefully as runs, forcing a BITSET container with an independently - # aligned words allocation. - set dense [string repeat [binary format H* aa] 8192] - r set bitmap:defrag:dense $dense - convert_string_bitmap_to_native r bitmap:defrag:dense - assert_equal bitmap [r type bitmap:defrag:dense] - assert_equal 32768 [r bitcount bitmap:defrag:dense] - assert_morethan [r memory usage bitmap:defrag:dense] 8192 + # Alternating bit patterns exceed ARRAY capacity and cannot be + # compressed usefully as runs, forcing independently aligned + # BITSET word allocations. Vary the contents across 16 containers + # so forced defrag performs many distinct relocations. + set chunks [list \ + [string repeat [binary format H* aa] 8192] \ + [string repeat [binary format H* 55] 8192] \ + [string repeat [binary format H* cc] 8192]] + set dense "" + for {set i 0} {$i < 16} {incr i} { + append dense [lindex $chunks [expr {$i % [llength $chunks]}]] + } + set key bitmap:defrag:dense + r set $key $dense + convert_string_bitmap_to_native r $key + assert_equal bitmap [r type $key] + assert_equal [expr {[string length $dense] * 4}] [r bitcount $key] + assert_morethan [r memory usage $key] [string length $dense] set old_hz [config_get_set hz 100] set old_threshold [config_get_set active-defrag-threshold-lower 1] @@ -1004,10 +1013,10 @@ start_server {tags {"bitmap" "bitmap-native" "needs:debug" "cluster:skip"}} { r config set active-defrag-cycle-max $old_cycle_max r config set active-defrag-cycle-min $old_cycle_min r config set active-defrag-ignore-bytes $old_ignore_bytes - assert_equal bitmap [r type bitmap:defrag:dense] - assert_equal 32768 [r bitcount bitmap:defrag:dense] - assert_equal $dense [r debug bitmap-raw bitmap:defrag:dense] - assert_equal 1 [r del bitmap:defrag:dense] + assert_equal bitmap [r type $key] + assert_equal [expr {[string length $dense] * 4}] [r bitcount $key] + assert_equal $dense [r debug bitmap-raw $key] + assert_equal 1 [r del $key] r config set bitmap-default-native $old_bitmap_default_native assert_equal OK [r config set activedefrag $old_activedefrag] } {} {needs:config-resetstat} diff --git a/tests/unit/moduleapi/croaring-symbols.tcl b/tests/unit/moduleapi/croaring-symbols.tcl deleted file mode 100644 index 830186885..000000000 --- a/tests/unit/moduleapi/croaring-symbols.tcl +++ /dev/null @@ -1,15 +0,0 @@ -set testmodule [file normalize tests/modules/croaring_collision.so] - -if {$::tcl_platform(os) eq "Linux"} { - start_server {tags {"modules external:skip"}} { - r module load $testmodule - - test {vendored CRoaring symbols do not interpose module symbols} { - assert_equal 1 [r croaring_collision.resolves-locally] - } - - test {unload the CRoaring collision module} { - assert_equal OK [r module unload croaring_collision] - } - } -}