Fix various bugs in mca_base_pvar.

Fixes:

 - Segmentation fault when using watermark variables.

 - Segmentation fault when using a handle bound to a no longer valid
   performance variable.

 - Incorrect return codes from MPI_T_pvar_* functions.

cmr=v1.7.4:reviewer=jsquyres

This commit was SVN r29481.

ompi/mpi/tool/mpit-internal.h

@@ -37,6 +37,7 @@ void mpit_unlock (void);
 extern volatile uint32_t mpit_init_count;
 
 int ompit_var_type_to_datatype (mca_base_var_type_t type, MPI_Datatype *datatype);
+int ompit_opal_to_mpit_error (int rc);
 
 static inline int mpit_is_initialized (void)
 {

ompi/mpi/tool/mpit_common.c

@@ -82,3 +82,18 @@ int ompit_var_type_to_datatype (mca_base_var_type_t type, MPI_Datatype *datatype
 
     return OMPI_SUCCESS;
 }
+
+int ompit_opal_to_mpit_error (int rc)
+{
+    switch (rc) {
+    case OPAL_SUCCESS:
+        return MPI_SUCCESS;
+    case OPAL_ERR_OUT_OF_RESOURCE:
+        return MPI_T_ERR_MEMORY;
+    case OPAL_ERR_VALUE_OUT_OF_BOUNDS:
+    case OPAL_ERR_NOT_BOUND:
+        return MPI_T_ERR_INVALID_HANDLE;
+    default:
+        return MPI_ERR_UNKNOWN;
+    }
+}

ompi/mpi/tool/pvar_handle_alloc.c

@@ -50,16 +50,9 @@ int MPI_T_pvar_handle_alloc(MPI_T_pvar_session session, int pvar_index,
 
         ret = mca_base_pvar_handle_alloc (session, pvar_index, obj_handle,
                                           handle, count);
-        if (OPAL_ERR_OUT_OF_RESOURCE == ret) {
-            ret = MPI_T_ERR_MEMORY;
-        } else if (OPAL_ERR_VALUE_OUT_OF_BOUNDS == ret) {
-            ret = MPI_T_ERR_INVALID_HANDLE;
-        } else if (OPAL_SUCCESS != ret) {
-            ret = MPI_ERR_UNKNOWN;
-        }
     } while (0);
 
     mpit_unlock ();
 
-    return ret;
+    return ompit_opal_to_mpit_error(ret);
 }

ompi/mpi/tool/pvar_read.c

@@ -40,5 +40,5 @@ int MPI_T_pvar_read(MPI_T_pvar_session session, MPI_T_pvar_handle handle,
 
     mpit_unlock ();
 
-    return ret;
+    return ompit_opal_to_mpit_error (ret);
 }

ompi/mpi/tool/pvar_reset.c

@@ -46,5 +46,5 @@ int MPI_T_pvar_reset(MPI_T_pvar_session session, MPI_T_pvar_handle handle)
 
     mpit_unlock ();
 
-    return ret;
+    return ompit_opal_to_mpit_error (ret);
 }

ompi/mpi/tool/pvar_session_create.c

@@ -31,13 +31,10 @@ int MPI_T_pvar_session_create(MPI_T_pvar_session *session)
 
     mpit_lock ();
 
-    do {
+    *session = OBJ_NEW(mca_base_pvar_session_t);
-        *session = OBJ_NEW(mca_base_pvar_session_t);
+    if (NULL == *session) {
-        if (NULL == *session) {
+        ret = MPI_ERR_NO_MEM;
-            ret = MPI_ERR_NO_MEM;
+    }
-            break;
-        }
-    } while (0);
 
     mpit_unlock ();
 

ompi/mpi/tool/pvar_start.c

@@ -55,5 +55,5 @@ int MPI_T_pvar_start(MPI_T_pvar_session session, MPI_T_pvar_handle handle)
 
     mpit_unlock ();
 
-    return ret;
+    return ompit_opal_to_mpit_error (ret);
 }

ompi/mpi/tool/pvar_stop.c

@@ -57,5 +57,5 @@ int MPI_T_pvar_stop(MPI_T_pvar_session session, MPI_T_pvar_handle handle)
 
     mpit_unlock ();
 
-    return ret;
+    return ompit_opal_to_mpit_error (ret);
 }

ompi/mpi/tool/pvar_write.c

@@ -40,5 +40,5 @@ int MPI_T_pvar_write(MPI_T_pvar_session session, MPI_T_pvar_handle handle,
 
     mpit_unlock ();
 
-    return ret;
+    return ompit_opal_to_mpit_error (ret);
 }

opal/mca/base/mca_base_pvar.c

@@ -463,26 +463,8 @@ int mca_base_pvar_handle_alloc (mca_base_pvar_session_t *session, int index, voi
             break;
         }
 
-        if (mca_base_pvar_is_sum (pvar)) {
-            /* for sums (counters, timers, etc) we need to keep track of
-               what the last value of the underlying counter was. this allows
-               us to push the computation of handle values from the event(s)
-               (which could be in a critical path) to pvar read/stop/reset/etc */
-            pvar_handle->last_value = calloc (*count, datatype_size);
-            if (NULL == pvar_handle->last_value) {
-                ret = OPAL_ERR_OUT_OF_RESOURCE;
-                break;
-            }
-        }
-
         if (!mca_base_pvar_is_continuous (pvar) || mca_base_pvar_is_sum (pvar) ||
             mca_base_pvar_is_watermark (pvar)) {
-            pvar_handle->tmp_value = calloc (*count, datatype_size);
-            if (NULL == pvar_handle->tmp_value) {
-                ret = OPAL_ERR_OUT_OF_RESOURCE;
-                break;
-            }
-       
             /* if a variable is not continuous we will need to keep track of its last value
                to support start->stop->read correctly. use calloc to initialize the current
                value to 0. */
@@ -493,20 +475,37 @@ int mca_base_pvar_handle_alloc (mca_base_pvar_session_t *session, int index, voi
             }
         }
 
-        /* get the current value of the performance variable if this is a
+        if (mca_base_pvar_is_sum (pvar) || mca_base_pvar_is_watermark (pvar)) {
-           continuous sum or watermark. if this variable needs to be started first the
+            /* for sums (counters, timers, etc) we need to keep track of
-           current value is not relevant. */
+               what the last value of the underlying counter was. this allows
-        if (mca_base_pvar_is_continuous (pvar) && (mca_base_pvar_is_sum (pvar) || mca_base_pvar_is_sum (pvar))) {
+               us to push the computation of handle values from the event(s)
-            if (mca_base_pvar_is_sum (pvar)) {
+               (which could be in a critical path) to pvar read/stop/reset/etc */
-                /* the initial value of a sum is 0 */
+            pvar_handle->tmp_value = calloc (*count, datatype_size);
-                ret = pvar->get_value (pvar, pvar_handle->last_value, pvar_handle->obj_handle);
+            if (NULL == pvar_handle->tmp_value) {
-            } else {
+                ret = OPAL_ERR_OUT_OF_RESOURCE;
-                /* the initial value of a watermark is the current value of the variable */
+                break;
-                ret = pvar->get_value (pvar, pvar_handle->current_value, pvar_handle->obj_handle);
             }
        
-            if (OPAL_SUCCESS != ret) {
+            pvar_handle->last_value = calloc (*count, datatype_size);
-                return ret;
+            if (NULL == pvar_handle->last_value) {
+                ret = OPAL_ERR_OUT_OF_RESOURCE;
+                break;
+            }
+
+            /* get the current value of the performance variable if this is a
+               continuous sum or watermark. if this variable needs to be started first the
+               current value is not relevant. */
+            if (mca_base_pvar_is_continuous (pvar)) {
+                if (mca_base_pvar_is_sum (pvar)) {
+                    ret = pvar->get_value (pvar, pvar_handle->last_value, pvar_handle->obj_handle);
+                } else {
+                    /* the initial value of a watermark is the current value of the variable */
+                    ret = pvar->get_value (pvar, pvar_handle->current_value, pvar_handle->obj_handle);
+                }
+
+                if (OPAL_SUCCESS != ret) {
+                    return ret;
+                }
             }
         }
 
@@ -533,14 +532,6 @@ int mca_base_pvar_handle_alloc (mca_base_pvar_session_t *session, int index, voi
 
 int mca_base_pvar_handle_free (mca_base_pvar_handle_t *handle)
 {
-    if (handle->session) {
-        opal_list_remove_item (&handle->session->handles, &handle->super);
-    }
-
-    if (handle->pvar) {
-        opal_list_remove_item (&handle->pvar->bound_handles, &handle->list2);
-    }
-
     OBJ_RELEASE(handle);
 
     return OPAL_SUCCESS;
@@ -551,6 +542,10 @@ int mca_base_pvar_handle_update (mca_base_pvar_handle_t *handle)
     int i, ret;
     void *tmp;
 
+    if (mca_base_pvar_is_invalid (handle->pvar)) {
+        return OPAL_ERR_NOT_BOUND;
+    }
+
     if (!mca_base_pvar_handle_is_running (handle)) {
         return OPAL_SUCCESS;
     }
@@ -657,6 +652,10 @@ int mca_base_pvar_handle_read_value (mca_base_pvar_handle_t *handle, void *value
 {
     int ret;
 
+    if (mca_base_pvar_is_invalid (handle->pvar)) {
+        return OPAL_ERR_NOT_BOUND;
+    }
+
     /* ensure this handle's value is up to date. */
     ret = mca_base_pvar_handle_update (handle);
     if (OPAL_SUCCESS != ret) {
@@ -679,6 +678,10 @@ int mca_base_pvar_handle_write_value (mca_base_pvar_handle_t *handle, const void
 {
     int ret;
 
+    if (mca_base_pvar_is_invalid (handle->pvar)) {
+        return OPAL_ERR_NOT_BOUND;
+    }
+
     if (mca_base_pvar_is_readonly (handle->pvar)) {
         return OPAL_ERR_PERM;
     }
@@ -735,6 +738,10 @@ int mca_base_pvar_handle_stop (mca_base_pvar_handle_t *handle)
 {
     int ret;
 
+    if (mca_base_pvar_is_invalid (handle->pvar)) {
+        return OPAL_ERR_NOT_BOUND;
+    }
+
     /* Can't stop a continuous or an already stopped variable */
     if (!mca_base_pvar_handle_is_running (handle) || mca_base_pvar_is_continuous (handle->pvar)) {
         return OPAL_ERR_NOT_SUPPORTED;
@@ -758,6 +765,10 @@ int mca_base_pvar_handle_reset (mca_base_pvar_handle_t *handle)
 {
     int ret = OPAL_SUCCESS;
 
+    if (mca_base_pvar_is_invalid (handle->pvar)) {
+        return OPAL_ERR_NOT_BOUND;
+    }
+
     /* reset this handle to a state analagous to when it was created */
     if (mca_base_pvar_is_sum (handle->pvar)) {
         /* reset the running sum to 0 */
@@ -908,7 +919,8 @@ static void ompi_mpi_pvar_session_destructor (mca_base_pvar_session_t *session)
     mca_base_pvar_handle_t *handle, *next;
 
     /* it is likely a user error if there are any allocated handles when the session
-       is freed. clean it up anyway. */
+     * is freed. clean it up anyway. The handle destructor will remove the handle from
+     * the session's handle list. */
     OPAL_LIST_FOREACH_SAFE(handle, next, &session->handles, mca_base_pvar_handle_t) {
         OBJ_DESTRUCT(handle);
     }
@@ -945,7 +957,15 @@ static void mca_base_pvar_handle_destructor (mca_base_pvar_handle_t *handle)
         free (handle->tmp_value);
     }
 
+    /* remove this handle from the pvar list */
+    opal_list_remove_item (&handle->pvar->bound_handles, &handle->list2);
+
     OBJ_DESTRUCT(&handle->list2);
+
+    /* remove this handle from the session */
+    if (handle->session) {
+        opal_list_remove_item (&handle->session->handles, &handle->super);
+    }
 }
 
 OBJ_CLASS_INSTANCE(mca_base_pvar_handle_t, opal_list_item_t, mca_base_pvar_handle_constructor,

opal/mca/base/mca_base_pvar.h

@@ -451,6 +451,11 @@ static inline bool mca_base_pvar_is_atomic (const mca_base_pvar_t *pvar)
     return !!(pvar->flags & MCA_BASE_PVAR_FLAG_ATOMIC);
 }
 
+static inline bool mca_base_pvar_is_invalid (const mca_base_pvar_t *pvar)
+{
+    return !!(pvar->flags & MCA_BASE_PVAR_FLAG_INVALID);
+}
+
 /* Handle functions */
 
 /**