Skip to content
Open
Show file tree
Hide file tree
Changes from 12 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: ['13.5', '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.

Beginning with 14.3 will be sufficient, as 13.5 has reached EOL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See my comment on #5800 (comment) though. FreeBSD switched flipped argument ordering between 13 and 14, so it may not be a bad idea to test both.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lrknox and @hyoklee can clarify, but we have a testing policy of not supporting OS that have reached EOL. But it is up to them if they want to forgo this policy in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's test 13.5.


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 ""
99 changes: 99 additions & 0 deletions .github/workflows/netbsd.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
name: NetBSD 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:
netbsd-build-and-test:
runs-on: ubuntu-latest
name: NetBSD ${{ matrix.netbsd-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:
netbsd-version: ['10.1']

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

- name: Build and test on NetBSD
uses: vmactions/netbsd-vm@v1
with:
release: ${{ matrix.netbsd-version }}
usesh: true
prepare: |
/usr/sbin/pkg_add -v pkgin
pkgin -y update
pkgin -y install cmake gmake pkgconf curl gcc13
echo "Package installation completed with exit code: $?"
run: |
set -e

# Set up library path for gcc13
export LD_LIBRARY_PATH=/usr/pkg/gcc13/lib:$LD_LIBRARY_PATH

# Get number of processors (NetBSD uses sysctl in /sbin)
NPROC=$(/sbin/sysctl -n hw.ncpu)
echo "Number of processors: $NPROC"

# Configure the build
mkdir build
cd build
cmake -C ../config/cmake/cacheinit.cmake \
--log-level=VERBOSE \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_C_COMPILER=/usr/pkg/gcc13/bin/gcc \
-DCMAKE_CXX_COMPILER=/usr/pkg/gcc13/bin/g++ \
-DCMAKE_MAKE_PROGRAM=/usr/pkg/bin/gmake \
-DBUILD_SHARED_LIBS:BOOL=ON \
-DHDF5_ENABLE_ALL_WARNINGS: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_BUILD_HL_LIB: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 $NPROC
echo ""

# TODO: Later threadsafe tests (rwlock 2+) timeout - should be investigated
# Skipping ttsafe for now to test qsort_r fallback

# Run tests (excluding ttsafe which times out)
ctest . --parallel $NPROC -V -E ttsafe
echo ""
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
4 changes: 3 additions & 1 deletion src/H5RT.c
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,9 @@ H5RT__bulk_load(H5RT_node_t *node, int rank, H5RT_leaf_t *leaves, size_t count,
if (prev_sort_dim != rank - 1) {
assert(prev_sort_dim < rank - 1);
sort_dim = prev_sort_dim + 1;
HDqsort_r((void *)leaves, count, sizeof(H5RT_leaf_t), H5RT__leaf_compare, (void *)&sort_dim);
if (H5_UNLIKELY(HDqsort_r((void *)leaves, count, sizeof(H5RT_leaf_t), H5RT__leaf_compare,
(void *)&sort_dim) < 0))
HGOTO_ERROR(H5E_INTERNAL, H5E_CANTSORT, FAIL, "failed to sort R-tree leaves");
}
else {
sort_dim = prev_sort_dim;
Expand Down
20 changes: 16 additions & 4 deletions src/H5private.h
Original file line number Diff line number Diff line change
Expand Up @@ -650,8 +650,13 @@ H5_DLL H5_ATTR_CONST int Nflock(int fd, int operation);
#endif /* HDflock */

#if defined(H5_HAVE_WIN32_API) || defined(H5_HAVE_DARWIN) || (defined(__FreeBSD__) && __FreeBSD__ < 14)
H5_DLL void HDqsort_context(void *base, size_t nel, size_t size,
int (*compar)(const void *, const void *, void *), void *arg);
H5_DLL herr_t 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 herr_t HDqsort_fallback(void *base, size_t nel, size_t size,
int (*compar)(const void *, const void *, void *), void *arg);
#endif

#ifndef HDfseek
Expand Down Expand Up @@ -770,10 +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)
/* Wrap native GNU qsort_r to vacuously return success */
#define HDqsort_r(B, N, S, C, A) (qsort_r(B, N, S, C, A), SUCCEED)
#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
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
126 changes: 125 additions & 1 deletion src/H5system.c
Original file line number Diff line number Diff line change
Expand Up @@ -1441,7 +1441,7 @@ HDqsort_context_wrapper_func(void *wrapper_arg, const void *a, const void *b)
return w->gnu_compar(a, b, w->gnu_arg);
}

void
herr_t
HDqsort_context(void *base, size_t nel, size_t size, int (*compar)(const void *, const void *, void *),
void *arg)
{
Expand All @@ -1454,5 +1454,129 @@ HDqsort_context(void *base, size_t nel, size_t size, int (*compar)(const void *,
/* Old BSD-style: context parameter comes before comparator function */
qsort_r(base, nel, size, &wrapper, HDqsort_context_wrapper_func);
#endif
return SUCCEED;
}
#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
*
* Error Handling Note:
* HDqsort_fallback_key_init() still uses assertions because it's a callback with void signature.
* However, HDqsort_fallback() now returns herr_t and can propagate TLS operation failures
* through the normal HDF5 error stack.
*/
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) */
/* If this operation fails, it will be detected shortly during
* HDqsort_fallback when operations are attempted on the non-existent key */
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);
}

herr_t
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_value = SUCCEED;

FUNC_ENTER_NOAPI_NOERR

/* Ensure the TLS key is initialized */
if (H5_UNLIKELY(H5TS_once(&HDqsort_fallback_key_once, HDqsort_fallback_key_init) < 0))
HGOTO_ERROR(H5E_INTERNAL, H5E_CANTINIT, FAIL, "failed to initialize TLS key for qsort");

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

/* Store context in thread-local storage */
if (H5_UNLIKELY(H5TS_key_set_value(HDqsort_fallback_key, &ctx) < 0))
HGOTO_ERROR(H5E_INTERNAL, H5E_CANTSET, FAIL, "failed to set TLS context for qsort");

qsort(base, nel, size, HDqsort_fallback_wrapper);

/* Clear the thread-local storage */
if (H5_UNLIKELY(H5TS_key_set_value(HDqsort_fallback_key, NULL) < 0))
HGOTO_ERROR(H5E_INTERNAL, H5E_CANTSET, FAIL, "failed to clear TLS context for qsort");

done:
FUNC_LEAVE_NOAPI(ret_value)
}

#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);
}

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

return SUCCEED;
}
#endif /* H5_HAVE_THREADSAFE */

#endif /* !H5_HAVE_QSORT_REENTRANT */
Loading