Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions .github/workflows/freebsd.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
name: FreeBSD CI

# Triggers the workflow on push or pull request or on demand
on:
workflow_dispatch:
push:
pull_request:
branches: [ develop ]
paths-ignore:
- '.github/CODEOWNERS'
- '.github/FUNDING.yml'
- 'doc/**'
- 'release_docs/**'
- 'ACKNOWLEDGEMENTS'
- 'LICENSE**'
- '**.md'

# Using concurrency to cancel any in-progress job or run
concurrency:
group: ${{ github.workflow }}-${{ github.head_ref && github.ref || github.run_id }}
cancel-in-progress: true

permissions:
contents: read

jobs:
freebsd-build-and-test:
runs-on: ubuntu-latest
name: FreeBSD ${{ matrix.freebsd-version }} Build and Test

# Don't run the action if the commit message says to skip CI
if: "!contains(github.event.head_commit.message, 'skip-ci')"

strategy:
fail-fast: false
matrix:
freebsd-version: ['14.3', '15.0']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add 13.5 back, as long as we plan to test that


steps:
- name: Checkout repository
uses: actions/checkout@v5

- name: Build and test on FreeBSD
uses: vmactions/freebsd-vm@v1
with:
release: ${{ matrix.freebsd-version }}
usesh: true
prepare: |
pkg install -y cmake ninja pkgconf bash curl
run: |
set -e
# Configure the build
mkdir build
cd build
cmake -C ../config/cmake/cacheinit.cmake \
-G Ninja \
--log-level=VERBOSE \
-DCMAKE_BUILD_TYPE=Release \
-DBUILD_SHARED_LIBS:BOOL=ON \
-DHDF5_ENABLE_ALL_WARNINGS:BOOL=ON \
-DHDF5_ENABLE_PARALLEL:BOOL=OFF \
-DHDF5_BUILD_CPP_LIB:BOOL=ON \
-DHDF5_BUILD_FORTRAN:BOOL=OFF \
-DHDF5_BUILD_JAVA:BOOL=OFF \
-DHDF5_BUILD_DOC:BOOL=OFF \
-DHDF5_ENABLE_ZLIB_SUPPORT:BOOL=ON \
-DHDF5_ENABLE_SZIP_SUPPORT:BOOL=ON \
-DLIBAEC_USE_LOCALCONTENT:BOOL=OFF \
-DZLIB_USE_LOCALCONTENT:BOOL=OFF \
-DHDF5_TEST_API:BOOL=ON \
-DHDF5_TEST_SHELL_SCRIPTS:BOOL=OFF \
-DENABLE_EXTENDED_TESTS:BOOL=OFF \
..
echo ""
# Build
cmake --build . --parallel 3 --config Release
echo ""
# Run tests
ctest . --parallel 2 -C Release -V
echo ""
95 changes: 95 additions & 0 deletions .github/workflows/test-qsort-fallback.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
name: Test qsort_r Fallback

# Triggers the workflow on push or pull request or on demand
on:
workflow_dispatch:
push:
pull_request:
branches: [ develop ]
paths-ignore:
- '.github/CODEOWNERS'
- '.github/FUNDING.yml'
- 'doc/**'
- 'release_docs/**'
- 'ACKNOWLEDGEMENTS'
- 'LICENSE**'
- '**.md'

# Using concurrency to cancel any in-progress job or run
concurrency:
group: ${{ github.workflow }}-${{ github.head_ref && github.ref || github.run_id }}
cancel-in-progress: true

permissions:
contents: read

jobs:
test-qsort-fallback:
runs-on: ${{ matrix.os }}
name: Test qsort_r fallback on ${{ matrix.os }}

strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]

# Don't run the action if the commit message says to skip CI
if: "!contains(github.event.head_commit.message, 'skip-ci')"

steps:
- name: Checkout repository
uses: actions/checkout@v5

- name: Force qsort fallback by modifying ConfigureChecks.cmake
run: |
sed -i.bak '/CHECK_FUNCTION_EXISTS (qsort_r _HAVE_QSORT_R_TMP)/,/^endif ()$/{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with testing the fallback function, but it probably doesn't need a completely separate workflow run everywhere just to test something that won't be used in most places. Instead, I think this should probably be driven by testing on some platform we support or want/plan to support where we know the fallback function is going to be used.

/CHECK_FUNCTION_EXISTS (qsort_r _HAVE_QSORT_R_TMP)/c\
# Force qsort fallback for testing\
set(${HDF_PREFIX}_HAVE_QSORT_REENTRANT 0)
/CHECK_FUNCTION_EXISTS (qsort_s _HAVE_QSORT_S_TMP)/d
/if (_HAVE_QSORT_R_TMP OR _HAVE_QSORT_S_TMP)/d
/set (${HDF_PREFIX}_HAVE_QSORT_REENTRANT 1)/d
/^endif ()$/d
}' config/ConfigureChecks.cmake
shell: bash

- name: Set up build dependencies (Ubuntu)
if: runner.os == 'Linux'
run: |
sudo apt-get update
sudo apt-get install -y cmake gcc g++ zlib1g-dev
- name: Set up build dependencies (macOS)
if: runner.os == 'macOS'
run: |
brew install cmake
- name: Set up build dependencies (Windows)
if: runner.os == 'Windows'
uses: microsoft/setup-msbuild@v2

- name: Configure and build HDF5
run: |
mkdir build
cd build
cmake -C ../config/cmake/cacheinit.cmake \
-DCMAKE_BUILD_TYPE=Release \
-DBUILD_SHARED_LIBS:BOOL=ON \
-DHDF5_ENABLE_PARALLEL:BOOL=OFF \
-DHDF5_BUILD_CPP_LIB:BOOL=OFF \
-DHDF5_BUILD_FORTRAN:BOOL=OFF \
-DHDF5_BUILD_JAVA:BOOL=OFF \
-DHDF5_BUILD_DOC:BOOL=OFF \
-DHDF5_ENABLE_ZLIB_SUPPORT:BOOL=ON \
-DHDF5_ENABLE_SZIP_SUPPORT:BOOL=OFF \
-DHDF5_TEST_API:BOOL=ON \
-DHDF5_TEST_SHELL_SCRIPTS:BOOL=OFF \
..
cmake --build . --config Release --parallel 4
shell: bash

- name: Run tests with fallback implementation
run: |
cd build
ctest --output-on-failure --parallel 4 -C Release
shell: bash
7 changes: 7 additions & 0 deletions config/ConfigureChecks.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,13 @@ CHECK_FUNCTION_EXISTS (asprintf ${HDF_PREFIX}_HAVE_ASPRINTF)
CHECK_FUNCTION_EXISTS (vasprintf ${HDF_PREFIX}_HAVE_VASPRINTF)
CHECK_FUNCTION_EXISTS (waitpid ${HDF_PREFIX}_HAVE_WAITPID)

# Check for reentrant qsort variants (qsort_r on Unix/BSD, qsort_s on Windows)
CHECK_FUNCTION_EXISTS (qsort_r _HAVE_QSORT_R_TMP)
CHECK_FUNCTION_EXISTS (qsort_s _HAVE_QSORT_S_TMP)
if (_HAVE_QSORT_R_TMP OR _HAVE_QSORT_S_TMP)
set (${HDF_PREFIX}_HAVE_QSORT_REENTRANT 1)
endif ()

#-----------------------------------------------------------------------------
# sigsetjmp is special; may actually be a macro
#-----------------------------------------------------------------------------
Expand Down
13 changes: 12 additions & 1 deletion src/H5private.h
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,11 @@ H5_DLL void HDqsort_context(void *base, size_t nel, size_t size,
int (*compar)(const void *, const void *, void *), void *arg);
#endif

#ifndef H5_HAVE_QSORT_REENTRANT
H5_DLL void HDqsort_fallback(void *base, size_t nel, size_t size,
int (*compar)(const void *, const void *, void *), void *arg);
#endif

#ifndef HDfseek
#define HDfseek(F, O, W) fseeko(F, O, W)
#endif
Expand Down Expand Up @@ -770,11 +775,17 @@ H5_DLL void HDqsort_context(void *base, size_t nel, size_t size,
#define HDunsetenv(S) unsetenv(S)
#endif
#ifndef HDqsort_r
#ifdef H5_HAVE_DARWIN
#ifdef H5_HAVE_QSORT_REENTRANT
#if defined(H5_HAVE_DARWIN) || (defined(__FreeBSD__) && __FreeBSD__ < 14)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the near future we should probably move this into something we try to check at configure time. I imagine there are other platforms that use this signature and checking at configure time should get rid of platform-specific #ifdefs and also avoid needing the (probably minor) overhead of a fallback function. Maybe something like https://gitlab.gnome.org/GNOME/glib/-/blob/2.30.0/configure.ac?ref_type=tags#L589-627.

/* Darwin and FreeBSD < 14 use BSD-style qsort_r with different signature/argument order */
#define HDqsort_r(B, N, S, C, A) HDqsort_context(B, N, S, C, A)
#else
#define HDqsort_r(B, N, S, C, A) qsort_r(B, N, S, C, A)
#endif
#else
/* No native qsort_r/qsort_s available - use fallback implementation */
#define HDqsort_r(B, N, S, C, A) HDqsort_fallback(B, N, S, C, A)
#endif
#endif
#ifndef HDvasprintf
#ifdef H5_HAVE_VASPRINTF
Expand Down
3 changes: 3 additions & 0 deletions src/H5pubconf.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,9 @@
/* Define to 1 if you have the <pwd.h> header file. */
#cmakedefine H5_HAVE_PWD_H @H5_HAVE_PWD_H@

/* Define to 1 if you have a reentrant qsort function (qsort_r or qsort_s). */
#cmakedefine H5_HAVE_QSORT_REENTRANT @H5_HAVE_QSORT_REENTRANT@

/* Define whether the Read-Only S3 virtual file driver (VFD) should be
compiled */
#cmakedefine H5_HAVE_ROS3_VFD @H5_HAVE_ROS3_VFD@
Expand Down
96 changes: 96 additions & 0 deletions src/H5system.c
Original file line number Diff line number Diff line change
Expand Up @@ -1456,3 +1456,99 @@ HDqsort_context(void *base, size_t nel, size_t size, int (*compar)(const void *,
#endif
}
#endif

/*
* HDqsort_fallback - Fallback qsort implementation for platforms without qsort_r/qsort_s
*
* For platforms that don't provide any reentrant qsort variant, this fallback uses
* thread-local storage (when thread-safety is
* enabled) or a global variable to store the comparator context, then uses standard qsort().
*
* The threadsafe version uses thread-local storage but does not support recursive sorting (a
* comparator calling HDqsort_r) as it would overwrite the previous context.
*/
#ifndef H5_HAVE_QSORT_REENTRANT

typedef struct HDqsort_fallback_context_t {
int (*gnu_compar)(const void *, const void *, void *);
void *gnu_arg;
} HDqsort_fallback_context_t;

#ifdef H5_HAVE_THREADSAFE
/* Thread-local storage approach for thread-safe builds */
static H5TS_key_t HDqsort_fallback_key;
static H5TS_once_t HDqsort_fallback_key_once = H5TS_ONCE_INITIALIZER;

static void
HDqsort_fallback_key_init(void)
{
/* Create the thread-local storage key (no destructor needed) */
H5TS_key_create(&HDqsort_fallback_key, NULL);
}

static int
HDqsort_fallback_wrapper(const void *a, const void *b)
{
HDqsort_fallback_context_t *ctx = NULL;

/* Retrieve the context from thread-local storage
* This should never fail since we just set it in HDqsort_fallback() */
if (H5_UNLIKELY(H5TS_key_get_value(HDqsort_fallback_key, (void **)&ctx) < 0))
return 0; /* Should never happen, but return 0 (equal) if it does */

/* Call the original GNU-style comparator with context */
return ctx->gnu_compar(a, b, ctx->gnu_arg);
}

void
HDqsort_fallback(void *base, size_t nel, size_t size, int (*compar)(const void *, const void *, void *),
void *arg)
{
HDqsort_fallback_context_t ctx;

/* Ensure the TLS key is initialized */
H5TS_once(&HDqsort_fallback_key_once, HDqsort_fallback_key_init);

ctx.gnu_compar = compar;
ctx.gnu_arg = arg;

/* Store context in thread-local storage */
H5TS_key_set_value(HDqsort_fallback_key, &ctx);

qsort(base, nel, size, HDqsort_fallback_wrapper);

/* Clear the thread-local storage */
H5TS_key_set_value(HDqsort_fallback_key, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HDF5 code always checks H5TS_key_set_value return values, for example:

src/H5TSrec_rwlock.c:
if (H5_UNLIKELY(H5TS_key_set_value(lock->rec_read_lock_count_key, (void *)count) < 0))

src/H5TSint.c:
if (H5_UNLIKELY(H5TS_key_set_value(H5TS_thrd_info_key_g, tinfo_node))) {
    // Error handling
}

But

H5TS_key_create(&HDqsort_fallback_key, NULL);        // No check
H5TS_key_set_value(HDqsort_fallback_key, &ctx);      // No check 
H5TS_key_set_value(HDqsort_fallback_key, NULL);      // No check

}

#else
/* Non-threadsafe: use global variable */
static HDqsort_fallback_context_t *HDqsort_fallback_global_ctx = NULL;

static int
HDqsort_fallback_wrapper(const void *a, const void *b)
{
/* Call the original GNU-style comparator with context from global */
return HDqsort_fallback_global_ctx->gnu_compar(a, b, HDqsort_fallback_global_ctx->gnu_arg);
}

void
HDqsort_fallback(void *base, size_t nel, size_t size, int (*compar)(const void *, const void *, void *),
void *arg)
{
HDqsort_fallback_context_t ctx;

ctx.gnu_compar = compar;
ctx.gnu_arg = arg;

/* Store context in global variable */
HDqsort_fallback_global_ctx = &ctx;

qsort(base, nel, size, HDqsort_fallback_wrapper);

/* Clear the global pointer */
HDqsort_fallback_global_ctx = NULL;
}
#endif /* H5_HAVE_THREADSAFE */

#endif /* !H5_HAVE_QSORT_REENTRANT */
Loading