Skip to content

Commit

Permalink
Sync develop changes April 3 - April4 to hdf5_1_14 (#4324)
Browse files Browse the repository at this point in the history 
* Remove VS ptable error from Known Problems (#4317)

* Simply check for datatypes with unusual number of unused bits (#4309)

Avoids potential undefined behavior in H5T_is_numeric_with_unusual_unused_bits

* Fix issues with empty or uninitialized link names (#4322)

Converts an assertion in H5G_loc_find into a normal error
check that checks for empty link names

Initializes H5O_link_t structure early in H5G__ent_to_link
to avoid trying to free potentially uninitialized memory

Checks for an empty link name after H5MM_strndup in
H5G__ent_to_link

Fixes GitHub #4307

* Fix h5py testing failure due to invalid datatype IDs (#4321)

Fixes an issue where invalid datatype IDs are passed to application conversion
functions in the case where the top-level conversion function is a library-internal
function that operates on a container-like datatype, but one or more of the
base datatype members are converted with an application conversion function.

* Revise _Float16 configure checks (#4323)

Run configure checks with and without CFLAGS/CMAKE_C_FLAGS since some
compilers work in one case while not working in the other case

Sync CMake configure checks with Autotools
  • Loading branch information
@lrknox
lrknox authored Apr 4, 2024
1 parent ab26ad6 commit 3d3f15a
Show file tree
Hide file tree
Showing 9 changed files with 497 additions and 118 deletions.
34 changes: 24 additions & 10 deletions config/cmake/ConfigureChecks.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -923,46 +923,56 @@ if (HDF5_ENABLE_NONSTANDARD_FEATURE_FLOAT16)
# compile a program that will generate these functions to check for _Float16
# support. If we fail to compile this program, we will simply disable
# _Float16 support for the time being.
H5ConversionTests (
${HDF_PREFIX}_FLOAT16_CONVERSION_FUNCS_LINK
FALSE
"Checking if compiler can convert _Float16 type with casts"
)

# Some compilers, notably AppleClang on MacOS 12, will succeed in the
# configure check below when optimization flags like -O3 are manually
# configure check above when optimization flags like -O3 are manually
# passed in CMAKE_C_FLAGS. However, the build will then fail when it
# reaches compilation of H5Tconv.c because of the issue mentioned above.
# MacOS 13 appears to have fixed this, but, just to be sure, backup and
# clear CMAKE_C_FLAGS before performing these configure checks.
# MacOS 13 appears to have fixed this, but, just to be sure, make sure
# the check also passes without the passed in CMAKE_C_FLAGS.
set (cmake_c_flags_backup "${CMAKE_C_FLAGS}")
set (CMAKE_C_FLAGS "")

H5ConversionTests (
${HDF_PREFIX}_FLOAT16_CONVERSION_FUNCS_LINK
${HDF_PREFIX}_FLOAT16_CONVERSION_FUNCS_LINK_NO_FLAGS
FALSE
"Checking if compiler can convert _Float16 type with casts"
"Checking if compiler can convert _Float16 type with casts (without CMAKE_C_FLAGS)"
)

set (CMAKE_C_FLAGS "${cmake_c_flags_backup}")

if (${${HDF_PREFIX}_FLOAT16_CONVERSION_FUNCS_LINK})
if (${HDF_PREFIX}_FLOAT16_CONVERSION_FUNCS_LINK AND ${HDF_PREFIX}_FLOAT16_CONVERSION_FUNCS_LINK_NO_FLAGS)
# Finally, MacOS 13 appears to have a bug specifically when converting
# long double values to _Float16. Release builds of the dt_arith test
# would cause any assignments to a _Float16 variable to be elided,
# whereas Debug builds would perform incorrect hardware conversions by
# simply chopping off all the bytes of the value except for the first 2.
# These tests pass on MacOS 14, so let's perform a quick test to check
# if the hardware conversion is done correctly.
H5ConversionTests (
${HDF_PREFIX}_LDOUBLE_TO_FLOAT16_CORRECT
TRUE
"Checking if correctly converting long double to _Float16 values"
)

# Backup and clear CMAKE_C_FLAGS before performing configure checks
# Backup and clear CMAKE_C_FLAGS before performing configure check again
set (cmake_c_flags_backup "${CMAKE_C_FLAGS}")
set (CMAKE_C_FLAGS "")

H5ConversionTests (
${HDF_PREFIX}_LDOUBLE_TO_FLOAT16_CORRECT
${HDF_PREFIX}_LDOUBLE_TO_FLOAT16_CORRECT_NO_FLAGS
TRUE
"Checking if correctly converting long double to _Float16 values"
"Checking if correctly converting long double to _Float16 values (without CMAKE_C_FLAGS)"
)

set (CMAKE_C_FLAGS "${cmake_c_flags_backup}")

if (NOT ${${HDF_PREFIX}_LDOUBLE_TO_FLOAT16_CORRECT})
if (NOT ${HDF_PREFIX}_LDOUBLE_TO_FLOAT16_CORRECT OR NOT ${HDF_PREFIX}_LDOUBLE_TO_FLOAT16_CORRECT_NO_FLAGS)
message (VERBOSE "Conversions from long double to _Float16 appear to be incorrect. These will be emulated through a soft conversion function.")
endif ()

Expand All @@ -985,3 +995,7 @@ else ()
unset (${HDF_PREFIX}_HAVE__FLOAT16 CACHE)
unset (${HDF_PREFIX}_LDOUBLE_TO_FLOAT16_CORRECT CACHE)
endif ()

if (NOT ${HDF_PREFIX}_HAVE__FLOAT16)
set (HDF5_ENABLE_NONSTANDARD_FEATURE_FLOAT16 OFF CACHE BOOL "Enable support for _Float16 C datatype" FORCE)
endif ()
4 changes: 2 additions & 2 deletions config/cmake/ConversionTests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ int HDF_NO_UBSAN main(void)

#endif

#ifdef H5_FLOAT16_CONVERSION_FUNCS_LINK_TEST
#if defined(H5_FLOAT16_CONVERSION_FUNCS_LINK_TEST) || defined(H5_FLOAT16_CONVERSION_FUNCS_LINK_NO_FLAGS_TEST)

#define __STDC_WANT_IEC_60559_TYPES_EXT__

Expand Down Expand Up @@ -379,7 +379,7 @@ int HDF_NO_UBSAN main(void)

#endif

#ifdef H5_LDOUBLE_TO_FLOAT16_CORRECT_TEST
#if defined(H5_LDOUBLE_TO_FLOAT16_CORRECT_TEST) || defined(H5_LDOUBLE_TO_FLOAT16_CORRECT_NO_FLAGS_TEST)

#define __STDC_WANT_IEC_60559_TYPES_EXT__

Expand Down
61 changes: 52 additions & 9 deletions configure.ac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -674,11 +674,34 @@ if test "X$ENABLE_FLOAT16" = "Xyes"; then
AC_CACHE_VAL([hdf5_cv_float16_conversion_funcs_link],
[AC_RUN_IFELSE(
[AC_LANG_SOURCE([$TEST_SRC])],
[hdf5_cv_float16_conversion_funcs_link=yes], [hdf5_cv_float16_conversion_funcs_link=no], [hdf5_cv_float16_conversion_funcs_link=no])])
[hdf5_cv_float16_conversion_funcs_link=yes],
[hdf5_cv_float16_conversion_funcs_link=no],
[hdf5_cv_float16_conversion_funcs_link=no])])
AC_MSG_RESULT(${hdf5_cv_float16_conversion_funcs_link})

# Some compilers, notably AppleClang on MacOS 12, will succeed in the
# configure check above when optimization flags like -O3 are manually
# passed in CFLAGS. However, the build will then fail when it reaches
# compilation of H5Tconv.c because of the issue mentioned above. MacOS
# 13 appears to have fixed this, but, just to be sure, make sure the
# check also passes without the passed in CFLAGS.
conftest_cflags_backup="$CFLAGS"
CFLAGS=""

AC_MSG_CHECKING([if compiler can correctly compile and run a test program which converts _Float16 to other types with casts (without CFLAGS)])
TEST_SRC="`(echo \"#define H5_FLOAT16_CONVERSION_FUNCS_LINK_NO_FLAGS_TEST 1\"; cat $srcdir/config/cmake/ConversionTests.c)`"
AC_CACHE_VAL([hdf5_cv_float16_conversion_funcs_link_no_flags],
[AC_RUN_IFELSE(
[AC_LANG_SOURCE([$TEST_SRC])],
[hdf5_cv_float16_conversion_funcs_link_no_flags=yes],
[hdf5_cv_float16_conversion_funcs_link_no_flags=no],
[hdf5_cv_float16_conversion_funcs_link_no_flags=no])])
AC_MSG_RESULT(${hdf5_cv_float16_conversion_funcs_link_no_flags})

if test ${hdf5_cv_float16_conversion_funcs_link} = "yes"; then
AC_MSG_RESULT([yes])
CFLAGS="$conftest_cflags_backup"

if test ${hdf5_cv_float16_conversion_funcs_link} = "yes" &&
test ${hdf5_cv_float16_conversion_funcs_link_no_flags} = "yes"; then
# Finally, MacOS 13 appears to have a bug specifically when converting
# long double values to _Float16. Release builds of the dt_arith test
# would cause any assignments to a _Float16 variable to be elided,
Expand All @@ -694,15 +717,37 @@ if test "X$ENABLE_FLOAT16" = "Xyes"; then
AC_CACHE_VAL([hdf5_cv_ldouble_to_float16_correct],
[AC_RUN_IFELSE(
[AC_LANG_SOURCE([$TEST_SRC])],
[hdf5_cv_ldouble_to_float16_correct=yes], [hdf5_cv_ldouble_to_float16_correct=no], [hdf5_cv_ldouble_to_float16_correct=yes])])
[hdf5_cv_ldouble_to_float16_correct=yes],
[hdf5_cv_ldouble_to_float16_correct=no],
[hdf5_cv_ldouble_to_float16_correct=yes])])
fi
AC_MSG_RESULT(${hdf5_cv_ldouble_to_float16_correct})

# Backup and clear CFLAGS before performing configure check again
conftest_cflags_backup="$CFLAGS"
CFLAGS=""

if test ${hdf5_cv_ldouble_to_float16_correct} = "yes"; then
AC_MSG_CHECKING([if compiler can correctly convert long double values to _Float16 (without CFLAGS)])
TEST_SRC="`(echo \"#define H5_LDOUBLE_TO_FLOAT16_CORRECT_NO_FLAGS_TEST 1\"; cat $srcdir/config/cmake/ConversionTests.c)`"
if test ${ac_cv_sizeof_long_double} = 0; then
hdf5_cv_ldouble_to_float16_correct_no_flags=${hdf5_cv_ldouble_to_float16_correct_no_flags=no}
else
AC_CACHE_VAL([hdf5_cv_ldouble_to_float16_correct_no_flags],
[AC_RUN_IFELSE(
[AC_LANG_SOURCE([$TEST_SRC])],
[hdf5_cv_ldouble_to_float16_correct_no_flags=yes],
[hdf5_cv_ldouble_to_float16_correct_no_flags=no],
[hdf5_cv_ldouble_to_float16_correct_no_flags=yes])])
fi
AC_MSG_RESULT(${hdf5_cv_ldouble_to_float16_correct_no_flags})

CFLAGS="$conftest_cflags_backup"

if test ${hdf5_cv_ldouble_to_float16_correct} = "yes" &&
test ${hdf5_cv_ldouble_to_float16_correct_no_flags} = "yes"; then
AC_DEFINE([LDOUBLE_TO_FLOAT16_CORRECT], [1],
[Define if your system can convert long double to _Float16 values correctly.])
AC_MSG_RESULT([yes])
else
AC_MSG_RESULT([no])
AC_MSG_NOTICE([Conversions from long double to _Float16 appear to be incorrect. These will be emulated through a soft conversion function.])
fi

Expand All @@ -714,8 +759,6 @@ if test "X$ENABLE_FLOAT16" = "Xyes"; then

# Define HAVE__FLOAT16 macro for H5pubconf.h if _Float16 is available.
AC_DEFINE([HAVE__FLOAT16], [1], [Determine if _Float16 is available])
else
AC_MSG_RESULT([no])
fi
fi

Expand Down
3 changes: 0 additions & 3 deletions release_docs/RELEASE.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1106,9 +1106,6 @@ Known Problems
images, but since this is a collective operation, a deadlock is possible
if one or more processes do not participate.

CPP ptable test fails on both VS2017 and VS2019 with Intel compiler, JIRA
issue: HDFFV-10628. This test will pass with VS2015 with Intel compiler.

The subsetting option in ph5diff currently will fail and should be avoided.
The subsetting option works correctly in serial h5diff.

Expand Down
13 changes: 9 additions & 4 deletions src/H5Gent.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,19 +365,24 @@ H5G__ent_to_link(const H5G_entry_t *ent, const H5HL_t *heap, H5O_link_t *lnk)
assert(heap);
assert(lnk);

/* Initialize structure and set (default) common info for link */
lnk->type = H5L_TYPE_ERROR;
lnk->corder_valid = false; /* Creation order not valid for this link */
lnk->corder = 0;
lnk->cset = H5F_DEFAULT_CSET;
lnk->name = NULL;

/* Get the size of the heap block */
block_size = H5HL_heap_get_size(heap);

/* Get pointer to link's name in the heap */
if (NULL == (name = (const char *)H5HL_offset_into(heap, ent->name_off)))
HGOTO_ERROR(H5E_SYM, H5E_CANTGET, FAIL, "unable to get symbol table link name");

/* Set (default) common info for link */
lnk->cset = H5F_DEFAULT_CSET;
lnk->corder = 0;
lnk->corder_valid = false; /* Creation order not valid for this link */
if (NULL == (lnk->name = H5MM_strndup(name, (block_size - ent->name_off))))
HGOTO_ERROR(H5E_SYM, H5E_CANTGET, FAIL, "unable to duplicate link name");
if (!*lnk->name)
HGOTO_ERROR(H5E_SYM, H5E_BADVALUE, FAIL, "invalid link name");

/* Object is a symbolic or hard link */
if (ent->type == H5G_CACHED_SLINK) {
Expand Down
5 changes: 4 additions & 1 deletion src/H5Gloc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,9 +410,12 @@ H5G_loc_find(const H5G_loc_t *loc, const char *name, H5G_loc_t *obj_loc /*out*/)

/* Check args. */
assert(loc);
assert(name && *name);
assert(name);
assert(obj_loc);

if (!*name)
HGOTO_ERROR(H5E_SYM, H5E_BADVALUE, FAIL, "invalid object name");

/* Set up user data for locating object */
udata.loc = obj_loc;

Expand Down
44 changes: 22 additions & 22 deletions src/H5T.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3393,6 +3393,7 @@ H5T__create(H5T_class_t type, size_t size)
/* Copy the default string datatype */
if (NULL == (dt = H5T_copy(origin_dt, H5T_COPY_TRANSIENT)))
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, NULL, "unable to copy");
dt->shared->type = type;

/* Modify the datatype */
if (H5T__set_size(dt, size) < 0)
Expand Down Expand Up @@ -4577,6 +4578,8 @@ H5T_cmp(const H5T_t *dt1, const H5T_t *dt2, bool superset)
/*
* Compound data types...
*/
if (dt1->shared->u.compnd.nmembs == 0 && dt2->shared->u.compnd.nmembs == 0)
HGOTO_DONE(0);
if (dt1->shared->u.compnd.nmembs < dt2->shared->u.compnd.nmembs)
HGOTO_DONE(-1);
if (dt1->shared->u.compnd.nmembs > dt2->shared->u.compnd.nmembs)
Expand Down Expand Up @@ -4619,11 +4622,13 @@ H5T_cmp(const H5T_t *dt1, const H5T_t *dt2, bool superset)

#ifdef H5T_DEBUG
/* I don't quite trust the code above yet :-) --RPM */
for (u = 0; u < dt1->shared->u.compnd.nmembs - 1; u++) {
assert(strcmp(dt1->shared->u.compnd.memb[idx1[u]].name,
dt1->shared->u.compnd.memb[idx1[u + 1]].name));
assert(strcmp(dt2->shared->u.compnd.memb[idx2[u]].name,
dt2->shared->u.compnd.memb[idx2[u + 1]].name));
if (dt1->shared->u.compnd.nmembs > 0) {
for (u = 0; u < dt1->shared->u.compnd.nmembs - 1; u++) {
assert(strcmp(dt1->shared->u.compnd.memb[idx1[u]].name,
dt1->shared->u.compnd.memb[idx1[u + 1]].name));
assert(strcmp(dt2->shared->u.compnd.memb[idx2[u]].name,
dt2->shared->u.compnd.memb[idx2[u + 1]].name));
}
}
#endif

Expand Down Expand Up @@ -4659,6 +4664,8 @@ H5T_cmp(const H5T_t *dt1, const H5T_t *dt2, bool superset)
/*
* Enumeration data types...
*/
if (dt1->shared->u.enumer.nmembs == 0 && dt2->shared->u.enumer.nmembs == 0)
HGOTO_DONE(0);

/* If we are doing a "superset" comparison, dt2 is allowed to have
* more members than dt1
Expand Down Expand Up @@ -4716,9 +4723,13 @@ H5T_cmp(const H5T_t *dt1, const H5T_t *dt2, bool superset)

#ifdef H5T_DEBUG
/* I don't quite trust the code above yet :-) --RPM */
for (u = 0; u < dt1->shared->u.enumer.nmembs - 1; u++) {
assert(strcmp(dt1->shared->u.enumer.name[idx1[u]], dt1->shared->u.enumer.name[idx1[u + 1]]));
assert(strcmp(dt2->shared->u.enumer.name[idx2[u]], dt2->shared->u.enumer.name[idx2[u + 1]]));
if (dt1->shared->u.enumer.nmembs > 0) {
for (u = 0; u < dt1->shared->u.enumer.nmembs - 1; u++) {
assert(
strcmp(dt1->shared->u.enumer.name[idx1[u]], dt1->shared->u.enumer.name[idx1[u + 1]]));
assert(
strcmp(dt2->shared->u.enumer.name[idx2[u]], dt2->shared->u.enumer.name[idx2[u + 1]]));
}
}
#endif

Expand Down Expand Up @@ -6784,24 +6795,13 @@ H5T_is_numeric_with_unusual_unused_bits(const H5T_t *dt)
/* Is the correct type? */
if (H5T_INTEGER == dt->shared->type || H5T_FLOAT == dt->shared->type ||
H5T_BITFIELD == dt->shared->type) {
#if LDBL_MANT_DIG == 106
/* This currently won't work for the IBM long double type */
if (H5T_FLOAT == dt->shared->type && dt->shared->size == 16 &&
(dt->shared->u.atomic.prec == 64 || dt->shared->u.atomic.prec == 128))
HGOTO_DONE(false);
#endif

/* Has unused bits? */
if (dt->shared->u.atomic.prec < (dt->shared->size * 8)) {
unsigned surround_bits =
1U << (1 + H5VM_log2_gen((dt->shared->u.atomic.prec + dt->shared->u.atomic.offset) - 1));

if (dt->shared->size > 1 && dt->shared->u.atomic.prec < (dt->shared->size * 8))
/* Unused bits are unusually large? */
if (dt->shared->size > 1 && ((dt->shared->size * 8) > surround_bits))
HGOTO_DONE(true);
}
ret_value =
(dt->shared->size * 8) > (2 * (dt->shared->u.atomic.prec + dt->shared->u.atomic.offset));
}

done:
FUNC_LEAVE_NOAPI(ret_value)
} /* end H5T_is_numeric_with_unusual_unused_bits() */
Loading

0 comments on commit 3d3f15a

Please sign in to comment.