Fix problems with vlens and refs inside compound using H5VLget_file_t…

…ype() (HDFGroup#274)

* Fixed problems with vlens and refs inside compound using H5VLget_file_type()

* Fix date in RELEASE.txt

* Add assertions

* Move some manipulation of H5VL_object_t struct fields into the H5VL
package.

release_docs/RELEASE.txt

@@ -242,6 +242,15 @@ Bug Fixes since HDF5-1.12.0 release
 ==================================
     Library
     -------
+    - Fixed problems with vlens and refs inside compound using
+      H5VLget_file_type()
+
+        Modified library to properly ref count H5VL_object_t structs and only
+        consider file vlen and reference types to be equal if their files are
+        the same.
+
+        (NAF - 2021/01/22)
+
     - Fix bug and simplify collective metadata write operation when some ranks
         have no entries to contribute.  This fixes parallel regression test
         failures with IBM SpectrumScale MPI on the Summit system at ORNL.

src/H5Odtype.c

@@ -579,8 +579,10 @@ H5O__dtype_decode_helper(unsigned *ioflags /*in,out*/, const uint8_t **pp, H5T_t
 done:
     if (ret_value < 0)
         if (dt != NULL) {
-            if (dt->shared != NULL)
+            if (dt->shared != NULL) {
+                HDassert(!dt->shared->owned_vol_obj);
                 dt->shared = H5FL_FREE(H5T_shared_t, dt->shared);
+            } /* end if */
             dt = H5FL_FREE(H5T_t, dt);
         } /* end if */
 

src/H5T.c

@@ -1477,6 +1477,8 @@ H5T__init_package(void)
             if (copied_dtype)
                 (void)H5T_close_real(dt);
             else {
+                if (dt->shared->owned_vol_obj && H5VL_free_object(dt->shared->owned_vol_obj) < 0)
+                    HDONE_ERROR(H5E_DATATYPE, H5E_CANTCLOSEOBJ, FAIL, "unable to close owned VOL object")
                 dt->shared = H5FL_FREE(H5T_shared_t, dt->shared);
                 dt         = H5FL_FREE(H5T_t, dt);
             } /* end else */
@@ -3384,6 +3386,8 @@ H5T__create(H5T_class_t type, size_t size)
 done:
     if (NULL == ret_value) {
         if (dt) {
+            if (dt->shared->owned_vol_obj && H5VL_free_object(dt->shared->owned_vol_obj) < 0)
+                HDONE_ERROR(H5E_DATATYPE, H5E_CANTCLOSEOBJ, NULL, "unable to close owned VOL object")
             dt->shared = H5FL_FREE(H5T_shared_t, dt->shared);
             dt         = H5FL_FREE(H5T_t, dt);
         }
@@ -3426,18 +3430,24 @@ H5T__initiate_copy(const H5T_t *old_dt)
     /* Copy shared information */
     *(new_dt->shared) = *(old_dt->shared);
 
-    /* Reset VOL fields */
+    /* Increment ref count on owned VOL object */
+    if (new_dt->shared->owned_vol_obj)
+        (void)H5VL_object_inc_rc(new_dt->shared->owned_vol_obj);
+
+    /* Reset vol_obj field */
     new_dt->vol_obj               = NULL;
-    new_dt->shared->owned_vol_obj = NULL;
 
     /* Set return value */
     ret_value = new_dt;
 
 done:
     if (ret_value == NULL)
         if (new_dt) {
-            if (new_dt->shared)
+            if (new_dt->shared) {
+                if (new_dt->shared->owned_vol_obj && H5VL_free_object(new_dt->shared->owned_vol_obj) < 0)
+                    HDONE_ERROR(H5E_DATATYPE, H5E_CANTCLOSEOBJ, NULL, "unable to close owned VOL object")
                 new_dt->shared = H5FL_FREE(H5T_shared_t, new_dt->shared);
+            } /* end if */
             new_dt = H5FL_FREE(H5T_t, new_dt);
         } /* end if */
 
@@ -3766,6 +3776,8 @@ H5T_copy(const H5T_t *old_dt, H5T_copy_t method)
     if (ret_value == NULL)
         if (new_dt) {
             HDassert(new_dt->shared);
+            if (new_dt->shared->owned_vol_obj && H5VL_free_object(new_dt->shared->owned_vol_obj) < 0)
+                HDONE_ERROR(H5E_DATATYPE, H5E_CANTCLOSEOBJ, NULL, "unable to close owned VOL object")
             new_dt->shared = H5FL_FREE(H5T_shared_t, new_dt->shared);
             new_dt         = H5FL_FREE(H5T_t, new_dt);
         } /* end if */
@@ -3833,6 +3845,8 @@ H5T_copy_reopen(H5T_t *old_dt)
             /* The object is already open.  Free the H5T_shared_t struct
              * we had been using and use the one that already exists.
              * Not terribly efficient. */
+            if (new_dt->shared->owned_vol_obj && H5VL_free_object(new_dt->shared->owned_vol_obj) < 0)
+                HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCLOSEOBJ, NULL, "unable to close owned VOL object")
             new_dt->shared = H5FL_FREE(H5T_shared_t, new_dt->shared);
             new_dt->shared = reopened_fo;
 
@@ -3869,6 +3883,8 @@ H5T_copy_reopen(H5T_t *old_dt)
     if (ret_value == NULL)
         if (new_dt) {
             HDassert(new_dt->shared);
+            if (new_dt->shared->owned_vol_obj && H5VL_free_object(new_dt->shared->owned_vol_obj) < 0)
+                HDONE_ERROR(H5E_DATATYPE, H5E_CANTCLOSEOBJ, NULL, "unable to close owned VOL object")
             new_dt->shared = H5FL_FREE(H5T_shared_t, new_dt->shared);
             new_dt         = H5FL_FREE(H5T_t, new_dt);
         } /* end if */
@@ -3963,8 +3979,10 @@ H5T__alloc(void)
 done:
     if (ret_value == NULL)
         if (dt) {
-            if (dt->shared)
+            if (dt->shared) {
+                HDassert(!dt->shared->owned_vol_obj);
                 dt->shared = H5FL_FREE(H5T_shared_t, dt->shared);
+            } /* end if */
             dt = H5FL_FREE(H5T_t, dt);
         } /* end if */
 
@@ -4085,6 +4103,7 @@ H5T_close_real(H5T_t *dt)
         if (H5T__free(dt) < 0)
             HGOTO_ERROR(H5E_DATATYPE, H5E_CANTFREE, FAIL, "unable to free datatype");
 
+        HDassert(!dt->shared->owned_vol_obj);
         dt->shared = H5FL_FREE(H5T_shared_t, dt->shared);
     } /* end if */
     else
@@ -4219,6 +4238,7 @@ H5T__set_size(H5T_t *dt, size_t size)
 
     /* Check args */
     HDassert(dt);
+    HDassert(dt->shared);
     HDassert(size != 0);
     HDassert(H5T_REFERENCE != dt->shared->type);
     HDassert(!(H5T_ENUM == dt->shared->type && 0 == dt->shared->u.enumer.nmembs));
@@ -4406,10 +4426,37 @@ H5T_get_size(const H5T_t *dt)
 
     /* check args */
     HDassert(dt);
+    HDassert(dt->shared);
 
     FUNC_LEAVE_NOAPI(dt->shared->size)
 } /* end H5T_get_size() */
 
+/*-------------------------------------------------------------------------
+ * Function:  H5T_get_force_conv
+ *
+ * Purpose:   Determines if the type has forced conversion. This will be
+ *            true if and only if the type keeps a pointer to a file VOL
+ *            object internally.
+ *
+ * Return:    TRUE/FALSE (never fails)
+ *
+ * Programmer:    Neil Fortner
+ *        Thursday, January  21, 2021
+ *-------------------------------------------------------------------------
+ */
+hbool_t
+H5T_get_force_conv(const H5T_t *dt)
+{
+    /* Use FUNC_ENTER_NOAPI_NOINIT_NOERR here to avoid performance issues */
+    FUNC_ENTER_NOAPI_NOINIT_NOERR
+
+    /* check args */
+    HDassert(dt);
+    HDassert(dt->shared);
+
+    FUNC_LEAVE_NOAPI(dt->shared->force_conv)
+} /* end H5T_get_force_conv() */
+
 /*-------------------------------------------------------------------------
  * Function:  H5T_cmp
  *
@@ -4446,6 +4493,9 @@ H5T_cmp(const H5T_t *dt1, const H5T_t *dt2, hbool_t superset)
     if (dt1 == dt2)
         HGOTO_DONE(0);
 
+    HDassert(dt1->shared);
+    HDassert(dt2->shared);
+
     /* compare */
     if (dt1->shared->type < dt2->shared->type)
         HGOTO_DONE(-1);
@@ -4851,6 +4901,10 @@ H5T_cmp(const H5T_t *dt1, const H5T_t *dt2, hbool_t superset)
                         HGOTO_DONE(-1);
                     if (dt1->shared->u.atomic.u.r.loc > dt2->shared->u.atomic.u.r.loc)
                         HGOTO_DONE(1);
+                    if (dt1->shared->u.atomic.u.r.file < dt2->shared->u.atomic.u.r.file)
+                        HGOTO_DONE(-1);
+                    if (dt1->shared->u.atomic.u.r.file > dt2->shared->u.atomic.u.r.file)
+                        HGOTO_DONE(1);
                     break;
 
                 case H5T_NO_CLASS:
@@ -6229,6 +6283,7 @@ H5T_own_vol_obj(H5T_t *dt, H5VL_object_t *vol_obj)
 
     /* Take ownership */
     dt->shared->owned_vol_obj = vol_obj;
+    (void)H5VL_object_inc_rc(vol_obj);
 
 done:
     FUNC_LEAVE_NOAPI(ret_value)

src/H5Tcommit.c

@@ -147,11 +147,8 @@ H5Tcommit2(hid_t loc_id, const char *name, hid_t type_id, hid_t lcpl_id, hid_t t
         HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "unable to commit datatype")
 
     /* Set up VOL object */
-    if (NULL == (new_obj = H5FL_CALLOC(H5VL_object_t)))
-        HGOTO_ERROR(H5E_DATATYPE, H5E_CANTALLOC, FAIL, "can't allocate top object structure")
-    new_obj->connector = vol_obj->connector;
-    new_obj->connector->nrefs++;
-    new_obj->data = data;
+    if (NULL == (new_obj = H5VL_create_object(data, vol_obj->connector)))
+        HGOTO_ERROR(H5E_DATATYPE, H5E_CANTALLOC, FAIL, "can't create VOL object for committed datatype")
 
     /* Set the committed type object to the VOL connector pointer in the H5T_t struct */
     dt->vol_obj = new_obj;
@@ -301,11 +298,8 @@ H5Tcommit_anon(hid_t loc_id, hid_t type_id, hid_t tcpl_id, hid_t tapl_id)
         HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "unable to commit datatype")
 
     /* Setup VOL object */
-    if (NULL == (new_obj = H5FL_CALLOC(H5VL_object_t)))
-        HGOTO_ERROR(H5E_DATATYPE, H5E_CANTALLOC, FAIL, "can't allocate top object structure")
-    new_obj->connector = vol_obj->connector;
-    new_obj->connector->nrefs++;
-    new_obj->data = dt;
+    if (NULL == (new_obj = H5VL_create_object(dt, vol_obj->connector)))
+        HGOTO_ERROR(H5E_DATATYPE, H5E_CANTALLOC, FAIL, "can't create VOL object for committed datatype")
 
     /* Set the committed type object to the VOL connector pointer in the H5T_t struct */
     type->vol_obj = new_obj;
@@ -976,8 +970,11 @@ H5T_open(const H5G_loc_t *loc)
 done:
     if (ret_value == NULL) {
         if (dt) {
-            if (shared_fo == NULL) /* Need to free shared fo */
+            if (shared_fo == NULL) { /* Need to free shared fo */
+                if (dt->shared->owned_vol_obj && H5VL_free_object(dt->shared->owned_vol_obj) < 0)
+                    HDONE_ERROR(H5E_DATATYPE, H5E_CANTCLOSEOBJ, NULL, "unable to close owned VOL object")
                 dt->shared = H5FL_FREE(H5T_shared_t, dt->shared);
+            } /* end if */
 
             H5O_loc_free(&(dt->oloc));
             H5G_name_free(&(dt->path));

src/H5Tconv.c

@@ -2010,7 +2010,7 @@ H5T__conv_struct_init(H5T_t *src, H5T_t *dst, H5T_cdata_t *cdata)
         priv->subset_info.copy_size = 0;
 
         /*
-         * Insure that members are sorted.
+         * Ensure that members are sorted.
          */
         H5T__sort_value(src, NULL);
         H5T__sort_value(dst, NULL);

src/H5Tdeprec.c

@@ -134,11 +134,8 @@ H5Tcommit1(hid_t loc_id, const char *name, hid_t type_id)
         HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "unable to commit datatype")
 
     /* Set up VOL object */
-    if (NULL == (new_obj = H5FL_CALLOC(H5VL_object_t)))
-        HGOTO_ERROR(H5E_VOL, H5E_NOSPACE, FAIL, "can't allocate top object structure")
-    new_obj->connector = vol_obj->connector;
-    new_obj->connector->nrefs++;
-    new_obj->data = data;
+    if (NULL == (new_obj = H5VL_create_object(data, vol_obj->connector)))
+        HGOTO_ERROR(H5E_DATATYPE, H5E_CANTALLOC, FAIL, "can't create VOL object for committed datatype")
 
     /* Set the committed type object to the VOL connector pointer in the H5T_t struct */
     dt->vol_obj = new_obj;

src/H5Tpkg.h

@@ -859,7 +859,7 @@ H5_DLL void     H5T__bit_neg(uint8_t *buf, size_t start, size_t size);
 /* VL functions */
 H5_DLL H5T_t *H5T__vlen_create(const H5T_t *base);
 H5_DLL herr_t H5T__vlen_reclaim(void *elem, const H5T_t *dt, H5T_vlen_alloc_info_t *alloc_info);
-H5_DLL htri_t H5T__vlen_set_loc(const H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc);
+H5_DLL htri_t H5T__vlen_set_loc(H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc);
 
 /* Array functions */
 H5_DLL H5T_t *H5T__array_create(H5T_t *base, unsigned ndims, const hsize_t dim[/* ndims */]);
@@ -868,7 +868,7 @@ H5_DLL int    H5T__get_array_dims(const H5T_t *dt, hsize_t dims[]);
 
 /* Reference functions */
 H5_DLL herr_t H5T__ref_reclaim(void *elem, const H5T_t *dt);
-H5_DLL htri_t H5T__ref_set_loc(const H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc);
+H5_DLL htri_t H5T__ref_set_loc(H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc);
 
 /* Compound functions */
 H5_DLL herr_t H5T__insert(H5T_t *parent, const char *name, size_t offset, const H5T_t *member);

src/H5Tprivate.h

@@ -41,11 +41,13 @@ typedef struct H5T_t H5T_t;
 #define H5T_GET_SHARED(T)           ((T)->shared)
 #define H5T_GET_MEMBER_OFFSET(T, I) ((T)->u.compnd.memb[I].offset)
 #define H5T_GET_MEMBER_SIZE(T, I)   ((T)->u.compnd.memb[I].shared->size)
+#define H5T_GET_FORCE_CONV(T)       ((T)>shared->force_conv)
 #else /* H5T_MODULE */
 #define H5T_GET_SIZE(T)             (H5T_get_size(T))
 #define H5T_GET_SHARED(T)           (H5T_get_shared(T))
 #define H5T_GET_MEMBER_OFFSET(T, I) (H5T_get_member_offset((T), (I)))
 #define H5T_GET_MEMBER_SIZE(T, I)   (H5T_get_member_size((T), (I)))
+#define H5T_GET_FORCE_CONV(T)       (H5T_get_force_conv(T))
 #endif /* H5T_MODULE */
 
 /* Forward references of package typedefs (declared in H5Tpkg.h) */
@@ -114,6 +116,7 @@ H5_DLL H5T_t *     H5T_get_super(const H5T_t *dt);
 H5_DLL H5T_class_t H5T_get_class(const H5T_t *dt, htri_t internal);
 H5_DLL htri_t      H5T_detect_class(const H5T_t *dt, H5T_class_t cls, hbool_t from_api);
 H5_DLL size_t      H5T_get_size(const H5T_t *dt);
+H5_DLL hbool_t     H5T_get_force_conv(const H5T_t *dt);
 H5_DLL int         H5T_cmp(const H5T_t *dt1, const H5T_t *dt2, hbool_t superset);
 H5_DLL herr_t      H5T_encode(H5T_t *obj, unsigned char *buf, size_t *nalloc);
 H5_DLL H5T_t *           H5T_decode(size_t buf_size, const unsigned char *buf);

src/H5Tref.c

@@ -156,7 +156,7 @@ static const H5T_ref_class_t H5T_ref_dsetreg_disk_g = {
  *-------------------------------------------------------------------------
  */
 htri_t
-H5T__ref_set_loc(const H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc)
+H5T__ref_set_loc(H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc)
 {
     htri_t ret_value = FALSE; /* Indicate success, but no location change */
 
@@ -180,6 +180,13 @@ H5T__ref_set_loc(const H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc)
             /* Mark this type as being stored in memory */
             dt->shared->u.atomic.u.r.loc = H5T_LOC_MEMORY;
 
+            /* Release owned file */
+            if (dt->shared->owned_vol_obj) {
+                if(H5VL_free_object(dt->shared->owned_vol_obj) < 0)
+                    HGOTO_ERROR(H5E_REFERENCE, H5E_CANTCLOSEOBJ, FAIL, "unable to close owned VOL object")
+                dt->shared->owned_vol_obj = NULL;
+            } /* end if */
+
             /* Reset file ID (since this reference is in memory) */
             dt->shared->u.atomic.u.r.file = file; /* file is NULL */
 
@@ -220,6 +227,10 @@ H5T__ref_set_loc(const H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc)
             /* Set file pointer (since this reference is on disk) */
             dt->shared->u.atomic.u.r.file = file;
 
+            /* dt now owns a reference to file */
+            if (H5T_own_vol_obj(dt, file) < 0)
+                HGOTO_ERROR(H5E_REFERENCE, H5E_CANTINIT, FAIL, "can't give ownership of VOL object")
+
             if (dt->shared->u.atomic.u.r.rtype == H5R_OBJECT1) {
                 H5F_t *f;
 

src/H5Tvlen.c

@@ -245,7 +245,7 @@ H5T__vlen_create(const H5T_t *base)
  *-------------------------------------------------------------------------
  */
 htri_t
-H5T__vlen_set_loc(const H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc)
+H5T__vlen_set_loc(H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc)
 {
     H5VL_file_cont_info_t cont_info = {H5VL_CONTAINER_INFO_VERSION, 0, 0, 0};
     htri_t                ret_value = FALSE; /* Indicate success, but no location change */
@@ -282,6 +282,13 @@ H5T__vlen_set_loc(const H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc)
                 else
                     HDassert(0 && "Invalid VL type");
 
+                /* Release owned file */
+                if (dt->shared->owned_vol_obj) {
+                    if(H5VL_free_object(dt->shared->owned_vol_obj) < 0)
+                        HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCLOSEOBJ, FAIL, "unable to close owned VOL object")
+                    dt->shared->owned_vol_obj = NULL;
+                } /* end if */
+
                 /* Reset file pointer (since this VL is in memory) */
                 dt->shared->u.vlen.file = NULL;
                 break;
@@ -307,6 +314,10 @@ H5T__vlen_set_loc(const H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc)
 
                 /* Set file ID (since this VL is on disk) */
                 dt->shared->u.vlen.file = file;
+
+                /* dt now owns a reference to file */
+                if (H5T_own_vol_obj(dt, file) < 0)
+                    HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "can't give ownership of VOL object")
                 break;
 
             case H5T_LOC_BADLOC:

src/H5VL.c

@@ -704,8 +704,9 @@ H5VLget_file_type(void *file_obj, hid_t connector_id, hid_t dtype_id)
     if (NULL == (dtype = (H5T_t *)H5I_object_verify(dtype_id, H5I_DATATYPE)))
         HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a data type")
 
-    /* Create VOL object for file */
-    if (NULL == (file_vol_obj = H5VL_create_object_using_vol_id(H5I_FILE, file_obj, connector_id)))
+    /* Create VOL object for file if necessary (force_conv will be TRUE if and
+     * only if file needs to be passed to H5T_set_loc) */
+    if (H5T_GET_FORCE_CONV(dtype) && (NULL == (file_vol_obj = H5VL_create_object_using_vol_id(H5I_FILE, file_obj, connector_id))))
         HGOTO_ERROR(H5E_VOL, H5E_CANTCREATE, FAIL, "can't create VOL object")
 
     /* Copy the datatype */
@@ -722,10 +723,12 @@ H5VLget_file_type(void *file_obj, hid_t connector_id, hid_t dtype_id)
     if (H5T_set_loc(file_type, file_vol_obj, H5T_LOC_DISK) < 0)
         HGOTO_ERROR(H5E_VOL, H5E_CANTINIT, FAIL, "can't set datatype location")
 
-    /* file_type now owns file_vol_obj */
-    if (H5T_own_vol_obj(file_type, file_vol_obj) < 0)
-        HGOTO_ERROR(H5E_VOL, H5E_CANTINIT, FAIL, "can't give ownership of VOL object")
-    file_vol_obj = NULL;
+    /* Release our reference to file_type */
+    if (file_vol_obj) {
+        if (H5VL_free_object(file_vol_obj) < 0)
+            HGOTO_ERROR(H5E_VOL, H5E_CANTDEC, FAIL, "unable to free VOL object")
+        file_vol_obj = NULL;
+    } /* end if */
 
     /* Set return value */
     ret_value = file_type_id;