From b9eab4e780f294fcab23d0db5c27bc6f21536db7 Mon Sep 17 00:00:00 2001 From: Jay Howard Date: Sun, 6 Sep 2026 17:56:29 -0500 Subject: [PATCH 1/2] dictBuilder: sort COVER dmers with a multi-key quicksort stableSort() reached qsort() through a five-variant selection block (ZDICT_QSORT_APPLE/GNU/MSVC/C11/C90) that picked between qsort_r(), qsort_s() and a non-reentrant global, plus the _GNU_SOURCE and __STDC_WANT_LIB_EXT1__ feature tests needed to expose them. Sort the dmers directly instead, with a multi-key quicksort (Bentley & Sedgewick): partition on a single dmer byte and recurse into the equal bucket one byte deeper. A byte proven equal is never examined again, which matters here because dmers are heavily duplicated and a comparison sort restarts every comparison at byte 0. The order produced is the same total order as before -- by dmer, then by position -- so dictionaries are unchanged. Byte order follows each comparator: COVER_cmp8() compares a masked little-endian U64, making byte d-1 most significant, while COVER_cmp() uses memcmp(), making byte 0 most significant. MEM_readLE64() byte-swaps on big-endian hosts, so this holds on every architecture. Recursion descends into the two smaller spans and loops on the largest. If L is the largest span and A is either other, A <= L and A + L <= n, so A <= n/2 and depth is bounded by log2(n) regardless of d or input shape. Measured maximum depth is 18 across corpora designed to stress it. The sort is in-place and allocates nothing, the comparison is inlined rather than reached through a function pointer, and the platform dispatch and its global are removed. With no qsort_r() left to detect, the ZDICT_QSORT build macro goes too: drop its entry from lib/README.md, and its override from the musl CI job, whose musl 1.2.2 has no qsort_r() and now builds without the override. Extend the existing COVER ordering test to d=6, where dmers shorter than 8 bytes are compared as a masked 8-byte word; that path had no test. Its expected hash comes from the qsort()-based code this replaces. --- .github/workflows/dev-short-tests.yml | 2 +- lib/README.md | 5 - lib/dictBuilder/cover.c | 224 +++++++++++++++----------- tests/fuzzer.c | 10 +- 4 files changed, 133 insertions(+), 108 deletions(-) diff --git a/.github/workflows/dev-short-tests.yml b/.github/workflows/dev-short-tests.yml index c4db0a4963b..fd5f4208b67 100644 --- a/.github/workflows/dev-short-tests.yml +++ b/.github/workflows/dev-short-tests.yml @@ -637,7 +637,7 @@ jobs: sudo apt install -y musl-tools - name: Compile with musl-gcc and test-zstd run: | - CC=musl-gcc CFLAGS="-Werror -O3" CPPFLAGS=-DZDICT_QSORT=ZDICT_QSORT_C90 make -j -C tests test-zstd V=1 + CC=musl-gcc CFLAGS="-Werror -O3" make -j -C tests test-zstd V=1 intel-cet-compatibility: runs-on: ubuntu-latest diff --git a/lib/README.md b/lib/README.md index aa92bd659da..18ffbd2756c 100644 --- a/lib/README.md +++ b/lib/README.md @@ -208,11 +208,6 @@ The file structure is designed to make this selection manually achievable for an and assembly decoding loops. You may want to use this macro if these loops are slower on your platform. -- The macro `ZDICT_QSORT` can enforce selection of a specific sorting variant, - which is useful when autodetection fails, for example with older versions of `musl`. - For this scenario, it can be set as `ZDICT_QSORT=ZDICT_QSORT_C90`. - Other selectable suffixes are `_GNU`, `_APPLE`, `_MSVC` and `_C11`. - #### Windows : using MinGW+MSYS to create DLL DLL can be created using MinGW+MSYS with the `make libzstd` command. diff --git a/lib/dictBuilder/cover.c b/lib/dictBuilder/cover.c index 83debc0da9b..c6825b64148 100644 --- a/lib/dictBuilder/cover.c +++ b/lib/dictBuilder/cover.c @@ -21,20 +21,8 @@ /*-************************************* * Dependencies ***************************************/ -/* qsort_r is an extension. - * - * Android NDK does not ship qsort_r(). - */ -#if (defined(__linux__) && !defined(__ANDROID__)) || defined(__CYGWIN__) || defined(__MSYS__) -# ifndef _GNU_SOURCE -# define _GNU_SOURCE -# endif -#endif - -#define __STDC_WANT_LIB_EXT1__ 1 /* request C11 Annex K, which includes qsort_s() */ - #include /* fprintf */ -#include /* malloc, free, qsort_r */ +#include /* malloc, free */ #include /* memset */ #include /* clock */ @@ -65,31 +53,6 @@ #define COVER_MAX_SAMPLES_SIZE (sizeof(size_t) == 8 ? ((unsigned)-1) : ((unsigned)1 GB)) #define COVER_DEFAULT_SPLITPOINT 1.0 -/** - * Select the qsort() variant used by cover - */ -#define ZDICT_QSORT_MIN 0 -#define ZDICT_QSORT_C90 ZDICT_QSORT_MIN -#define ZDICT_QSORT_GNU 1 -#define ZDICT_QSORT_APPLE 2 -#define ZDICT_QSORT_MSVC 3 -#define ZDICT_QSORT_C11 ZDICT_QSORT_MAX -#define ZDICT_QSORT_MAX 4 - -#ifndef ZDICT_QSORT -# if defined(__APPLE__) -# define ZDICT_QSORT ZDICT_QSORT_APPLE /* uses qsort_r() with a different order for parameters */ -# elif (defined(__linux__) && !defined(__ANDROID__)) || defined(__CYGWIN__) || defined(__MSYS__) -# define ZDICT_QSORT ZDICT_QSORT_GNU /* uses qsort_r() */ -# elif defined(_WIN32) && defined(_MSC_VER) -# define ZDICT_QSORT ZDICT_QSORT_MSVC /* uses qsort_s() with a different order for parameters */ -# elif defined(STDC_LIB_EXT1) && (STDC_LIB_EXT1 > 0) /* C11 Annex K */ -# define ZDICT_QSORT ZDICT_QSORT_C11 /* uses qsort_s() */ -# else -# define ZDICT_QSORT ZDICT_QSORT_C90 /* uses standard qsort() which is not re-entrant (requires global variable) */ -# endif -#endif - /*-************************************* * Console display @@ -265,11 +228,6 @@ typedef struct { int displayLevel; } COVER_ctx_t; -#if ZDICT_QSORT == ZDICT_QSORT_C90 -/* Use global context for non-reentrant sort functions */ -static COVER_ctx_t *g_coverCtx = NULL; -#endif - /*-************************************* * Helper functions ***************************************/ @@ -309,73 +267,145 @@ static int COVER_cmp8(COVER_ctx_t *ctx, const void *lp, const void *rp) { return (lhs > rhs); } +/* Ranges this small, with dmer bytes left to compare, are finished by insertion sort. */ +#define COVER_ISORT_CUTOFF 24 + /** - * Same as COVER_cmp() except ties are broken by the position each element holds + * Order by dmer, then by position. Only used for short ranges and for the + * final tie-break; the partitioning below compares raw bytes. + * `lvl` bytes are already known equal. */ -#if (ZDICT_QSORT == ZDICT_QSORT_MSVC) || (ZDICT_QSORT == ZDICT_QSORT_APPLE) -static int WIN_CDECL COVER_strict_cmp(void* g_coverCtx, const void* lp, const void* rp) { -#elif (ZDICT_QSORT == ZDICT_QSORT_GNU) || (ZDICT_QSORT == ZDICT_QSORT_C11) -static int COVER_strict_cmp(const void *lp, const void *rp, void *g_coverCtx) { -#else /* C90 fallback.*/ -static int COVER_strict_cmp(const void *lp, const void *rp) { -#endif - int result = COVER_cmp((COVER_ctx_t*)g_coverCtx, lp, rp); - if (result == 0) { - U32 const lpos = *(const U32 *)lp; - U32 const rpos = *(const U32 *)rp; - result = (lpos < rpos) ? -1 : (lpos > rpos) ? 1 : 0; +static int COVER_lessFrom(const COVER_ctx_t *ctx, U32 a, U32 b, unsigned lvl) +{ + if (ctx->d > 8) { + int const c = memcmp(ctx->samples + a + lvl, ctx->samples + b + lvl, + ctx->d - lvl); + if (c != 0) return c < 0; + } else { + U64 const mask = (ctx->d == 8) ? (U64)-1 : (((U64)1 << (8 * ctx->d)) - 1); + U64 const lhs = MEM_readLE64(ctx->samples + a) & mask; + U64 const rhs = MEM_readLE64(ctx->samples + b) & mask; + if (lhs != rhs) return lhs < rhs; + } + return a < b; +} + +static void COVER_insertionSort(const COVER_ctx_t *ctx, U32 *a, size_t n, + unsigned lvl) +{ + size_t i; + for (i = 1; i < n; ++i) { + U32 const v = a[i]; + size_t j = i; + while (j > 0 && COVER_lessFrom(ctx, v, a[j - 1], lvl)) { + a[j] = a[j - 1]; + --j; + } + a[j] = v; } - return result; } + /** - * Faster version for d <= 8. + * Every dmer in this range is identical, so only ascending position matters. + * Heapsort keeps this O(n log n) with no allocation. */ -#if (ZDICT_QSORT == ZDICT_QSORT_MSVC) || (ZDICT_QSORT == ZDICT_QSORT_APPLE) -static int WIN_CDECL COVER_strict_cmp8(void* g_coverCtx, const void* lp, const void* rp) { -#elif (ZDICT_QSORT == ZDICT_QSORT_GNU) || (ZDICT_QSORT == ZDICT_QSORT_C11) -static int COVER_strict_cmp8(const void *lp, const void *rp, void *g_coverCtx) { -#else /* C90 fallback.*/ -static int COVER_strict_cmp8(const void *lp, const void *rp) { -#endif - int result = COVER_cmp8((COVER_ctx_t*)g_coverCtx, lp, rp); - if (result == 0) { - U32 const lpos = *(const U32 *)lp; - U32 const rpos = *(const U32 *)rp; - result = (lpos < rpos) ? -1 : (lpos > rpos) ? 1 : 0; +static void COVER_siftByPos(U32 *a, size_t root, size_t n) +{ + for (;;) { + size_t child = 2 * root + 1; + if (child >= n) break; + if (child + 1 < n && a[child] < a[child + 1]) ++child; + if (a[root] >= a[child]) break; + { U32 const t = a[root]; a[root] = a[child]; a[child] = t; } + root = child; + } +} + +static void COVER_sortByPos(U32 *a, size_t n) +{ + size_t i; + if (n < 2) return; + for (i = n / 2; i > 0; --i) COVER_siftByPos(a, i - 1, n); + for (i = n; i > 1; --i) { + U32 const t = a[0]; a[0] = a[i - 1]; a[i - 1] = t; + COVER_siftByPos(a, 0, i - 1); } - return result; +} + +static BYTE COVER_med3(BYTE x, BYTE y, BYTE z) +{ + if (x < y) { if (y < z) return y; return (x < z) ? z : x; } + if (x < z) return x; + return (y < z) ? z : y; } /** - * Abstract away divergence of qsort_r() parameters. - * Hopefully when C11 become the norm, we will be able - * to clean it up. + * Sorts the partial suffix array with a multi-key quicksort (Bentley & + * Sedgewick): partition on a single dmer byte and recurse into the equal + * bucket one byte deeper, so a byte proven equal is never examined again. + * That matters here because dmers are heavily duplicated, and because a + * comparison sort restarts every comparison at byte 0. + * + * Elements are ordered by dmer, then by position in the input. + * COVER_group() depends on the second part, and positions are unique, so no + * two elements compare equal. + * + * In-place and allocation-free, and the comparison is inlined rather than + * reached through a function pointer. */ +static void COVER_mkqSort(const COVER_ctx_t *ctx, U32 *a, size_t n, unsigned lvl) +{ + const BYTE *const samples = ctx->samples; + unsigned const d = ctx->d; + for (;;) { + size_t lt, gt, i, nl, ne, ng, off; + BYTE pv; + if (n <= 1) return; + if (lvl >= d) { COVER_sortByPos(a, n); return; } + if (n <= COVER_ISORT_CUTOFF) { COVER_insertionSort(ctx, a, n, lvl); return; } + /* COVER_cmp8() compares a masked little-endian U64, which makes byte d-1 + * the most significant; COVER_cmp() uses memcmp(), which makes byte 0 the + * most significant. Walking bytes in the same order reproduces each + * comparator's ordering exactly. MEM_readLE64() byte-swaps on big-endian + * hosts, so this holds on every architecture. + * Hoisted out of the loop below: it depends only on d and lvl. */ + off = (d <= 8) ? (size_t)(d - 1 - lvl) : (size_t)lvl; + pv = COVER_med3(samples[a[0] + off], samples[a[n / 2] + off], + samples[a[n - 1] + off]); + lt = 0; gt = n; i = 0; + while (i < gt) { + BYTE const c = samples[a[i] + off]; + if (c < pv) { + U32 const t = a[i]; a[i] = a[lt]; a[lt] = t; ++lt; ++i; + } else if (c > pv) { + --gt; { U32 const t = a[i]; a[i] = a[gt]; a[gt] = t; } + } else { + ++i; + } + } + nl = lt; ne = gt - lt; ng = n - gt; + /* Recurse into the two smaller spans and loop on the largest. If L is the + * largest and A is either other span, A <= L and A + L <= n, so A <= n/2: + * recursion depth is bounded by log2(n) whatever d or the data look like. */ + if (ne >= nl && ne >= ng) { + if (nl) COVER_mkqSort(ctx, a, nl, lvl); + if (ng) COVER_mkqSort(ctx, a + gt, ng, lvl); + a += lt; n = ne; ++lvl; + } else if (nl >= ng) { + if (ne) COVER_mkqSort(ctx, a + lt, ne, lvl + 1); + if (ng) COVER_mkqSort(ctx, a + gt, ng, lvl); + n = nl; + } else { + if (ne) COVER_mkqSort(ctx, a + lt, ne, lvl + 1); + if (nl) COVER_mkqSort(ctx, a, nl, lvl); + a += gt; n = ng; + } + } +} + static void stableSort(COVER_ctx_t *ctx) { - DEBUG_STATIC_ASSERT(ZDICT_QSORT_MIN <= ZDICT_QSORT && ZDICT_QSORT <= ZDICT_QSORT_MAX); -#if (ZDICT_QSORT == ZDICT_QSORT_APPLE) - qsort_r(ctx->suffix, ctx->suffixSize, sizeof(U32), - ctx, - (ctx->d <= 8 ? &COVER_strict_cmp8 : &COVER_strict_cmp)); -#elif (ZDICT_QSORT == ZDICT_QSORT_GNU) - qsort_r(ctx->suffix, ctx->suffixSize, sizeof(U32), - (ctx->d <= 8 ? &COVER_strict_cmp8 : &COVER_strict_cmp), - ctx); -#elif (ZDICT_QSORT == ZDICT_QSORT_MSVC) - qsort_s(ctx->suffix, ctx->suffixSize, sizeof(U32), - (ctx->d <= 8 ? &COVER_strict_cmp8 : &COVER_strict_cmp), - ctx); -#elif (ZDICT_QSORT == ZDICT_QSORT_C11) - qsort_s(ctx->suffix, ctx->suffixSize, sizeof(U32), - (ctx->d <= 8 ? &COVER_strict_cmp8 : &COVER_strict_cmp), - ctx); -#else /* C90 fallback.*/ - g_coverCtx = ctx; - /* TODO(cavalcanti): implement a reentrant qsort() when _r is not available. */ - qsort(ctx->suffix, ctx->suffixSize, sizeof(U32), - (ctx->d <= 8 ? &COVER_strict_cmp8 : &COVER_strict_cmp)); -#endif + COVER_mkqSort(ctx, ctx->suffix, ctx->suffixSize, 0); } /** diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 9f9e8a1c191..63c46b1c83b 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -3666,22 +3666,22 @@ static int basicUnitTests(U32 const seed, double compressibility) DISPLAYLEVEL(3, "OK, created dictionary of size %u \n", (unsigned)dictSize); DISPLAYLEVEL(3, "test%3i : COVER dictBuilder orders equal dmers by position : ", testNb++); - /* COVER sorts its suffix array with qsort(), breaking ties between + /* COVER sorts its suffix array by dmer, breaking ties between * equal dmers by sample position; COVER_group() relies on that order * to count how many samples contain a dmer. A tie-break that depends on * the sort's internal state instead (the former address comparison) * still produces a dictionary, just a different one. Low-entropy * samples make ties frequent, so train on them and compare against the - * dictionary the position order produces, for both comparator paths. */ + * dictionary the position order produces, on each comparison path. */ { size_t const dictCapacity = 16 KB; void* const coverDict = malloc(dictCapacity); - unsigned const dValues[2] = { 8, 16 }; /* COVER_cmp8() and COVER_cmp() */ - XXH64_hash_t const expectedHash[2] = { 0x77EB0C1D595FF7A3ULL, 0xE02EC0FD3A5BBF6DULL }; + unsigned const dValues[3] = { 6, 8, 16 }; /* masked 8-byte compare, full 8-byte compare, memcmp() */ + XXH64_hash_t const expectedHash[3] = { 0x54CA5F766977CEA4ULL, 0x77EB0C1D595FF7A3ULL, 0xE02EC0FD3A5BBF6DULL }; unsigned dIdx; if (coverDict==NULL) goto _output_error; RDG_genBuffer(decodedBuffer, totalSampleSize, 0.5, 0.05, 1); /* low-entropy: many repeated dmers */ { U32 u; for (u=0; u Date: Tue, 10 Nov 2026 23:38:08 -0600 Subject: [PATCH 2/2] common: remove leftovers of the qsort_r dispatch zstd_deps.h defined _GNU_SOURCE on Linux, Cygwin and MSYS solely to expose qsort_r() to the dictionary builder, as its comment said. compiler.h defined WIN_CDECL for the comparators the dictionary builder passed to MSVC's qsort_s(). The COVER sort no longer calls either, nothing else uses them, and an internal header should not opt every file that includes it into GNU extensions. Cleanup only: the previous commit does not depend on this one. --- lib/common/compiler.h | 11 ----------- lib/common/zstd_deps.h | 12 ------------ 2 files changed, 23 deletions(-) diff --git a/lib/common/compiler.h b/lib/common/compiler.h index 5e70570ec7a..80fca3e4d27 100644 --- a/lib/common/compiler.h +++ b/lib/common/compiler.h @@ -42,17 +42,6 @@ #endif -/** - On MSVC qsort requires that functions passed into it use the __cdecl calling conversion(CC). - This explicitly marks such functions as __cdecl so that the code will still compile - if a CC other than __cdecl has been made the default. -*/ -#if defined(_MSC_VER) -# define WIN_CDECL __cdecl -#else -# define WIN_CDECL -#endif - /* UNUSED_ATTR tells the compiler it is okay if the function is unused. */ #if defined(__GNUC__) || defined(__IAR_SYSTEMS_ICC__) # define UNUSED_ATTR __attribute__((unused)) diff --git a/lib/common/zstd_deps.h b/lib/common/zstd_deps.h index 8a9c7cc5313..4d767ae9b05 100644 --- a/lib/common/zstd_deps.h +++ b/lib/common/zstd_deps.h @@ -24,18 +24,6 @@ #ifndef ZSTD_DEPS_COMMON #define ZSTD_DEPS_COMMON -/* Even though we use qsort_r only for the dictionary builder, the macro - * _GNU_SOURCE has to be declared *before* the inclusion of any standard - * header and the script 'combine.sh' combines the whole zstd source code - * in a single file. - */ -#if defined(__linux) || defined(__linux__) || defined(linux) || defined(__gnu_linux__) || \ - defined(__CYGWIN__) || defined(__MSYS__) -#if !defined(_GNU_SOURCE) && !defined(__ANDROID__) /* NDK doesn't ship qsort_r(). */ -#define _GNU_SOURCE -#endif -#endif - #include #include #include