From 801b62c41886c985833183766b8f706f4f5b55d8 Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Fri, 22 Mar 2024 17:43:13 -0500 Subject: [PATCH 01/10] Fix issues with background buffers with array datatypes --- src/H5Tconv.c | 74 ++++++++++++++++++++++++++++----------------------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/src/H5Tconv.c b/src/H5Tconv.c index 422e3110497..2278616656b 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -1183,6 +1183,11 @@ typedef struct H5T_conv_enum_t { int *src2dst; /*map from src to dst index */ } H5T_conv_enum_t; +/* Cnversion data fot H5T__conv_array() */ +typedef struct H5T_conv_array_t { + H5T_path_t *tpath; /* Conversion path for parent types */ +} H5T_conv_array_t; + /* Conversion data for the hardware conversion functions */ typedef struct H5T_conv_hw_t { size_t s_aligned; /*number source elements aligned */ @@ -1218,9 +1223,6 @@ static herr_t H5T__reverse_order(uint8_t *rev, uint8_t *s, size_t size, H5T_orde /* Declare a free list to manage pieces of vlen data */ H5FL_BLK_DEFINE_STATIC(vlen_seq); -/* Declare a free list to manage pieces of array data */ -H5FL_BLK_DEFINE_STATIC(array_seq); - /* Declare a free list to manage pieces of reference data */ H5FL_BLK_DEFINE_STATIC(ref_seq); @@ -3844,19 +3846,18 @@ H5T__conv_vlen(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, const H5T herr_t H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, const H5T_conv_ctx_t H5_ATTR_UNUSED *conv_ctx, size_t nelmts, size_t buf_stride, - size_t bkg_stride, void *_buf, void H5_ATTR_UNUSED *_bkg) + size_t bkg_stride, void *_buf, void *_bkg) { + H5T_conv_array_t *priv = NULL; /* Private conversion data */ H5T_conv_ctx_t tmp_conv_ctx = {0}; /* Temporary conversion context */ - H5T_path_t *tpath; /* Type conversion path */ H5T_t *tsrc_cpy = NULL; /*temporary copy of source base datatype */ H5T_t *tdst_cpy = NULL; /*temporary copy of destination base datatype */ hid_t tsrc_id = H5I_INVALID_HID; /*temporary type atom */ hid_t tdst_id = H5I_INVALID_HID; /*temporary type atom */ - uint8_t *sp, *dp; /*source and dest traversal ptrs */ + uint8_t *sp, *dp, *bp; /*source, dest, and bkg traversal ptrs */ ssize_t src_delta, dst_delta; /*source & destination stride */ int direction; /*direction of traversal */ bool need_ids = false; /*whether we need IDs for the datatypes */ - void *bkg_buf = NULL; /*temporary background buffer */ herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_PACKAGE @@ -3884,13 +3885,32 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, HGOTO_ERROR(H5E_DATATYPE, H5E_UNSUPPORTED, FAIL, "array datatypes do not have the same sizes of dimensions"); - /* Array datatypes don't need a background buffer */ - cdata->need_bkg = H5T_BKG_NO; + /* Initialize parent type conversion if necessary. We need to do this here because we need to report whether we need a background buffer or not. */ + if (!cdata->priv) { + /* Allocate private data */ + if (NULL == (priv = (H5T_conv_array_t *)(cdata->priv = calloc(1, sizeof(*priv))))) + HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed"); + + /* Find conversion path between parent types */ + if (NULL == (priv->tpath = H5T_path_find(src->shared->parent, dst->shared->parent))) { + free(priv); + cdata->priv = NULL; + HGOTO_ERROR(H5E_DATATYPE, H5E_UNSUPPORTED, FAIL, "unable to convert between src and dest datatype"); + } + + /* Array datatypes don't need a background buffer by themselves, but the parent type might. Pass the need_bkg field through to the upper layer. */ + cdata->need_bkg = priv->tpath->cdata.need_bkg; + } break; case H5T_CONV_FREE: - /* QAK - Nothing to do currently */ + /* + * Free private data + */ + free(cdata->priv); + cdata->priv = NULL; + break; case H5T_CONV_CONV: @@ -3901,6 +3921,7 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a datatype"); if (NULL == conv_ctx) HGOTO_ERROR(H5E_DATATYPE, H5E_BADVALUE, FAIL, "invalid datatype conversion context pointer"); + priv = (H5T_conv_array_t *)cdata->priv; /* Initialize temporary conversion context */ tmp_conv_ctx = *conv_ctx; @@ -3913,12 +3934,14 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, * destination areas overlapping? */ if (src->shared->size >= dst->shared->size || buf_stride > 0) { - sp = dp = (uint8_t *)_buf; + sp = dp = (uint8_t *)_buf; + bp = _bkg; direction = 1; } else { sp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : src->shared->size); dp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : dst->shared->size); + bp = (uint8_t *)_bkg + (nelmts - 1) * (bkg_stride ? bkg_stride : dst->shared->size); direction = -1; } @@ -3932,11 +3955,7 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, dst_delta = (ssize_t)direction * (ssize_t)(buf_stride ? buf_stride : dst->shared->size); /* Set up conversion path for base elements */ - if (NULL == (tpath = H5T_path_find(src->shared->parent, dst->shared->parent))) { - HGOTO_ERROR(H5E_DATATYPE, H5E_UNSUPPORTED, FAIL, - "unable to convert between src and dest datatypes"); - } - else if (!H5T_path_noop(tpath)) { + if (!H5T_path_noop(priv->tpath)) { if (NULL == (tsrc_cpy = H5T_copy(src->shared->parent, H5T_COPY_ALL))) HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCOPY, FAIL, "unable to copy src base type for conversion"); @@ -3959,17 +3978,6 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, tmp_conv_ctx.u.conv.dst_type_id = tdst_id; } - /* Check if we need a background buffer for this conversion */ - if (tpath->cdata.need_bkg) { - size_t bkg_buf_size; /*size of background buffer in bytes */ - - /* Allocate background buffer */ - bkg_buf_size = src->shared->u.array.nelem * MAX(src->shared->size, dst->shared->size); - if (NULL == (bkg_buf = H5FL_BLK_CALLOC(array_seq, bkg_buf_size))) - HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, - "memory allocation failed for type conversion"); - } /* end if */ - /* Perform the actual conversion */ tmp_conv_ctx.u.conv.recursive = true; for (size_t elmtno = 0; elmtno < nelmts; elmtno++) { @@ -3977,13 +3985,15 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, memmove(dp, sp, src->shared->size); /* Convert array */ - if (H5T_convert_with_ctx(tpath, tsrc_cpy, tdst_cpy, &tmp_conv_ctx, src->shared->u.array.nelem, - (size_t)0, bkg_stride, dp, bkg_buf) < 0) + if (H5T_convert_with_ctx(priv->tpath, tsrc_cpy, tdst_cpy, &tmp_conv_ctx, src->shared->u.array.nelem, + (size_t)0, (size_t)0, dp, bp) < 0) HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCONVERT, FAIL, "datatype conversion failed"); - /* Advance the source & destination pointers */ + /* Advance the source, destination, and background pointers */ sp += src_delta; dp += dst_delta; + if (bp) + bp += dst_delta; } /* end for */ tmp_conv_ctx.u.conv.recursive = false; @@ -4011,10 +4021,6 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, HDONE_ERROR(H5E_DATATYPE, H5E_CANTCLOSEOBJ, FAIL, "can't close temporary datatype"); } - /* Release the background buffer, if we have one */ - if (bkg_buf) - bkg_buf = H5FL_BLK_FREE(array_seq, bkg_buf); - FUNC_LEAVE_NOAPI(ret_value) } /* end H5T__conv_array() */ From a6a775013accea1d48921cc4eac100fabee83a20 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 22 Mar 2024 22:45:21 +0000 Subject: [PATCH 02/10] Committing clang-format changes --- src/H5Tconv.c | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/H5Tconv.c b/src/H5Tconv.c index 2278616656b..d9a696b64ff 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -3848,17 +3848,17 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, const H5T_conv_ctx_t H5_ATTR_UNUSED *conv_ctx, size_t nelmts, size_t buf_stride, size_t bkg_stride, void *_buf, void *_bkg) { - H5T_conv_array_t *priv = NULL; /* Private conversion data */ - H5T_conv_ctx_t tmp_conv_ctx = {0}; /* Temporary conversion context */ - H5T_t *tsrc_cpy = NULL; /*temporary copy of source base datatype */ - H5T_t *tdst_cpy = NULL; /*temporary copy of destination base datatype */ - hid_t tsrc_id = H5I_INVALID_HID; /*temporary type atom */ - hid_t tdst_id = H5I_INVALID_HID; /*temporary type atom */ - uint8_t *sp, *dp, *bp; /*source, dest, and bkg traversal ptrs */ - ssize_t src_delta, dst_delta; /*source & destination stride */ - int direction; /*direction of traversal */ - bool need_ids = false; /*whether we need IDs for the datatypes */ - herr_t ret_value = SUCCEED; /* Return value */ + H5T_conv_array_t *priv = NULL; /* Private conversion data */ + H5T_conv_ctx_t tmp_conv_ctx = {0}; /* Temporary conversion context */ + H5T_t *tsrc_cpy = NULL; /*temporary copy of source base datatype */ + H5T_t *tdst_cpy = NULL; /*temporary copy of destination base datatype */ + hid_t tsrc_id = H5I_INVALID_HID; /*temporary type atom */ + hid_t tdst_id = H5I_INVALID_HID; /*temporary type atom */ + uint8_t *sp, *dp, *bp; /*source, dest, and bkg traversal ptrs */ + ssize_t src_delta, dst_delta; /*source & destination stride */ + int direction; /*direction of traversal */ + bool need_ids = false; /*whether we need IDs for the datatypes */ + herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_PACKAGE @@ -3885,7 +3885,8 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, HGOTO_ERROR(H5E_DATATYPE, H5E_UNSUPPORTED, FAIL, "array datatypes do not have the same sizes of dimensions"); - /* Initialize parent type conversion if necessary. We need to do this here because we need to report whether we need a background buffer or not. */ + /* Initialize parent type conversion if necessary. We need to do this here because we need to + * report whether we need a background buffer or not. */ if (!cdata->priv) { /* Allocate private data */ if (NULL == (priv = (H5T_conv_array_t *)(cdata->priv = calloc(1, sizeof(*priv))))) @@ -3895,10 +3896,12 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, if (NULL == (priv->tpath = H5T_path_find(src->shared->parent, dst->shared->parent))) { free(priv); cdata->priv = NULL; - HGOTO_ERROR(H5E_DATATYPE, H5E_UNSUPPORTED, FAIL, "unable to convert between src and dest datatype"); + HGOTO_ERROR(H5E_DATATYPE, H5E_UNSUPPORTED, FAIL, + "unable to convert between src and dest datatype"); } - /* Array datatypes don't need a background buffer by themselves, but the parent type might. Pass the need_bkg field through to the upper layer. */ + /* Array datatypes don't need a background buffer by themselves, but the parent type might. + * Pass the need_bkg field through to the upper layer. */ cdata->need_bkg = priv->tpath->cdata.need_bkg; } @@ -3934,8 +3937,8 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, * destination areas overlapping? */ if (src->shared->size >= dst->shared->size || buf_stride > 0) { - sp = dp = (uint8_t *)_buf; - bp = _bkg; + sp = dp = (uint8_t *)_buf; + bp = _bkg; direction = 1; } else { @@ -3985,8 +3988,8 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, memmove(dp, sp, src->shared->size); /* Convert array */ - if (H5T_convert_with_ctx(priv->tpath, tsrc_cpy, tdst_cpy, &tmp_conv_ctx, src->shared->u.array.nelem, - (size_t)0, (size_t)0, dp, bp) < 0) + if (H5T_convert_with_ctx(priv->tpath, tsrc_cpy, tdst_cpy, &tmp_conv_ctx, + src->shared->u.array.nelem, (size_t)0, (size_t)0, dp, bp) < 0) HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCONVERT, FAIL, "datatype conversion failed"); /* Advance the source, destination, and background pointers */ From 8483433421b821e6066da3196d605d0a2d2ffd43 Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Fri, 22 Mar 2024 17:46:19 -0500 Subject: [PATCH 03/10] Spelling --- src/H5Tconv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/H5Tconv.c b/src/H5Tconv.c index d9a696b64ff..4612ab3bd30 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -1183,7 +1183,7 @@ typedef struct H5T_conv_enum_t { int *src2dst; /*map from src to dst index */ } H5T_conv_enum_t; -/* Cnversion data fot H5T__conv_array() */ +/* Cnversion data for H5T__conv_array() */ typedef struct H5T_conv_array_t { H5T_path_t *tpath; /* Conversion path for parent types */ } H5T_conv_array_t; From ae0f2f2436f1c554517b60a6cabc0be751c3777e Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Thu, 28 Mar 2024 20:42:13 -0500 Subject: [PATCH 04/10] Fix bug in array conversion with strided background buffer. Convert some mommove calls to non-overlapping buffers to memcpy. --- src/H5Tconv.c | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/src/H5Tconv.c b/src/H5Tconv.c index 4612ab3bd30..7b8982b1229 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -2520,7 +2520,7 @@ H5T__conv_struct(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, const H } /* end if */ else offset -= dst_memb->size; - memmove(xbkg + dst_memb->offset, xbuf + offset, dst_memb->size); + memcpy(xbkg + dst_memb->offset, xbuf + offset, dst_memb->size); } /* end for */ tmp_conv_ctx.u.conv.recursive = false; @@ -2542,7 +2542,7 @@ H5T__conv_struct(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, const H * buffer. */ for (xbuf = buf, xbkg = bkg, elmtno = 0; elmtno < nelmts; elmtno++) { - memmove(xbuf, xbkg, dst->shared->size); + memcpy(xbuf, xbkg, dst->shared->size); xbuf += buf_stride ? buf_stride : dst->shared->size; xbkg += bkg_delta; } /* end for */ @@ -2742,7 +2742,7 @@ H5T__conv_struct_opt(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, con copy_size = priv->subset_info.copy_size; for (elmtno = 0; elmtno < nelmts; elmtno++) { - memmove(xbkg, xbuf, copy_size); + memcpy(xbkg, xbuf, copy_size); /* Update pointers */ xbuf += buf_stride; @@ -2780,7 +2780,7 @@ H5T__conv_struct_opt(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, con "unable to convert compound datatype member"); for (elmtno = 0; elmtno < nelmts; elmtno++) { - memmove(xbkg, xbuf, dst_memb->size); + memcpy(xbkg, xbuf, dst_memb->size); xbuf += buf_stride; xbkg += bkg_stride; } /* end for */ @@ -2825,7 +2825,7 @@ H5T__conv_struct_opt(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, con HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCONVERT, FAIL, "unable to convert compound datatype member"); for (elmtno = 0; elmtno < nelmts; elmtno++) { - memmove(xbkg, xbuf, dst_memb->size); + memcpy(xbkg, xbuf, dst_memb->size); xbuf += buf_stride; xbkg += bkg_stride; } /* end for */ @@ -2839,7 +2839,7 @@ H5T__conv_struct_opt(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, con /* Move background buffer into result buffer */ for (xbuf = buf, xbkg = bkg, elmtno = 0; elmtno < nelmts; elmtno++) { - memmove(xbuf, xbkg, dst->shared->size); + memcpy(xbuf, xbkg, dst->shared->size); xbuf += buf_stride; xbkg += bkg_stride; } /* end for */ @@ -3093,7 +3093,7 @@ H5T__conv_enum_init(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, cons } FUNC_LEAVE_NOAPI(ret_value) -} +} /* end H5T__conv_enum_init() */ /*------------------------------------------------------------------------- * Function: H5T__conv_enum @@ -3848,17 +3848,17 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, const H5T_conv_ctx_t H5_ATTR_UNUSED *conv_ctx, size_t nelmts, size_t buf_stride, size_t bkg_stride, void *_buf, void *_bkg) { - H5T_conv_array_t *priv = NULL; /* Private conversion data */ - H5T_conv_ctx_t tmp_conv_ctx = {0}; /* Temporary conversion context */ - H5T_t *tsrc_cpy = NULL; /*temporary copy of source base datatype */ - H5T_t *tdst_cpy = NULL; /*temporary copy of destination base datatype */ - hid_t tsrc_id = H5I_INVALID_HID; /*temporary type atom */ - hid_t tdst_id = H5I_INVALID_HID; /*temporary type atom */ - uint8_t *sp, *dp, *bp; /*source, dest, and bkg traversal ptrs */ - ssize_t src_delta, dst_delta; /*source & destination stride */ - int direction; /*direction of traversal */ - bool need_ids = false; /*whether we need IDs for the datatypes */ - herr_t ret_value = SUCCEED; /* Return value */ + H5T_conv_array_t *priv = NULL; /* Private conversion data */ + H5T_conv_ctx_t tmp_conv_ctx = {0}; /* Temporary conversion context */ + H5T_t *tsrc_cpy = NULL; /* Temporary copy of source base datatype */ + H5T_t *tdst_cpy = NULL; /* Temporary copy of destination base datatype */ + hid_t tsrc_id = H5I_INVALID_HID; /* Temporary type atom */ + hid_t tdst_id = H5I_INVALID_HID; /* Temporary type atom */ + uint8_t *sp, *dp, *bp; /* Source, dest, and bkg traversal ptrs */ + ssize_t src_delta, dst_delta, bkg_delta; /* Source, dest, and bkg strides */ + int direction; /* Direction of traversal */ + bool need_ids = false; /* Whether we need IDs for the datatypes */ + herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_PACKAGE @@ -3944,7 +3944,7 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, else { sp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : src->shared->size); dp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : dst->shared->size); - bp = (uint8_t *)_bkg + (nelmts - 1) * (bkg_stride ? bkg_stride : dst->shared->size); + bp = _bkg ? (uint8_t *)_bkg + (nelmts - 1) * (bkg_stride ? bkg_stride : dst->shared->size) : NULL; direction = -1; } @@ -3956,6 +3956,7 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, H5_CHECK_OVERFLOW(dst->shared->size, size_t, ssize_t); src_delta = (ssize_t)direction * (ssize_t)(buf_stride ? buf_stride : src->shared->size); dst_delta = (ssize_t)direction * (ssize_t)(buf_stride ? buf_stride : dst->shared->size); + bkg_delta = (ssize_t)direction * (ssize_t)(bkg_stride ? bkg_stride : dst->shared->size); /* Set up conversion path for base elements */ if (!H5T_path_noop(priv->tpath)) { @@ -3985,7 +3986,7 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, tmp_conv_ctx.u.conv.recursive = true; for (size_t elmtno = 0; elmtno < nelmts; elmtno++) { /* Copy the source array into the correct location for the destination */ - memmove(dp, sp, src->shared->size); + memcpy(dp, sp, src->shared->size); /* Convert array */ if (H5T_convert_with_ctx(priv->tpath, tsrc_cpy, tdst_cpy, &tmp_conv_ctx, @@ -3996,7 +3997,7 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, sp += src_delta; dp += dst_delta; if (bp) - bp += dst_delta; + bp += bkg_delta; } /* end for */ tmp_conv_ctx.u.conv.recursive = false; From 78b3f4bee109533cbc2c34d68c17dce83e28a85a Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 29 Mar 2024 01:44:39 +0000 Subject: [PATCH 05/10] Committing clang-format changes --- src/H5Tconv.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/H5Tconv.c b/src/H5Tconv.c index 7b8982b1229..2a03130e523 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -3942,9 +3942,10 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, direction = 1; } else { - sp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : src->shared->size); - dp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : dst->shared->size); - bp = _bkg ? (uint8_t *)_bkg + (nelmts - 1) * (bkg_stride ? bkg_stride : dst->shared->size) : NULL; + sp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : src->shared->size); + dp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : dst->shared->size); + bp = _bkg ? (uint8_t *)_bkg + (nelmts - 1) * (bkg_stride ? bkg_stride : dst->shared->size) + : NULL; direction = -1; } From 06da7574aa5119dc98e2fa65459029fefe1c6b7a Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Thu, 28 Mar 2024 21:31:20 -0500 Subject: [PATCH 06/10] Revert inappropriate use of mempy to memmove in H5T__conv_array --- src/H5Tconv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/H5Tconv.c b/src/H5Tconv.c index 2a03130e523..5e9fe80e70b 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -3987,7 +3987,7 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, tmp_conv_ctx.u.conv.recursive = true; for (size_t elmtno = 0; elmtno < nelmts; elmtno++) { /* Copy the source array into the correct location for the destination */ - memcpy(dp, sp, src->shared->size); + memmove(dp, sp, src->shared->size); /* Convert array */ if (H5T_convert_with_ctx(priv->tpath, tsrc_cpy, tdst_cpy, &tmp_conv_ctx, From aba6fefc59d81d9919385caab6888fea7a064023 Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Fri, 29 Mar 2024 11:31:42 -0500 Subject: [PATCH 07/10] Add testing --- test/dtypes.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 166 insertions(+), 1 deletion(-) diff --git a/test/dtypes.c b/test/dtypes.c index bebaef0b602..43161f68863 100644 --- a/test/dtypes.c +++ b/test/dtypes.c @@ -75,7 +75,7 @@ static const char *FILENAME[] = {"dtypes0", "dtypes1", "dtypes2", "dtypes3", "dtypes4", "dtypes5", "dtypes6", "dtypes7", "dtypes8", "dtypes9", - "dtypes10", "dtypes11", NULL}; + "dtypes10", "dtypes11", "dtypes12", NULL}; #define TESTFILE "bad_compound.h5" @@ -6466,6 +6466,170 @@ test__Float16(void) #endif } +/*------------------------------------------------------------------------- + * Function: test_array_cmpd_vl + * + * Purpose: Tests that conversion occurs correctly with an array of + * arrays of compounds containing a variable length sequence. + * + * Return: Success: 0 + * + * Failure: number of errors + * + *------------------------------------------------------------------------- + */ +static int +test_array_cmpd_vl(void) +{ + typedef struct cmpd_struct { + hvl_t vl; + } cmpd_struct; + + int int_wdata[2][3][2] = {{{0, 1}, {2, 3}, {4, 5}}, {{6, 7}, {8, 9}, {10, 11}}}; + cmpd_struct wdata[2][3]; + cmpd_struct rdata[2][3]; + hid_t file; + hid_t vl_tid, cmpd_tid, inner_array_tid, outer_array_tid; + hid_t space_id; + hid_t dset_id; + hsize_t dim1[1]; + char filename[1024]; + + TESTING("array of arrays of compounds with a vlen"); + + /* Create File */ + h5_fixname(FILENAME[12], H5P_DEFAULT, filename, sizeof filename); + if ((file = H5Fcreate(filename, H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT)) < 0) { + H5_FAILED(); + AT(); + printf("Can't create file!\n"); + goto error; + } /* end if */ + + /* Create VL of ints datatype */ + if ((vl_tid = H5Tvlen_create(H5T_NATIVE_INT)) < 0) { + H5_FAILED(); + AT(); + printf("Can't create datatype!\n"); + goto error; + } /* end if */ + + /* Create compound datatype */ + if ((cmpd_tid = H5Tcreate(H5T_COMPOUND, sizeof(struct cmpd_struct))) < 0) { + H5_FAILED(); + AT(); + printf("Can't create datatype!\n"); + goto error; + } /* end if */ + + if (H5Tinsert(cmpd_tid, "vl", HOFFSET(struct cmpd_struct, vl), vl_tid) < 0) { + H5_FAILED(); + AT(); + printf("Can't insert field 'vl'\n"); + goto error; + } /* end if */ + + /* Create inner array type */ + dim1[0] = 3; + if ((inner_array_tid = H5Tarray_create2(cmpd_tid, 1, dim1)) < 0) { + H5_FAILED(); + AT(); + printf("Can't create datatype!\n"); + goto error; + } /* end if */ + + /* Create outer array type */ + dim1[0] = 2; + if ((outer_array_tid = H5Tarray_create2(inner_array_tid, 1, dim1)) < 0) { + H5_FAILED(); + AT(); + printf("Can't create datatype!\n"); + goto error; + } /* end if */ + + /* Create space, dataset */ + dim1[0] = 1; + if ((space_id = H5Screate_simple(1, dim1, NULL)) < 0) { + H5_FAILED(); + AT(); + printf("Can't create space\n"); + goto error; + } /* end if */ + + if ((dset_id = H5Dcreate2(file, "Dataset", outer_array_tid, space_id, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < + 0) { + H5_FAILED(); + AT(); + printf("Can't create dataset\n"); + goto error; + } /* end if */ + + /* Initialize wdata */ + for (int i = 0; i < 2; i++) + for (int j = 0; j < 3; j++) { + wdata[i][j].vl.len = 2; + wdata[i][j].vl.p = int_wdata[i][j]; + } + + if (H5Dwrite(dset_id, outer_array_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, wdata) < 0) { + H5_FAILED(); + AT(); + printf("Can't write data\n"); + goto error; + } /* end if */ + + /* Initialize rdata */ + (void)memset(rdata, 0, sizeof(rdata)); + + /* Read data */ + if (H5Dread(dset_id, outer_array_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, rdata) < 0) { + H5_FAILED(); + AT(); + printf("Can't read data\n"); + goto error; + } /* end if */ + + /* Check for correctness of read data */ + for (int i = 0; i < 2; i++) + for (int j = 0; j < 3; j++) + if (rdata[i][j].vl.len != 2 || ((int *)rdata[i][j].vl.p)[0] != int_wdata[i][j][0] || ((int *)rdata[i][j].vl.p)[1] != int_wdata[i][j][1]) { + H5_FAILED(); + AT(); + printf("incorrect read data at [%d][%d]\n", i, j); + goto error; + } + + /* Reclaim memory */ + if (H5Treclaim(outer_array_tid, space_id, H5P_DEFAULT, rdata) < 0) { + H5_FAILED(); + AT(); + printf("Can't reclaim memory\n"); + goto error; + } /* end if */ + + /* Close */ + if (H5Dclose(dset_id) < 0) + goto error; + if (H5Tclose(outer_array_tid) < 0) + goto error; + if (H5Tclose(inner_array_tid) < 0) + goto error; + if (H5Tclose(cmpd_tid) < 0) + goto error; + if (H5Tclose(vl_tid) < 0) + goto error; + if (H5Sclose(space_id) < 0) + goto error; + if (H5Fclose(file) < 0) + goto error; + + PASSED(); + return 0; + +error: + return 1; +} /* end test_array_cmpd_vl() */ + /*------------------------------------------------------------------------- * Function: test_encode * @@ -9702,6 +9866,7 @@ main(void) nerrors += test_bitfield_funcs(); nerrors += test_opaque(); nerrors += test_set_order(); + nerrors += test_array_cmpd_vl(); nerrors += test__Float16(); From 7559255debda152b186e19c104928fa620c90bf6 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 29 Mar 2024 16:33:29 +0000 Subject: [PATCH 08/10] Committing clang-format changes --- test/dtypes.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/dtypes.c b/test/dtypes.c index 43161f68863..31ae365a807 100644 --- a/test/dtypes.c +++ b/test/dtypes.c @@ -73,8 +73,8 @@ } \ } while (0) -static const char *FILENAME[] = {"dtypes0", "dtypes1", "dtypes2", "dtypes3", "dtypes4", - "dtypes5", "dtypes6", "dtypes7", "dtypes8", "dtypes9", +static const char *FILENAME[] = {"dtypes0", "dtypes1", "dtypes2", "dtypes3", "dtypes4", + "dtypes5", "dtypes6", "dtypes7", "dtypes8", "dtypes9", "dtypes10", "dtypes11", "dtypes12", NULL}; #define TESTFILE "bad_compound.h5" @@ -6556,8 +6556,8 @@ test_array_cmpd_vl(void) goto error; } /* end if */ - if ((dset_id = H5Dcreate2(file, "Dataset", outer_array_tid, space_id, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < - 0) { + if ((dset_id = H5Dcreate2(file, "Dataset", outer_array_tid, space_id, H5P_DEFAULT, H5P_DEFAULT, + H5P_DEFAULT)) < 0) { H5_FAILED(); AT(); printf("Can't create dataset\n"); @@ -6568,7 +6568,7 @@ test_array_cmpd_vl(void) for (int i = 0; i < 2; i++) for (int j = 0; j < 3; j++) { wdata[i][j].vl.len = 2; - wdata[i][j].vl.p = int_wdata[i][j]; + wdata[i][j].vl.p = int_wdata[i][j]; } if (H5Dwrite(dset_id, outer_array_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, wdata) < 0) { @@ -6592,7 +6592,8 @@ test_array_cmpd_vl(void) /* Check for correctness of read data */ for (int i = 0; i < 2; i++) for (int j = 0; j < 3; j++) - if (rdata[i][j].vl.len != 2 || ((int *)rdata[i][j].vl.p)[0] != int_wdata[i][j][0] || ((int *)rdata[i][j].vl.p)[1] != int_wdata[i][j][1]) { + if (rdata[i][j].vl.len != 2 || ((int *)rdata[i][j].vl.p)[0] != int_wdata[i][j][0] || + ((int *)rdata[i][j].vl.p)[1] != int_wdata[i][j][1]) { H5_FAILED(); AT(); printf("incorrect read data at [%d][%d]\n", i, j); From 585861e46508d5b65b42f0745d0bc26fe5fdd9a4 Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Fri, 29 Mar 2024 12:01:13 -0500 Subject: [PATCH 09/10] Add RELEASE.txt note and overwrite test case. --- release_docs/RELEASE.txt | 8 +++++++ test/dtypes.c | 45 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index fbd6b78e547..75e0f925fea 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -693,6 +693,14 @@ Bug Fixes since HDF5-1.14.0 release Library ------- + - Fixed a bug when using array datatypes with certain parent types + + Array datatype conversion would never use a background buffer, even if the + array's parent type (what the array is an array of) required a background + buffer for conversion. This resulted in crashes in some cases when using + an array of compound, variable length, or reference datatypes. Array types + now use a background buffer if needed by the parent type. + - Fixed an issue with the Subfiling VFD and multiple opens of a file diff --git a/test/dtypes.c b/test/dtypes.c index 43161f68863..907f58bd8d8 100644 --- a/test/dtypes.c +++ b/test/dtypes.c @@ -6571,6 +6571,51 @@ test_array_cmpd_vl(void) wdata[i][j].vl.p = int_wdata[i][j]; } + /* Write data */ + if (H5Dwrite(dset_id, outer_array_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, wdata) < 0) { + H5_FAILED(); + AT(); + printf("Can't write data\n"); + goto error; + } /* end if */ + + /* Initialize rdata */ + (void)memset(rdata, 0, sizeof(rdata)); + + /* Read data */ + if (H5Dread(dset_id, outer_array_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, rdata) < 0) { + H5_FAILED(); + AT(); + printf("Can't read data\n"); + goto error; + } /* end if */ + + /* Check for correctness of read data */ + for (int i = 0; i < 2; i++) + for (int j = 0; j < 3; j++) + if (rdata[i][j].vl.len != 2 || ((int *)rdata[i][j].vl.p)[0] != int_wdata[i][j][0] || ((int *)rdata[i][j].vl.p)[1] != int_wdata[i][j][1]) { + H5_FAILED(); + AT(); + printf("incorrect read data at [%d][%d]\n", i, j); + goto error; + } + + /* Reclaim memory */ + if (H5Treclaim(outer_array_tid, space_id, H5P_DEFAULT, rdata) < 0) { + H5_FAILED(); + AT(); + printf("Can't reclaim memory\n"); + goto error; + } /* end if */ + + /* Adjust write buffer */ + for (int i = 0; i < 2; i++) + for (int j = 0; j < 3; j++) { + int_wdata[i][j][0] += 100; + int_wdata[i][j][1] += 100; + } + + /* Overwrite dataset with adjusted wdata */ if (H5Dwrite(dset_id, outer_array_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, wdata) < 0) { H5_FAILED(); AT(); From fd257de2ca2b542e7836a20150d867664f846173 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 29 Mar 2024 17:03:26 +0000 Subject: [PATCH 10/10] Committing clang-format changes --- test/dtypes.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/dtypes.c b/test/dtypes.c index 9d1d7bb646d..94a52155836 100644 --- a/test/dtypes.c +++ b/test/dtypes.c @@ -6593,7 +6593,8 @@ test_array_cmpd_vl(void) /* Check for correctness of read data */ for (int i = 0; i < 2; i++) for (int j = 0; j < 3; j++) - if (rdata[i][j].vl.len != 2 || ((int *)rdata[i][j].vl.p)[0] != int_wdata[i][j][0] || ((int *)rdata[i][j].vl.p)[1] != int_wdata[i][j][1]) { + if (rdata[i][j].vl.len != 2 || ((int *)rdata[i][j].vl.p)[0] != int_wdata[i][j][0] || + ((int *)rdata[i][j].vl.p)[1] != int_wdata[i][j][1]) { H5_FAILED(); AT(); printf("incorrect read data at [%d][%d]\n", i, j);