Skip to content
Open
Show file tree
Hide file tree
Changes from 9 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
121 changes: 121 additions & 0 deletions src/H5system.c
Original file line number Diff line number Diff line change
Expand Up @@ -1456,3 +1456,124 @@ 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
*
* Note: The functions below use assertions instead of the HDF5 error stack because
* qsort and its variants return void, preventing propagation of errors.
*/
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)
{
herr_t ret = SUCCEED;

/* Create the thread-local storage key (no destructor needed) */
ret = H5TS_key_create(&HDqsort_fallback_key, NULL);

/* Assert that initialization succeeded - cannot propagate errors from here */
if (H5_UNLIKELY(ret < 0)) {
assert(false && "Failed to create TLS key for qsort fallback");
Copy link
Collaborator

Choose a reason for hiding this comment

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

We must avoid assertions in main library code for cases that can realistically occur, even if that means ignoring errors (though ideally we don't do that either). In this case though, it looks like ret isn't assigned anyway, so this doesn't seem to be a useful check, unless ret should have been assigned from H5TS_key_create.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That said, I'd be much more in favor of simply dropping the thread-safe variant unless there's a good use case, and requiring qsort_r to exist for thread-safe builds, rather than bending over backwards a bit to add in error handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, ret should have been assigned from the key creation. I've removed this, since like the comment says if key creation fails, that'll be detected immediately afterwards in HDqsort_fallback().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, I'd be much more in favor of simply dropping the thread-safe variant unless there's a good use case, and requiring qsort_r to exist for thread-safe builds, rather than bending over backwards a bit to add in error handling.

What exactly do you mean by requiring qsort_r to exist for threadsafe builds? I think that having the entire threadsafe build be impossible on systems that don't provide qsort_r directly seems too severe. As it stands, a threadsafe build should acquire the API lock on entry to the dataset routines that trigger qsort_r at tree create time anyway, so this should still be safe.

If the API lock isn't enough to handle the cases where the user wants a threadsafe build AND their system doesn't provide qsort_r, then I'd be more inclined to either fallback to not using the r-tree, or to acquire a special lock at tree creation time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's reasonable to require more modern features as the library progresses forward, but for the time being we could simply document the issues for now. For concurrency builds (as opposed to thread-safe builds), this may eventually become an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, the current commit should be ready, unless there's somewhere else that this should be documented.

(void)0; /* Ensure non-empty body even when asserts are disabled */
}
}

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;
herr_t ret;

/* Ensure the TLS key is initialized */
ret = H5TS_once(&HDqsort_fallback_key_once, HDqsort_fallback_key_init);
if (H5_UNLIKELY(ret < 0)) {
assert(false && "Failed to initialize TLS key for qsort fallback");
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this is in main library code, we need to do better here than asserting on a condition that has the potential to occur in debug builds and simply ignoring errors in release builds. Since the signature of this function currently doesn't allow propagating of errors, either the interface needs reworked to support that, or we should just abandon this variant of the function, using the global variable approach only, and document the thread-safety issues until we actually need a thread-safe version.

(void)0; /* Ensure non-empty body even when asserts are disabled */
}

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

/* Store context in thread-local storage */
ret = H5TS_key_set_value(HDqsort_fallback_key, &ctx);
if (H5_UNLIKELY(ret < 0)) {
assert(false && "Failed to set TLS value for qsort fallback");
(void)0; /* Ensure non-empty body even when asserts are disabled */
}

qsort(base, nel, size, HDqsort_fallback_wrapper);

/* Clear the thread-local storage */
ret = H5TS_key_set_value(HDqsort_fallback_key, NULL);
if (H5_UNLIKELY(ret < 0)) {
assert(false && "Failed to clear TLS value for qsort fallback");
(void)0; /* Ensure non-empty body even when asserts are disabled */
}
}

#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