From eed2ea424b233c451c81569aa2df48c99bc8c79e Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Fri, 2 Oct 2009 15:40:01 -0500 Subject: [svn-r17585] Purpose: Fix bug 1597 Description: When copying a dataset using a vlen inside a compound, the various dataset copying callbacks would allocate a background buffer but would not use it when converting from disk to memory, only memory to disk. This caused an assertion failure as compounds always need a background buffer. These callbacks have been modified to use the background buffer for both conversions. Tested: jam, linew, smirom (h5committest) --- release_docs/RELEASE.txt | 2 + src/H5Dchunk.c | 2 +- src/H5Dcompact.c | 12 +- src/H5Dcontig.c | 2 +- src/H5Tconv.c | 85 ++++---- test/objcopy.c | 521 ++++++++++++++++++++++++++++++++++++++++++++++- 6 files changed, 571 insertions(+), 53 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 39b7641..e7185a8 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -191,6 +191,8 @@ Bug Fixes since HDF5-1.8.0 release Library ------- + - Fixed an assertion failure that occurred when H5Ocopy was called on a + dataset using a vlen inside a compound. (NAF - 2009/10/02 - 1597) - Fixed incorrect return value for H5Pget_filter_by_id1/2 in H5Ppublic.h. (NAF - 2009/09/25 - 1620) - Fixed a bug where properties weren't being compared with the registered diff --git a/src/H5Dchunk.c b/src/H5Dchunk.c index 62a3941..8788d86 100644 --- a/src/H5Dchunk.c +++ b/src/H5Dchunk.c @@ -4210,7 +4210,7 @@ H5D_chunk_copy_cb(const H5D_chunk_rec_t *chunk_rec, void *_udata) /* Convert from source file to memory */ H5_CHECK_OVERFLOW(udata->nelmts, uint32_t, size_t); - if(H5T_convert(tpath_src_mem, tid_src, tid_mem, (size_t)udata->nelmts, (size_t)0, (size_t)0, buf, NULL, udata->idx_info_dst->dxpl_id) < 0) + if(H5T_convert(tpath_src_mem, tid_src, tid_mem, (size_t)udata->nelmts, (size_t)0, (size_t)0, buf, bkg, udata->idx_info_dst->dxpl_id) < 0) HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, H5_ITER_ERROR, "datatype conversion failed") /* Copy into another buffer, to reclaim memory later */ diff --git a/src/H5Dcompact.c b/src/H5Dcompact.c index b52bd2c..33b92de 100644 --- a/src/H5Dcompact.c +++ b/src/H5Dcompact.c @@ -484,15 +484,19 @@ H5D_compact_copy(H5F_t *f_src, H5O_storage_compact_t *storage_src, H5F_t *f_dst, HDmemcpy(buf, storage_src->buf, storage_src->size); + /* allocate temporary bkg buff for data conversion */ + if(NULL == (bkg = H5FL_BLK_MALLOC(type_conv, buf_size))) + HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed") + /* Convert from source file to memory */ - if(H5T_convert(tpath_src_mem, tid_src, tid_mem, nelmts, (size_t)0, (size_t)0, buf, NULL, dxpl_id) < 0) + if(H5T_convert(tpath_src_mem, tid_src, tid_mem, nelmts, (size_t)0, (size_t)0, buf, bkg, dxpl_id) < 0) HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "datatype conversion failed") + /* Copy into another buffer, to reclaim memory later */ HDmemcpy(reclaim_buf, buf, buf_size); - /* allocate temporary bkg buff for data conversion */ - if(NULL == (bkg = H5FL_BLK_CALLOC(type_conv, buf_size))) - HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed") + /* Set background buffer to all zeros */ + HDmemset(bkg, 0, buf_size); /* Convert from memory to destination file */ if(H5T_convert(tpath_mem_dst, tid_mem, tid_dst, nelmts, (size_t)0, (size_t)0, buf, bkg, dxpl_id) < 0) diff --git a/src/H5Dcontig.c b/src/H5Dcontig.c index 0ff3964..3e88be6 100644 --- a/src/H5Dcontig.c +++ b/src/H5Dcontig.c @@ -1396,7 +1396,7 @@ H5D_contig_copy(H5F_t *f_src, const H5O_storage_contig_t *storage_src, /* Perform datatype conversion, if necessary */ if(is_vlen) { /* Convert from source file to memory */ - if(H5T_convert(tpath_src_mem, tid_src, tid_mem, nelmts, (size_t)0, (size_t)0, buf, NULL, dxpl_id) < 0) + if(H5T_convert(tpath_src_mem, tid_src, tid_mem, nelmts, (size_t)0, (size_t)0, buf, bkg, dxpl_id) < 0) HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "datatype conversion failed") /* Copy into another buffer, to reclaim memory later */ diff --git a/src/H5Tconv.c b/src/H5Tconv.c index 08e7fa1..ab9e703 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -1935,51 +1935,48 @@ H5T_conv_struct_init(H5T_t *src, H5T_t *dst, H5T_cdata_t *cdata, hid_t dxpl_id) } /* end if */ } /* end for */ - /* Check if we need a background buffer */ - if(H5T_detect_class(src, H5T_COMPOUND) == TRUE || H5T_detect_class(dst, H5T_COMPOUND) == TRUE) { - cdata->need_bkg = H5T_BKG_YES; - - if(src_nmembs < dst_nmembs) { - priv->subset_info.subset = H5T_SUBSET_SRC; - for(i = 0; i < src_nmembs; i++) { - /* If any of source members doesn't have counterpart in the same - * order or there's conversion between members, don't do the - * optimization. - */ - if(src2dst[i] != i || (src->shared->u.compnd.memb[i].offset != dst->shared->u.compnd.memb[i].offset) || (priv->memb_path[i])->is_noop == FALSE) { - priv->subset_info.subset = H5T_SUBSET_FALSE; - break; - } /* end if */ - } /* end for */ - /* Compute the size of the data to be copied for each element. It - * may be smaller than either src or dst if there is extra space at - * the end of src. - */ - if(priv->subset_info.subset == H5T_SUBSET_SRC) - priv->subset_info.copy_size = src->shared->u.compnd.memb[src_nmembs-1].offset - + src->shared->u.compnd.memb[src_nmembs-1].size; - } else if(dst_nmembs < src_nmembs) { - priv->subset_info.subset = H5T_SUBSET_DST; - for(i = 0; i < dst_nmembs; i++) { - /* If any of source members doesn't have counterpart in the same order or - * there's conversion between members, don't do the optimization. */ - if(src2dst[i] != i || (src->shared->u.compnd.memb[i].offset != dst->shared->u.compnd.memb[i].offset) || (priv->memb_path[i])->is_noop == FALSE) { - priv->subset_info.subset = H5T_SUBSET_FALSE; - break; - } - } /* end for */ - /* Compute the size of the data to be copied for each element. It - * may be smaller than either src or dst if there is extra space at - * the end of dst. - */ - if(priv->subset_info.subset == H5T_SUBSET_DST) - priv->subset_info.copy_size = dst->shared->u.compnd.memb[dst_nmembs-1].offset - + dst->shared->u.compnd.memb[dst_nmembs-1].size; - } else /* If the numbers of source and dest members are equal and no conversion is needed, - * the case should have been handled as noop earlier in H5Dio.c. */ - ; + /* The compound conversion functions need a background buffer */ + cdata->need_bkg = H5T_BKG_YES; - } /* end if */ + if(src_nmembs < dst_nmembs) { + priv->subset_info.subset = H5T_SUBSET_SRC; + for(i = 0; i < src_nmembs; i++) { + /* If any of source members doesn't have counterpart in the same + * order or there's conversion between members, don't do the + * optimization. + */ + if(src2dst[i] != i || (src->shared->u.compnd.memb[i].offset != dst->shared->u.compnd.memb[i].offset) || (priv->memb_path[i])->is_noop == FALSE) { + priv->subset_info.subset = H5T_SUBSET_FALSE; + break; + } /* end if */ + } /* end for */ + /* Compute the size of the data to be copied for each element. It + * may be smaller than either src or dst if there is extra space at + * the end of src. + */ + if(priv->subset_info.subset == H5T_SUBSET_SRC) + priv->subset_info.copy_size = src->shared->u.compnd.memb[src_nmembs-1].offset + + src->shared->u.compnd.memb[src_nmembs-1].size; + } else if(dst_nmembs < src_nmembs) { + priv->subset_info.subset = H5T_SUBSET_DST; + for(i = 0; i < dst_nmembs; i++) { + /* If any of source members doesn't have counterpart in the same order or + * there's conversion between members, don't do the optimization. */ + if(src2dst[i] != i || (src->shared->u.compnd.memb[i].offset != dst->shared->u.compnd.memb[i].offset) || (priv->memb_path[i])->is_noop == FALSE) { + priv->subset_info.subset = H5T_SUBSET_FALSE; + break; + } + } /* end for */ + /* Compute the size of the data to be copied for each element. It + * may be smaller than either src or dst if there is extra space at + * the end of dst. + */ + if(priv->subset_info.subset == H5T_SUBSET_DST) + priv->subset_info.copy_size = dst->shared->u.compnd.memb[dst_nmembs-1].offset + + dst->shared->u.compnd.memb[dst_nmembs-1].size; + } else /* If the numbers of source and dest members are equal and no conversion is needed, + * the case should have been handled as noop earlier in H5Dio.c. */ + ; cdata->recalc = FALSE; diff --git a/test/objcopy.c b/test/objcopy.c index 4bd4662..50dabcc 100755 --- a/test/objcopy.c +++ b/test/objcopy.c @@ -82,6 +82,7 @@ const char *FILENAME[] = { #define NAME_DATASET_MULTI_OHDR2 "dataset_multi_ohdr2" #define NAME_DATASET_VL "dataset_vl" #define NAME_DATASET_VL_VL "dataset_vl_vl" +#define NAME_DATASET_CMPD_VL "dataset_cmpd_vl" #define NAME_DATASET_SUB_SUB "/g0/g00/g000/dataset_simple" #define NAME_GROUP_UNCOPIED "/uncopied" #define NAME_GROUP_EMPTY "/empty" @@ -842,8 +843,82 @@ compare_data(hid_t parent1, hid_t parent2, hid_t pid, hid_t tid, size_t nelmts, /* Check size of each element */ if((elmt_size = H5Tget_size(tid)) == 0) TEST_ERROR - /* Check for vlen datatype */ - if(H5Tdetect_class(tid, H5T_VLEN) == TRUE) { + /* If the type is a compound containing a vlen, loop over all elements for + * each compound member. Compounds containing reference are not supported + * yet. */ + if((H5Tget_class(tid) == H5T_COMPOUND) + && (H5Tdetect_class(tid, H5T_VLEN) == TRUE)) { + hid_t memb_id; /* Member id */ + const uint8_t *memb1; /* Pointer to current member */ + const uint8_t *memb2; /* Pointer to current member */ + int nmembs; /* Number of members */ + size_t memb_off; /* Member offset */ + size_t memb_size; /* Member size */ + unsigned memb_idx; /* Member index */ + size_t elmt; /* Current element */ + + /* Get number of members in compound */ + if((nmembs = H5Tget_nmembers(tid)) < 0) TEST_ERROR + + /* Loop over members */ + for(memb_idx=0; memb_idx<(unsigned)nmembs; memb_idx++) { + /* Get member offset. Note that we cannot check for an error here. + */ + memb_off = H5Tget_member_offset(tid, memb_idx); + + /* Get member id */ + if((memb_id = H5Tget_member_type(tid, memb_idx)) < 0) TEST_ERROR + + /* Get member size */ + if((memb_size = H5Tget_size(memb_id)) == 0) TEST_ERROR + + /* Set up pointers to member in the first element */ + memb1 = (const uint8_t *)buf1 + memb_off; + memb2 = (const uint8_t *)buf2 + memb_off; + + /* Check if this member contains (or is) a vlen */ + if(H5Tget_class(memb_id) == H5T_VLEN) { + hid_t base_id; /* vlen base type id */ + + /* Get base type of vlen datatype */ + if((base_id = H5Tget_super(memb_id)) < 0) TEST_ERROR + + /* Iterate over all elements, recursively calling this function + * for each */ + for(elmt=0; elmtlen + != ((const hvl_t *)memb2)->len) + TEST_ERROR + + /* Check vlen data */ + if(!compare_data(parent1, parent2, pid, base_id, + ((const hvl_t *)memb1)->len, + ((const hvl_t *)memb1)->p, + ((const hvl_t *)memb2)->p, obj_owner)) + TEST_ERROR + + /* Update member pointers */ + memb1 += elmt_size; + memb2 += elmt_size; + } /* end for */ + } else { + /* vlens cannot currently be nested below the top layer of a + * compound */ + HDassert(H5Tdetect_class(memb_id, H5T_VLEN) == FALSE); + + /* Iterate over all elements, calling memcmp() for each */ + for(elmt=0; elmt