From: Yann Collet <cyan@fb.com>
Date: Tue, 11 Mar 2025 18:58:16 +0000 (-0700)
Subject: re-design qsort() selection in cover
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=dcf675886bc138c03c3f157f7464798c4140829f;p=thirdparty%2Fzstd.git

re-design qsort() selection in cover

centralizes auto detection tests,
then distribute the outcome in all the places where it's active.
---

diff --git a/.github/workflows/dev-short-tests.yml b/.github/workflows/dev-short-tests.yml
index 2a97c32d0..164dc15c7 100644
--- a/.github/workflows/dev-short-tests.yml
+++ b/.github/workflows/dev-short-tests.yml
@@ -695,7 +695,7 @@ jobs:
           sudo apt install -y musl-tools
       - name: Compile the project with musl-gcc
         run: |
-          CC=musl-gcc CPPFLAGS=-DZSTD_USE_C90_QSORT make -j V=1 zstd
+          CC=musl-gcc CFLAGS="-Werror -O3" CPPFLAGS=-DZDICT_QSORT=ZDICT_QSORT_C90 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 9df516c5a..83bf00c19 100644
--- a/lib/README.md
+++ b/lib/README.md
@@ -193,9 +193,11 @@ 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 `ZSTD_USE_C90_QSORT` forces usage of C90's `qsort()`,
-  for situations where the code cannot determine that `qsort_r()` is not supported,
-  such as, for example, older versions of `musl`.
+- The macro `ZDICT_QSORT` can enforce selection of a specific sorting variant.
+  It can be notably set as `ZDICT_QSORT=ZDICT_QSORT_C90`,
+  for situations where autodetection fails,
+  for example with older versions of `musl`.
+  Other selectable suffixes are `_GNU`, `_APPLE` and `_MSVC`.
 
 #### Windows : using MinGW+MSYS to create DLL
 
diff --git a/lib/dictBuilder/cover.c b/lib/dictBuilder/cover.c
index ed750e31d..9ed360033 100644
--- a/lib/dictBuilder/cover.c
+++ b/lib/dictBuilder/cover.c
@@ -39,6 +39,7 @@
 #  define ZDICT_STATIC_LINKING_ONLY
 #endif
 
+#include "../common/debug.h" /* DEBUG_STATIC_ASSERT */
 #include "../common/mem.h" /* read */
 #include "../common/pool.h" /* POOL_ctx */
 #include "../common/threading.h" /* ZSTD_pthread_mutex_t */
@@ -60,6 +61,29 @@
 #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 ZDICT_QSORT_MAX
+#define ZDICT_QSORT_MAX 3
+
+#ifndef ZDICT_QSORT
+# if defined(__APPLE__)
+#   define ZDICT_QSORT ZDICT_QSORT_APPLE /* uses qsort_r() with a different order for parameters */
+# elif defined(_GNU_SOURCE)
+#   define ZDICT_QSORT ZDICT_QSORT_GNU /* uses qsort_r() */
+# elif defined(_WIN32) && defined(_MSC_VER)
+#   define ZDICT_QSORT ZDICT_QSORT_MSVC /* 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
 *
@@ -234,8 +258,7 @@ typedef struct {
   int displayLevel;
 } COVER_ctx_t;
 
-#if defined(ZSTD_USE_C90_QSORT) \
-  || (!defined(_GNU_SOURCE) && !defined(__APPLE__) && !defined(_MSC_VER))
+#if ZDICT_QSORT == ZDICT_QSORT_C90
 /* Use global context for non-reentrant sort functions */
 static COVER_ctx_t *g_coverCtx = NULL;
 #endif
@@ -282,9 +305,9 @@ static int COVER_cmp8(COVER_ctx_t *ctx, const void *lp, const void *rp) {
 /**
  * Same as COVER_cmp() except ties are broken by pointer value
  */
-#if (defined(_WIN32) && defined(_MSC_VER)) || defined(__APPLE__)
+#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 defined(_GNU_SOURCE) && !defined(ZSTD_USE_C90_QSORT)
+#elif (ZDICT_QSORT == ZDICT_QSORT_GNU)
 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) {
@@ -298,9 +321,9 @@ static int COVER_strict_cmp(const void *lp, const void *rp) {
 /**
  * Faster version for d <= 8.
  */
-#if (defined(_WIN32) && defined(_MSC_VER)) || defined(__APPLE__)
+#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 defined(_GNU_SOURCE) && !defined(ZSTD_USE_C90_QSORT)
+#elif (ZDICT_QSORT == ZDICT_QSORT_GNU)
 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) {
@@ -317,20 +340,26 @@ static int COVER_strict_cmp8(const void *lp, const void *rp) {
  * Hopefully when C11 become the norm, we will be able
  * to clean it up.
  */
-static void stableSort(COVER_ctx_t *ctx) {
-#if defined(__APPLE__)
+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 defined(_GNU_SOURCE) && !defined(ZSTD_USE_C90_QSORT)
+#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 defined(_WIN32) && defined(_MSC_VER)
+#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 defined(__OpenBSD__)
+    /* On OpenBSD, qsort() is not guaranteed to be stable, their mergesort() is.
+     * Note(@cyan): qsort() is never guaranteed to be stable,
+     *              so why would this property only matter for OpenBSD ?
+     */
     g_coverCtx = ctx;
     mergesort(ctx->suffix, ctx->suffixSize, sizeof(U32),
           (ctx->d <= 8 ? &COVER_strict_cmp8 : &COVER_strict_cmp));
@@ -782,7 +811,7 @@ ZDICTLIB_STATIC_API size_t ZDICT_trainFromBuffer_cover(
   BYTE* const dict = (BYTE*)dictBuffer;
   COVER_ctx_t ctx;
   COVER_map_t activeDmers;
-  const int displayLevel = parameters.zParams.notificationLevel;
+  const int displayLevel = (int)parameters.zParams.notificationLevel;
   parameters.splitPoint = 1.0;
   /* Checks */
   if (!COVER_checkParameters(parameters, dictBufferCapacity)) {