Merge pull request #10603 from ThomasWaldmann/chunker-review-fixes

chunkers: review fixes (chunking_time precision, param checks, cleanups)

Author: ThomasWaldmann

src/borg/chunkers/base.pxd

@@ -18,7 +18,7 @@ cdef class ChunkerBase:
     cdef int done, eof
     cdef size_t min_size, buf_size, remaining, position, last
     cdef long long bytes_read, bytes_yielded
-    cdef readonly float chunking_time
+    cdef readonly double chunking_time  # a sum of many small increments, a C float would stop growing
     cdef object file_reader
     cdef size_t reader_block_size
     cdef bint sparse

src/borg/chunkers/base.pyx

@@ -50,8 +50,12 @@ cdef class ChunkerBase:
         """
         min_size = 1 << chunk_min_exp
         max_size = 1 << chunk_max_exp
-        assert max_size <= len(zeros)
-        assert min_size + 1 <= max_size, "too small max_size"
+        # The parameter checks here and in the subclasses raise ValueError rather than
+        # assert: the scan code relies on them for memory safety and python -O drops asserts.
+        if max_size > len(zeros):
+            raise ValueError("too big max_size")
+        if min_size + 1 > max_size:
+            raise ValueError("too small max_size")
 
         self.algo = algo
         self.chunk_mask = _mask_bits(hash_mask_bits, high_masks)
@@ -61,9 +65,10 @@ cdef class ChunkerBase:
         # probability). This concentrates chunk sizes around the target and reduces
         # chunk-size variance. nc_level == 0 disables it, keeping behavior byte-identical
         # to the single-mask chunker.
-        assert nc_level >= 0
-        assert hash_mask_bits - nc_level >= 1, "nc_level too large for hash_mask_bits"
-        assert hash_mask_bits + nc_level <= 48, "nc_level too large for hash_mask_bits"
+        if nc_level < 0:
+            raise ValueError("nc_level must not be negative")
+        if hash_mask_bits - nc_level < 1 or hash_mask_bits + nc_level > 48:
+            raise ValueError("nc_level too large for hash_mask_bits")
         self.nc_level = nc_level
         if nc_level:
             self.mask_s = _mask_bits(hash_mask_bits + nc_level, high_masks)
@@ -152,10 +157,9 @@ cdef class ChunkerBase:
         cdef uint64_t digest = 0, mask, mask_s = self.mask_s, mask_l = self.mask_l
         cdef int nc_level = self.nc_level
         cdef size_t n, old_last, min_size = self.min_size
-        cdef size_t normal_size = self.normal_size, normal_pos, chunk_len, span, did
+        cdef size_t normal_size = self.normal_size, chunk_len, span, did
         cdef int64_t r
         cdef uint8_t* p
-        cdef uint8_t* stop
 
         if self.done:
             if self.bytes_read == self.bytes_yielded:
@@ -202,14 +206,13 @@ cdef class ChunkerBase:
                 continue
 
             p = self.data + self.position
-            stop = p + self.remaining
-            if nc_level and chunk_len < normal_size:
-                # do not scan past the strict->loose transition; re-evaluate the mask there
-                normal_pos = self.last + normal_size
-                if (self.data + normal_pos) < stop:
-                    stop = self.data + normal_pos
-
-            span = stop - p
+            span = self.remaining
+            if nc_level and chunk_len < normal_size and normal_size - chunk_len < span:
+                # do not scan past the strict->loose transition; re-evaluate the mask there.
+                # This compares lengths, not positions: normal_size is a caller-given size_t
+                # and can be anything, self.last + normal_size might not even be representable.
+                span = normal_size - chunk_len
+
             r = self._scan(p, span, &digest, mask)
             if r == -2:
                 raise Exception(f"{self.algo}: OpenSSL EVP encryption failed")

src/borg/chunkers/buzhash.pyx

@@ -138,7 +138,10 @@ cdef class Chunker(ChunkerBase):
     def __cinit__(self, int seed, int chunk_min_exp, int chunk_max_exp, int hash_mask_bits, int hash_window_size, bint sparse=False):
         self.table = NULL
         # see process, first while loop condition, first term must be able to get True:
-        assert hash_window_size + (1 << chunk_min_exp) + 1 <= (1 << chunk_max_exp), "too small max_size"
+        if hash_window_size < 0:
+            raise ValueError("hash_window_size must not be negative")
+        if hash_window_size + (1 << chunk_min_exp) + 1 > (1 << chunk_max_exp):
+            raise ValueError("too small max_size")
         self.window_size = hash_window_size
         self.table = buzhash_init_table(seed & 0xffffffff)
         # buzhash is single-mask (no normalized chunking): nc_level 0 makes
@@ -167,7 +170,7 @@ cdef class Chunker(ChunkerBase):
             else:
                 raise Exception("chunkifier byte count mismatch")
 
-        while self.remaining < min_size + window_size + 1 and not self.eof:  # see assert in Chunker init
+        while self.remaining < min_size + window_size + 1 and not self.eof:  # see check in __cinit__
             self.fill()
 
         # Here we either are at eof...

src/borg/chunkers/buzhash64.pyx

@@ -150,7 +150,10 @@ cdef class ChunkerBuzHash64(ChunkerBase):
         self.table = NULL
         self.table_rot = NULL
         # see process, first while loop condition, first term must be able to get True:
-        assert hash_window_size + (1 << chunk_min_exp) + 1 <= (1 << chunk_max_exp), "too small max_size"
+        if hash_window_size < 0:
+            raise ValueError("hash_window_size must not be negative")
+        if hash_window_size + (1 << chunk_min_exp) + 1 > (1 << chunk_max_exp):
+            raise ValueError("too small max_size")
 
         self.window_size = hash_window_size
         self.table = buzhash64_init_table(key)
@@ -191,10 +194,8 @@ cdef class ChunkerBuzHash64(ChunkerBase):
         cdef uint64_t mask_s = self.mask_s, mask_l = self.mask_l
         cdef int nc_level = self.nc_level
         cdef size_t n, old_last, min_size = self.min_size, window_size = self.window_size
-        cdef size_t normal_size = self.normal_size, normal_pos
+        cdef size_t normal_size = self.normal_size, chunk_len
         cdef uint8_t* p
-        cdef uint8_t* stop_at
-        cdef uint8_t* nc_stop
         cdef size_t did_bytes, span
         cdef int kernel_id = self.kernel_id
 
@@ -204,7 +205,7 @@ cdef class ChunkerBuzHash64(ChunkerBase):
             else:
                 raise Exception("chunkifier byte count mismatch")
 
-        while self.remaining < min_size + window_size + 1 and not self.eof:  # see assert in Chunker init
+        while self.remaining < min_size + window_size + 1 and not self.eof:  # see check in __cinit__
             self.fill()
 
         # Here we either are at eof...
@@ -231,29 +232,28 @@ cdef class ChunkerBuzHash64(ChunkerBase):
         # Normalized chunking: pick the mask based on how far we are into the current chunk.
         # While below normal_size use the strict mask (lower cut probability), afterward the
         # loose mask (higher cut probability). The mask is re-evaluated at the top of every
-        # iteration, so the transition is honored exactly at normal_pos. When nc is disabled,
-        # mask_s == mask_l == chunk_mask and the normal_pos cap is not applied, so this reduces
+        # iteration, so the transition is honored exactly at normal_size. When nc is disabled,
+        # mask_s == mask_l == chunk_mask and the normal_size cap is not applied, so this reduces
         # to the original single-mask behavior.
         mask = mask_s
-        normal_pos = 0
+        chunk_len = 0
         while True:
             if nc_level:
-                normal_pos = self.last + normal_size
-                mask = mask_s if self.position < normal_pos else mask_l
+                chunk_len = self.position - self.last
+                mask = mask_s if chunk_len < normal_size else mask_l
 
             if not (self.remaining > window_size and (sum & mask) and not (self.eof and self.remaining <= window_size)):
                 break
 
             p = self.data + self.position
-            stop_at = p + self.remaining - window_size
+            span = self.remaining - window_size
 
-            if nc_level and self.position < normal_pos:
-                # do not scan past the strict->loose transition; re-evaluate the mask there
-                nc_stop = self.data + normal_pos
-                if nc_stop < stop_at:
-                    stop_at = nc_stop
+            if nc_level and chunk_len < normal_size and normal_size - chunk_len < span:
+                # do not scan past the strict->loose transition; re-evaluate the mask there.
+                # This compares lengths, not positions: normal_size is a caller-given size_t
+                # and can be anything, self.last + normal_size might not even be representable.
+                span = normal_size - chunk_len
 
-            span = stop_at - p
             with nogil:
                 did_bytes = bz64_scan(self.table, self.table_rot, p, p + window_size,
                                       span, &sum, mask, kernel_id)

src/borg/chunkers/buzhash64_impl.c

@@ -428,14 +428,10 @@ int bz64_kernel_select(const char *name, int *out_id)
         return BZ_KSEL_OK;
     }
     if (strcmp(name, "avx512") == 0) {
-#ifndef BZ_KIND_512
-        return BZ_KSEL_NOTBUILT; /* compiler too old for the target attribute */
-#else
         if (!__builtin_cpu_supports("avx512f"))
             return BZ_KSEL_NOCPU;
         *out_id = BZ_K_VECTOR512;
         return BZ_KSEL_OK;
-#endif
     }
 #endif
     return BZ_KSEL_UNKNOWN;

src/borg/chunkers/buzhash64_impl.h

@@ -32,7 +32,7 @@
 /* Results of bz64_kernel_select(). */
 #define BZ_KSEL_OK 0
 #define BZ_KSEL_UNKNOWN 1  /* not a kernel name on this platform */
-#define BZ_KSEL_NOTBUILT 2 /* known, but not compiled into this binary */
+#define BZ_KSEL_NOTBUILT 2 /* known, but not compiled into this binary (never returned, see fastcdc_impl.h) */
 #define BZ_KSEL_NOCPU 3    /* known and built, but this CPU cannot run it */
 
 /* Resolve a kernel name for this build; see fc_kernel_select(). */

src/borg/chunkers/fastcdc_impl.c

@@ -409,14 +409,10 @@ int fc_kernel_select(const char *name, int *out_id)
         return FC_KSEL_OK;
     }
     if (strcmp(name, "avx512") == 0) {
-#ifndef FC_KIND_512
-        return FC_KSEL_NOTBUILT; /* compiler too old for the target attribute */
-#else
         if (!__builtin_cpu_supports("avx512f"))
             return FC_KSEL_NOCPU;
         *out_id = FC_K_VECTOR512;
         return FC_KSEL_OK;
-#endif
     }
 #endif
     return FC_KSEL_UNKNOWN;

src/borg/chunkers/fastcdc_impl.h

@@ -28,13 +28,16 @@
 /* Results of fc_kernel_select(). */
 #define FC_KSEL_OK 0
 #define FC_KSEL_UNKNOWN 1  /* not a kernel name on this platform */
-#define FC_KSEL_NOTBUILT 2 /* known, but not compiled into this binary */
+#define FC_KSEL_NOTBUILT 2 /* known, but not compiled into this binary (see below) */
 #define FC_KSEL_NOCPU 3    /* known and built, but this CPU cannot run it */
 
 /* Resolve a kernel name for this build. On FC_KSEL_OK the id is stored in
- * *out_id, otherwise *out_id is left alone. The three failures are kept apart
- * because they need different fixes: a typo, too old a compiler, or the wrong
- * CPU. */
+ * *out_id, otherwise *out_id is left alone. The failures are kept apart
+ * because they need different fixes: a typo or the wrong CPU.
+ * FC_KSEL_NOTBUILT (too old a compiler) is never returned: a compiler that can
+ * build this file at all builds all of a platform's kernels. The code exists
+ * because the kernel selectors share their result codes (see kernel_env.py)
+ * and the AES chunkers' selector does return it. */
 int fc_kernel_select(const char *name, int *out_id);
 
 /* Comma-separated list of the kernel names this build accepts, for error

src/borg/chunkers/goldilocks_aes_impl.c

@@ -171,7 +171,7 @@ uint64_t gl_digest64(const GL_CTX *c, const uint8_t *q)
 #define PH_DIGEST64 gl_digest64
 #include "phte_scan.h"
 
-GL_CTX *gl_new(const uint64_t tables[GL_TABLES * 256], uint64_t k1, uint64_t k2,
+GL_CTX *gl_new(const uint64_t *tables, uint64_t k1, uint64_t k2,
                const uint8_t aes_key[16], int kernel)
 {
     GL_CTX *c = calloc(1, sizeof(GL_CTX));

src/borg/chunkers/phte_chunker.pyx

@@ -33,7 +33,8 @@ cdef class ChunkerPHTE(ChunkerBase):
         # the rolling window needs 64 bytes of in-chunk history at the first
         # possible cut position (min_size), so the minimum chunk size must be
         # at least the window size:
-        assert chunk_min_exp >= 6, "chunk_min_exp must be >= 6 (2^6 = 64 = window size)"
+        if chunk_min_exp < 6:
+            raise ValueError("chunk_min_exp must be >= 6 (2^6 = 64 = window size)")
         self.kernel_str = "unknown"
         # the AES output is uniform, so contiguous low-bit masks are fine
         return self._setup_common(algo, chunk_min_exp, chunk_max_exp, hash_mask_bits,