Refactor datatype conversion code to use pointers rather than IDs (#4104)

The datatype conversion code previously used IDs for the source and
destination datatypes rather than pointers to the internal structures
for those datatypes. This was mostly due to the need for an ID for these
datatypes that can be passed to an application-registered datatype
conversion function or datatype conversion exception function. However,
using IDs internally caused a lot of unnecessary ID lookups and hurt
performance of datatype conversions in general. This was especially
problematic for compound datatype conversions, where the ID lookups were
occuring on every member of every compound element of a dataset. The
code has now been refactored to use pointers internally and only create
IDs for datatypes when necessary.

Fixed a test issue in dt_arith where a library datatype conversion
function was being cast to an application conversion function. Since the
two have different prototypes, this started failing after the parameters
for a library conversion function changed from hid_t to H5T_t * and an
extra parameter was added. This appears to have worked coincidentally in
the past since the only different between a library conversion function
and application conversion function was an extra DXPL parameter at the
end of an application conversion function

Fixed an issue where memory wasn't being freed in the h5fc_chk_idx test
program. Even though the program exits quickly after allocating the
memory, it still causes failures when testing with -fsanitize=address

1 files changed, 51 insertions, 69 deletions

diff --git a/src/H5Aint.c b/src/H5Aint.c
index 170edc1..160e359 100644
--- a/src/H5Aint.c
+++ b/src/H5Aint.c
@@ -677,15 +677,16 @@ done:
 herr_t
 H5A__read(const H5A_t *attr, const H5T_t *mem_type, void *buf)
 {
-    uint8_t    *tconv_buf = NULL;         /* datatype conv buffer*/
-    uint8_t    *bkg_buf   = NULL;         /* background buffer */
-    hssize_t    snelmts;                  /* elements in attribute */
-    size_t      nelmts;                   /* elements in attribute*/
-    H5T_path_t *tpath  = NULL;            /* type conversion info    */
-    hid_t       src_id = -1, dst_id = -1; /* temporary type IDs*/
-    size_t      src_type_size;            /* size of source type     */
-    size_t      dst_type_size;            /* size of destination type */
-    size_t      buf_size;                 /* desired buffer size    */
+    uint8_t    *tconv_buf = NULL; /* datatype conv buffer*/
+    uint8_t    *bkg_buf   = NULL; /* background buffer */
+    hssize_t    snelmts;          /* elements in attribute */
+    size_t      nelmts;           /* elements in attribute*/
+    H5T_path_t *tpath    = NULL;  /* type conversion info    */
+    H5T_t      *src_type = NULL;  /* temporary datatype */
+    H5T_t      *dst_type = NULL;  /* temporary datatype */
+    size_t      src_type_size;    /* size of source type     */
+    size_t      dst_type_size;    /* size of destination type */
+    size_t      buf_size;         /* desired buffer size    */
     herr_t      ret_value = SUCCEED;
 
     FUNC_ENTER_PACKAGE_TAG(attr->oloc.addr)
@@ -722,10 +723,10 @@ H5A__read(const H5A_t *attr, const H5T_t *mem_type, void *buf)
             if (!H5T_path_noop(tpath)) {
                 H5T_bkg_t need_bkg; /* Background buffer type */
 
-                if ((src_id = H5I_register(H5I_DATATYPE, H5T_copy(attr->shared->dt, H5T_COPY_ALL), false)) <
-                        0 ||
-                    (dst_id = H5I_register(H5I_DATATYPE, H5T_copy(mem_type, H5T_COPY_ALL), false)) < 0)
-                    HGOTO_ERROR(H5E_ATTR, H5E_CANTREGISTER, FAIL, "unable to register types for conversion");
+                if (NULL == (src_type = H5T_copy(attr->shared->dt, H5T_COPY_ALL)))
+                    HGOTO_ERROR(H5E_ATTR, H5E_CANTCOPY, FAIL, "unable to copy attribute datatype");
+                if (NULL == (dst_type = H5T_copy(mem_type, H5T_COPY_ALL)))
+                    HGOTO_ERROR(H5E_ATTR, H5E_CANTCOPY, FAIL, "unable to copy memory datatype");
 
                 /* Get the maximum buffer size needed and allocate it */
                 buf_size = nelmts * MAX(src_type_size, dst_type_size);
@@ -751,8 +752,9 @@ H5A__read(const H5A_t *attr, const H5T_t *mem_type, void *buf)
                 }
 
                 /* Perform datatype conversion.  */
-                if (H5T_convert(tpath, src_id, dst_id, nelmts, (size_t)0, (size_t)0, tconv_buf, bkg_buf) < 0)
-                    HGOTO_ERROR(H5E_ATTR, H5E_CANTENCODE, FAIL, "datatype conversion failed");
+                if (H5T_convert(tpath, src_type, dst_type, nelmts, (size_t)0, (size_t)0, tconv_buf, bkg_buf) <
+                    0)
+                    HGOTO_ERROR(H5E_ATTR, H5E_CANTCONVERT, FAIL, "datatype conversion failed");
 
                 /* Copy the converted data into the user's buffer */
                 H5MM_memcpy(buf, tconv_buf, (dst_type_size * nelmts));
@@ -769,10 +771,10 @@ H5A__read(const H5A_t *attr, const H5T_t *mem_type, void *buf)
 
 done:
     /* Release resources */
-    if (src_id >= 0 && H5I_dec_ref(src_id) < 0)
-        HDONE_ERROR(H5E_ATTR, H5E_CANTDEC, FAIL, "unable to close temporary object");
-    if (dst_id >= 0 && H5I_dec_ref(dst_id) < 0)
-        HDONE_ERROR(H5E_ATTR, H5E_CANTDEC, FAIL, "unable to close temporary object");
+    if (src_type && H5T_close(src_type) < 0)
+        HDONE_ERROR(H5E_ATTR, H5E_CANTCLOSEOBJ, FAIL, "unable to close temporary datatype");
+    if (dst_type && H5T_close(dst_type) < 0)
+        HDONE_ERROR(H5E_ATTR, H5E_CANTCLOSEOBJ, FAIL, "unable to close temporary datatype");
     if (tconv_buf)
         tconv_buf = H5FL_BLK_FREE(attr_buf, tconv_buf);
     if (bkg_buf)
@@ -799,15 +801,16 @@ done:
 herr_t
 H5A__write(H5A_t *attr, const H5T_t *mem_type, const void *buf)
 {
-    uint8_t    *tconv_buf = NULL;         /* datatype conv buffer */
-    uint8_t    *bkg_buf   = NULL;         /* temp conversion buffer */
-    hssize_t    snelmts;                  /* elements in attribute */
-    size_t      nelmts;                   /* elements in attribute */
-    H5T_path_t *tpath  = NULL;            /* conversion information*/
-    hid_t       src_id = -1, dst_id = -1; /* temporary type IDs */
-    size_t      src_type_size;            /* size of source type    */
-    size_t      dst_type_size;            /* size of destination type*/
-    size_t      buf_size;                 /* desired buffer size    */
+    uint8_t    *tconv_buf = NULL; /* datatype conv buffer */
+    uint8_t    *bkg_buf   = NULL; /* temp conversion buffer */
+    hssize_t    snelmts;          /* elements in attribute */
+    size_t      nelmts;           /* elements in attribute */
+    H5T_path_t *tpath    = NULL;  /* conversion information*/
+    H5T_t      *src_type = NULL;  /* temporary datatype */
+    H5T_t      *dst_type = NULL;  /* temporary datatype */
+    size_t      src_type_size;    /* size of source type    */
+    size_t      dst_type_size;    /* size of destination type*/
+    size_t      buf_size;         /* desired buffer size    */
     herr_t      ret_value = SUCCEED;
 
     FUNC_ENTER_PACKAGE_TAG(attr->oloc.addr)
@@ -840,9 +843,10 @@ H5A__write(H5A_t *attr, const H5T_t *mem_type, const void *buf)
         if (!H5T_path_noop(tpath)) {
             H5T_bkg_t need_bkg; /* Background buffer type */
 
-            if ((src_id = H5I_register(H5I_DATATYPE, H5T_copy(mem_type, H5T_COPY_ALL), false)) < 0 ||
-                (dst_id = H5I_register(H5I_DATATYPE, H5T_copy(attr->shared->dt, H5T_COPY_ALL), false)) < 0)
-                HGOTO_ERROR(H5E_ATTR, H5E_CANTREGISTER, FAIL, "unable to register types for conversion");
+            if (NULL == (src_type = H5T_copy(mem_type, H5T_COPY_ALL)))
+                HGOTO_ERROR(H5E_ATTR, H5E_CANTCOPY, FAIL, "unable to copy memory datatype");
+            if (NULL == (dst_type = H5T_copy(attr->shared->dt, H5T_COPY_ALL)))
+                HGOTO_ERROR(H5E_ATTR, H5E_CANTCOPY, FAIL, "unable to copy attribute datatype");
 
             /* Get the maximum buffer size needed and allocate it */
             buf_size = nelmts * MAX(src_type_size, dst_type_size);
@@ -876,8 +880,8 @@ H5A__write(H5A_t *attr, const H5T_t *mem_type, const void *buf)
             }
 
             /* Perform datatype conversion */
-            if (H5T_convert(tpath, src_id, dst_id, nelmts, (size_t)0, (size_t)0, tconv_buf, bkg_buf) < 0)
-                HGOTO_ERROR(H5E_ATTR, H5E_CANTENCODE, FAIL, "datatype conversion failed");
+            if (H5T_convert(tpath, src_type, dst_type, nelmts, (size_t)0, (size_t)0, tconv_buf, bkg_buf) < 0)
+                HGOTO_ERROR(H5E_ATTR, H5E_CANTCONVERT, FAIL, "datatype conversion failed");
 
             /* Free the previous attribute data buffer, if there is one */
             if (attr->shared->data)
@@ -907,10 +911,10 @@ H5A__write(H5A_t *attr, const H5T_t *mem_type, const void *buf)
 
 done:
     /* Release resources */
-    if (src_id >= 0 && H5I_dec_ref(src_id) < 0)
-        HDONE_ERROR(H5E_ATTR, H5E_CANTDEC, FAIL, "unable to close temporary object");
-    if (dst_id >= 0 && H5I_dec_ref(dst_id) < 0)
-        HDONE_ERROR(H5E_ATTR, H5E_CANTDEC, FAIL, "unable to close temporary object");
+    if (src_type && H5T_close(src_type) < 0)
+        HDONE_ERROR(H5E_ATTR, H5E_CANTCLOSEOBJ, FAIL, "unable to close temporary datatype");
+    if (dst_type && H5T_close(dst_type) < 0)
+        HDONE_ERROR(H5E_ATTR, H5E_CANTCLOSEOBJ, FAIL, "unable to close temporary datatype");
     if (tconv_buf)
         tconv_buf = H5FL_BLK_FREE(attr_buf, tconv_buf);
     if (bkg_buf)
@@ -2079,9 +2083,7 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
                     H5O_copy_t H5_ATTR_NDEBUG_UNUSED *cpy_info)
 {
     H5A_t   *attr_dst    = NULL; /* Destination attribute */
-    hid_t    tid_src     = -1;   /* Datatype ID for source datatype */
-    hid_t    tid_dst     = -1;   /* Datatype ID for destination datatype */
-    hid_t    tid_mem     = -1;   /* Datatype ID for memory datatype */
+    H5T_t   *dt_mem      = NULL; /* Memory datatype */
     void    *buf         = NULL; /* Buffer for copying data */
     void    *reclaim_buf = NULL; /* Buffer for reclaiming data */
     void    *bkg_buf     = NULL; /* Background buffer */
@@ -2195,7 +2197,6 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
         /* Check if we need to convert data */
         if (H5T_detect_class(attr_src->shared->dt, H5T_VLEN, false) > 0) {
             H5T_path_t *tpath_src_mem, *tpath_mem_dst; /* Datatype conversion paths */
-            H5T_t      *dt_mem;                        /* Memory datatype */
             size_t      src_dt_size;                   /* Source datatype size */
             size_t      tmp_dt_size;                   /* Temp. datatype size */
             size_t      max_dt_size;                   /* Max atatype size */
@@ -2204,20 +2205,9 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
             size_t      nelmts;                        /* Number of elements in buffer */
             size_t      buf_size;                      /* Size of copy buffer */
 
-            /* Create datatype ID for src datatype */
-            if ((tid_src = H5I_register(H5I_DATATYPE, attr_src->shared->dt, false)) < 0)
-                HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, NULL, "unable to register source file datatype");
-
             /* create a memory copy of the variable-length datatype */
             if (NULL == (dt_mem = H5T_copy(attr_src->shared->dt, H5T_COPY_TRANSIENT)))
                 HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, NULL, "unable to copy");
-            if ((tid_mem = H5I_register(H5I_DATATYPE, dt_mem, false)) < 0)
-                HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, NULL, "unable to register memory datatype");
-
-            /* create variable-length datatype at the destination file */
-            if ((tid_dst = H5I_register(H5I_DATATYPE, attr_dst->shared->dt, false)) < 0)
-                HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, NULL,
-                            "unable to register destination file datatype");
 
             /* Set up the conversion functions */
             if (NULL == (tpath_src_mem = H5T_path_find(attr_src->shared->dt, dt_mem)))
@@ -2273,7 +2263,8 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
                     HGOTO_ERROR(H5E_ATTR, H5E_CANTALLOC, NULL, "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, bkg_buf) < 0)
+            if (H5T_convert(tpath_src_mem, attr_src->shared->dt, dt_mem, nelmts, (size_t)0, (size_t)0, buf,
+                            bkg_buf) < 0)
                 HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, NULL, "datatype conversion NULLed");
 
             H5MM_memcpy(reclaim_buf, buf, buf_size);
@@ -2283,12 +2274,13 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
                 memset(bkg_buf, 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_buf) < 0)
+            if (H5T_convert(tpath_mem_dst, dt_mem, attr_dst->shared->dt, nelmts, (size_t)0, (size_t)0, buf,
+                            bkg_buf) < 0)
                 HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, NULL, "datatype conversion NULLed");
 
             H5MM_memcpy(attr_dst->shared->data, buf, attr_dst->shared->data_size);
 
-            if (H5T_reclaim(tid_mem, buf_space, reclaim_buf) < 0)
+            if (H5T_reclaim(dt_mem, buf_space, reclaim_buf) < 0)
                 HGOTO_ERROR(H5E_DATASET, H5E_BADITER, NULL, "unable to reclaim variable-length data");
         } /* end if */
         else {
@@ -2313,19 +2305,9 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
 
 done:
     if (buf_sid > 0 && H5I_dec_ref(buf_sid) < 0)
-        HDONE_ERROR(H5E_ATTR, H5E_CANTFREE, NULL, "Can't decrement temporary dataspace ID");
-    if (tid_src > 0)
-        /* Don't decrement ID, we want to keep underlying datatype */
-        if (NULL == H5I_remove(tid_src))
-            HDONE_ERROR(H5E_ATTR, H5E_CANTFREE, NULL, "Can't decrement temporary datatype ID");
-    if (tid_dst > 0)
-        /* Don't decrement ID, we want to keep underlying datatype */
-        if (NULL == H5I_remove(tid_dst))
-            HDONE_ERROR(H5E_ATTR, H5E_CANTFREE, NULL, "Can't decrement temporary datatype ID");
-    if (tid_mem > 0)
-        /* Decrement the memory datatype ID, it's transient */
-        if (H5I_dec_ref(tid_mem) < 0)
-            HDONE_ERROR(H5E_ATTR, H5E_CANTFREE, NULL, "Can't decrement temporary datatype ID");
+        HDONE_ERROR(H5E_ATTR, H5E_CANTFREE, NULL, "can't decrement temporary dataspace ID");
+    if (dt_mem && (H5T_close(dt_mem) < 0))
+        HDONE_ERROR(H5E_ATTR, H5E_CANTCLOSEOBJ, NULL, "can't close temporary datatype");
     if (buf)
         buf = H5FL_BLK_FREE(attr_buf, buf);
     if (reclaim_buf)
@@ -2416,7 +2398,7 @@ H5A__attr_post_copy_file(const H5O_loc_t *src_oloc, const H5A_t *attr_src, H5O_l
         /* Check for expanding references */
         if (cpy_info->expand_ref) {
             /* Copy objects referenced in source buffer to destination file and set destination elements */
-            if (H5O_copy_expand_ref(file_src, H5I_INVALID_HID, attr_src->shared->dt, attr_src->shared->data,
+            if (H5O_copy_expand_ref(file_src, attr_src->shared->dt, attr_src->shared->data,
                                     attr_src->shared->data_size, file_dst, attr_dst->shared->data,
                                     cpy_info) < 0)
                 HGOTO_ERROR(H5E_ATTR, H5E_CANTCOPY, FAIL, "unable to copy reference attribute");