Use Tcl-only bitmap defrag coverage

.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}}

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 \

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

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 */

src/server.c

@@ -7932,7 +7932,6 @@ struct redisTest {
     {"ebuckets", ebucketsTest},
     {"vector", vectorTest},
     {"bitmap", bitopsTest},
-    {"bitmap-roaring", bitmapRoaringTest},
     {"rax", raxTest},
     {"zset", zsetTest},
     {"topk", chkTopKTest},

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)

tests/modules/croaring_collision.c

@@ -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;
-}

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" \
         ]

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}

tests/unit/moduleapi/croaring-symbols.tcl

@@ -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]
-        }
-    }
-}