From 0d608f72c54c342451da49dac415a23b2426b550 Mon Sep 17 00:00:00 2001 From: Joseph Schuchart Date: Sun, 1 May 2022 13:04:25 -0400 Subject: [PATCH 1/2] Attributes: replace custom void* union with C union The current implementation uses a void* to store different types of attribute value integers and attempts to figure out proper offsets for storing smaller integers in that pointer. The required pointer aliasing is UB and causes issues with GCC 11. The new implementation replaces the self-built pointer-based union with a C union and selects the (pointer to the) right field based on the av_set_from value. This patch also fixes a bug where copied attributes always had the set_from field set to C pointer, which worked but is technically not correct. Signed-off-by: Joseph Schuchart (cherry picked from commit fdc710843b9cf32b72b1b260fe2c11484393b742) --- ompi/attribute/attribute.c | 163 +++++++++++++++++-------------------- 1 file changed, 74 insertions(+), 89 deletions(-) diff --git a/ompi/attribute/attribute.c b/ompi/attribute/attribute.c index cfe2ac387e5..e0e428ac7b1 100644 --- a/ompi/attribute/attribute.c +++ b/ompi/attribute/attribute.c @@ -353,8 +353,8 @@ do { \ if (MPI_SUCCESS != OMPI_FINT_2_INT(f_err)) { \ err = OMPI_FINT_2_INT(f_err); \ } else { \ - out_attr->av_value = (void*) 0; \ - *out_attr->av_fint_pointer = out; \ + out_attr->av_value.av_fint = out; \ + out_attr->av_set_from = OMPI_ATTRIBUTE_FINT; \ flag = OMPI_LOGICAL_2_INT(f_flag); \ } \ } \ @@ -371,7 +371,8 @@ do { \ if (MPI_SUCCESS != OMPI_FINT_2_INT(f_err)) { \ err = OMPI_FINT_2_INT(f_err); \ } else { \ - out_attr->av_value = (void *) out; \ + out_attr->av_value.av_aint = out; \ + out_attr->av_set_from = OMPI_ATTRIBUTE_AINT; \ flag = OMPI_LOGICAL_2_INT(f_flag); \ } \ } \ @@ -383,8 +384,9 @@ do { \ if ((err = (*((keyval_obj->copy_attr_fn).attr_##type##_copy_fn)) \ ((ompi_##type##_t *)old_object, key, keyval_obj->extra_state.c_ptr, \ in, &out, &flag)) == MPI_SUCCESS) { \ - out_attr->av_value = out; \ - } \ + out_attr->av_value.av_pointer = out; \ + out_attr->av_set_from = OMPI_ATTRIBUTE_C; \ + } \ } \ OPAL_THREAD_LOCK(&attribute_lock); \ } while (0) @@ -393,25 +395,30 @@ do { \ * Cases for attribute values */ typedef enum ompi_attribute_translate_t { + OMPI_ATTRIBUTE_INVALID = -1, OMPI_ATTRIBUTE_C, OMPI_ATTRIBUTE_INT, OMPI_ATTRIBUTE_FINT, - OMPI_ATTRIBUTE_AINT + OMPI_ATTRIBUTE_AINT, } ompi_attribute_translate_t; +typedef union attribute_value_t { + MPI_Fint av_fint; + MPI_Aint av_aint; + int av_int; + void *av_pointer; +} attribute_value_t; + /* * struct to hold attribute values on each MPI object */ -typedef struct attribute_value_t { +typedef struct attribute_key_value_t { opal_object_t super; int av_key; - void *av_value; - int *av_int_pointer; - MPI_Fint *av_fint_pointer; - MPI_Aint *av_aint_pointer; - int av_set_from; + attribute_value_t av_value; + ompi_attribute_translate_t av_set_from; int av_sequence; -} attribute_value_t; +} attribute_key_value_t; /* * struct to hold state of attr subsys @@ -426,20 +433,20 @@ typedef struct attr_subsys_t { /* * Local functions */ -static void attribute_value_construct(attribute_value_t *item); +static void attribute_key_value_construct(attribute_key_value_t *item); static void ompi_attribute_keyval_construct(ompi_attribute_keyval_t *keyval); static void ompi_attribute_keyval_destruct(ompi_attribute_keyval_t *keyval); static void attr_subsys_construct(attr_subsys_t *subsys); static void attr_subsys_destruct(attr_subsys_t *subsys); static int set_value(ompi_attribute_type_t type, void *object, opal_hash_table_t **attr_hash, int key, - attribute_value_t *new_attr, + attribute_key_value_t *new_attr, bool predefined); static int get_value(opal_hash_table_t *attr_hash, int key, - attribute_value_t **attribute, int *flag); -static void *translate_to_c(attribute_value_t *val); -static MPI_Fint translate_to_fint(attribute_value_t *val); -static MPI_Aint translate_to_aint(attribute_value_t *val); + attribute_key_value_t **attribute, int *flag); +static void *translate_to_c(attribute_key_value_t *val); +static MPI_Fint translate_to_fint(attribute_key_value_t *val); +static MPI_Aint translate_to_aint(attribute_key_value_t *val); static int compare_attr_sequence(const void *attr1, const void *attr2); @@ -452,11 +459,11 @@ static OBJ_CLASS_INSTANCE(attr_subsys_t, attr_subsys_destruct); /* - * attribute_value_t class + * attribute_key_value_t class */ -static OBJ_CLASS_INSTANCE(attribute_value_t, +static OBJ_CLASS_INSTANCE(attribute_key_value_t, opal_object_t, - attribute_value_construct, + attribute_key_value_construct, NULL); @@ -473,8 +480,6 @@ static OBJ_CLASS_INSTANCE(ompi_attribute_keyval_t, */ static attr_subsys_t *attr_subsys = NULL; -static unsigned int int_pos = 12345; -static unsigned int integer_pos = 12345; static int attr_sequence; /* @@ -482,18 +487,16 @@ static int attr_sequence; * approach. However, this lock is released before a user provided callback is * triggered and acquired right after, allowing for recursive behaviors. */ -static opal_mutex_t attribute_lock = OPAL_MUTEX_STATIC_INIT; +static opal_mutex_t attribute_lock = OPAL_MUTEX_STATIC_INIT; /* - * attribute_value_t constructor function + * attribute_key_value_t constructor function */ -static void attribute_value_construct(attribute_value_t *item) +static void attribute_key_value_construct(attribute_key_value_t *item) { item->av_key = MPI_KEYVAL_INVALID; - item->av_aint_pointer = (MPI_Aint*) &item->av_value; - item->av_int_pointer = (int *)&item->av_value + int_pos; - item->av_fint_pointer = (MPI_Fint *)&item->av_value + integer_pos; - item->av_set_from = 0; + item->av_value.av_aint = 0; + item->av_set_from = OMPI_ATTRIBUTE_INVALID; item->av_sequence = -1; } @@ -574,8 +577,6 @@ int ompi_attr_put_ref(void) static void attr_subsys_construct(attr_subsys_t *subsys) { int ret; - void *bogus = (void*) 1; - int *p = (int *) &bogus; subsys->keyval_hash = OBJ_NEW(opal_hash_table_t); @@ -584,7 +585,7 @@ static void attr_subsys_construct(attr_subsys_t *subsys) /* * Set the max size to OMPI_FORTRAN_HANDLE_MAX to enforce bound */ - opal_bitmap_set_max_size (subsys->key_bitmap, + opal_bitmap_set_max_size (subsys->key_bitmap, OMPI_FORTRAN_HANDLE_MAX); ret = opal_bitmap_init(subsys->key_bitmap, 32); if (OPAL_SUCCESS != ret) { @@ -595,20 +596,6 @@ static void attr_subsys_construct(attr_subsys_t *subsys) opal_bitmap_set_bit(subsys->key_bitmap, i); } - for (int_pos = 0; int_pos < (sizeof(void*) / sizeof(int)); - ++int_pos) { - if (p[int_pos] == 1) { - break; - } - } - - for (integer_pos = 0; integer_pos < (sizeof(void*) / sizeof(MPI_Fint)); - ++integer_pos) { - if (p[integer_pos] == 1) { - break; - } - } - ret = opal_hash_table_init(subsys->keyval_hash, ATTR_TABLE_SIZE); if (OPAL_SUCCESS != ret) { abort(); @@ -664,7 +651,7 @@ static int ompi_attr_create_keyval_impl(ompi_attribute_type_t type, if (!(flags & OMPI_KEYVAL_PREDEFINED)) { ret = CREATE_KEY(key); } - + if (OMPI_SUCCESS == ret) { keyval->key = *key; ret = opal_hash_table_set_value_uint32(attr_subsys->keyval_hash, @@ -803,14 +790,14 @@ int ompi_attr_set_c(ompi_attribute_type_t type, void *object, int key, void *attribute, bool predefined) { int ret = MPI_SUCCESS; - attribute_value_t *new_attr = OBJ_NEW(attribute_value_t); + attribute_key_value_t *new_attr = OBJ_NEW(attribute_key_value_t); if (NULL == new_attr) { return OMPI_ERR_OUT_OF_RESOURCE; } OPAL_THREAD_LOCK(&attribute_lock); - new_attr->av_value = attribute; + new_attr->av_value.av_pointer = attribute; new_attr->av_set_from = OMPI_ATTRIBUTE_C; ret = set_value(type, object, attr_hash, key, new_attr, predefined); if (OMPI_SUCCESS != ret) { @@ -833,15 +820,14 @@ int ompi_attr_set_int(ompi_attribute_type_t type, void *object, int key, int attribute, bool predefined) { int ret; - attribute_value_t *new_attr = OBJ_NEW(attribute_value_t); + attribute_key_value_t *new_attr = OBJ_NEW(attribute_key_value_t); if (NULL == new_attr) { return OMPI_ERR_OUT_OF_RESOURCE; } OPAL_THREAD_LOCK(&attribute_lock); - new_attr->av_value = (void *) 0; - *new_attr->av_int_pointer = attribute; + new_attr->av_value.av_int = attribute; new_attr->av_set_from = OMPI_ATTRIBUTE_INT; ret = set_value(type, object, attr_hash, key, new_attr, predefined); if (OMPI_SUCCESS != ret) { @@ -865,15 +851,14 @@ int ompi_attr_set_fint(ompi_attribute_type_t type, void *object, bool predefined) { int ret; - attribute_value_t *new_attr = OBJ_NEW(attribute_value_t); + attribute_key_value_t *new_attr = OBJ_NEW(attribute_key_value_t); if (NULL == new_attr) { return OMPI_ERR_OUT_OF_RESOURCE; } OPAL_THREAD_LOCK(&attribute_lock); - new_attr->av_value = (void *) 0; - *new_attr->av_fint_pointer = attribute; + new_attr->av_value.av_fint = attribute; new_attr->av_set_from = OMPI_ATTRIBUTE_FINT; ret = set_value(type, object, attr_hash, key, new_attr, predefined); if (OMPI_SUCCESS != ret) { @@ -897,14 +882,14 @@ int ompi_attr_set_aint(ompi_attribute_type_t type, void *object, bool predefined) { int ret; - attribute_value_t *new_attr = OBJ_NEW(attribute_value_t); + attribute_key_value_t *new_attr = OBJ_NEW(attribute_key_value_t); if (NULL == new_attr) { return OMPI_ERR_OUT_OF_RESOURCE; } OPAL_THREAD_LOCK(&attribute_lock); - new_attr->av_value = (void *) attribute; + new_attr->av_value.av_aint = attribute; new_attr->av_set_from = OMPI_ATTRIBUTE_AINT; ret = set_value(type, object, attr_hash, key, new_attr, predefined); if (OMPI_SUCCESS != ret) { @@ -926,7 +911,7 @@ int ompi_attr_set_aint(ompi_attribute_type_t type, void *object, int ompi_attr_get_c(opal_hash_table_t *attr_hash, int key, void **attribute, int *flag) { - attribute_value_t *val = NULL; + attribute_key_value_t *val = NULL; int ret; OPAL_THREAD_LOCK(&attribute_lock); @@ -949,7 +934,7 @@ int ompi_attr_get_c(opal_hash_table_t *attr_hash, int key, int ompi_attr_get_fint(opal_hash_table_t *attr_hash, int key, MPI_Fint *attribute, int *flag) { - attribute_value_t *val = NULL; + attribute_key_value_t *val = NULL; int ret; OPAL_THREAD_LOCK(&attribute_lock); @@ -972,7 +957,7 @@ int ompi_attr_get_fint(opal_hash_table_t *attr_hash, int key, int ompi_attr_get_aint(opal_hash_table_t *attr_hash, int key, MPI_Aint *attribute, int *flag) { - attribute_value_t *val = NULL; + attribute_key_value_t *val = NULL; int ret; OPAL_THREAD_LOCK(&attribute_lock); @@ -1003,7 +988,7 @@ int ompi_attr_copy_all(ompi_attribute_type_t type, void *old_object, uint32_t key; int flag; void *node, *in_node; - attribute_value_t *old_attr, *new_attr; + attribute_key_value_t *old_attr, *new_attr; ompi_attribute_keyval_t *hash_value; /* If there's nothing to do, just return */ @@ -1033,7 +1018,7 @@ int ompi_attr_copy_all(ompi_attribute_type_t type, void *old_object, } err = 0; - new_attr = OBJ_NEW(attribute_value_t); + new_attr = OBJ_NEW(attribute_key_value_t); switch (type) { case COMM_ATTR: /* Now call the copy_attr_fn */ @@ -1116,7 +1101,7 @@ static int ompi_attr_delete_impl(ompi_attribute_type_t type, void *object, { ompi_attribute_keyval_t *keyval; int ret = OMPI_SUCCESS; - attribute_value_t *attr; + attribute_key_value_t *attr; /* Check if the key is valid in the master keyval hash */ ret = opal_hash_table_get_value_uint32(attr_subsys->keyval_hash, key, @@ -1206,7 +1191,7 @@ int ompi_attr_delete_all(ompi_attribute_type_t type, void *object, int ret, i, num_attrs; uint32_t key; void *node, *in_node, *attr; - attribute_value_t **attrs; + attribute_key_value_t **attrs; /* Ensure that the table is not empty */ @@ -1223,7 +1208,7 @@ int ompi_attr_delete_all(ompi_attribute_type_t type, void *object, return MPI_SUCCESS; } - attrs = malloc(sizeof(attribute_value_t *) * num_attrs); + attrs = malloc(sizeof(attribute_key_value_t *) * num_attrs); if (NULL == attrs) { OPAL_THREAD_UNLOCK(&attribute_lock); return OMPI_ERR_OUT_OF_RESOURCE; @@ -1238,7 +1223,7 @@ int ompi_attr_delete_all(ompi_attribute_type_t type, void *object, } /* Sort attributes in the order that they were set */ - qsort(attrs, num_attrs, sizeof(attribute_value_t *), compare_attr_sequence); + qsort(attrs, num_attrs, sizeof(attribute_key_value_t *), compare_attr_sequence); /* Delete attributes in the reverse order that they were set. Actually this ordering is required only for MPI_COMM_SELF, as @@ -1269,12 +1254,12 @@ int ompi_attr_delete_all(ompi_attribute_type_t type, void *object, */ static int set_value(ompi_attribute_type_t type, void *object, opal_hash_table_t **attr_hash, int key, - attribute_value_t *new_attr, + attribute_key_value_t *new_attr, bool predefined) { ompi_attribute_keyval_t *keyval; int ret; - attribute_value_t *old_attr; + attribute_key_value_t *old_attr; bool had_old = false; /* Note that this function can be invoked by ompi_attr_copy_all() @@ -1358,7 +1343,7 @@ static int set_value(ompi_attribute_type_t type, void *object, * Assumes that you do NOT already have the attribute lock. */ static int get_value(opal_hash_table_t *attr_hash, int key, - attribute_value_t **attribute, int *flag) + attribute_key_value_t **attribute, int *flag) { int ret; void *attr; @@ -1384,7 +1369,7 @@ static int get_value(opal_hash_table_t *attr_hash, int key, ret = opal_hash_table_get_value_uint32(attr_hash, key, &attr); if (OMPI_SUCCESS == ret) { - *attribute = (attribute_value_t*)attr; + *attribute = (attribute_key_value_t*)attr; *flag = 1; } @@ -1400,25 +1385,25 @@ static int get_value(opal_hash_table_t *attr_hash, int key, * This function does not fail -- it is only invoked in "safe" * situations. */ -static void *translate_to_c(attribute_value_t *val) +static void *translate_to_c(attribute_key_value_t *val) { switch (val->av_set_from) { case OMPI_ATTRIBUTE_C: /* Case 1: wrote a C pointer, read a C pointer (unity) */ - return val->av_value; + return val->av_value.av_pointer; case OMPI_ATTRIBUTE_INT: /* Case 4: wrote an int, read a C pointer */ - return (void *) val->av_int_pointer; + return &val->av_value.av_int; case OMPI_ATTRIBUTE_FINT: /* Case 7: wrote a MPI_Fint, read a C pointer */ - return (void *) val->av_fint_pointer; + return &val->av_value.av_fint; case OMPI_ATTRIBUTE_AINT: /* Case 10: wrote a MPI_Aint, read a C pointer */ - return (void *) val->av_aint_pointer; + return &val->av_value.av_aint; default: /* Should never reach here */ @@ -1434,25 +1419,25 @@ static void *translate_to_c(attribute_value_t *val) * This function does not fail -- it is only invoked in "safe" * situations. */ -static MPI_Fint translate_to_fint(attribute_value_t *val) +static MPI_Fint translate_to_fint(attribute_key_value_t *val) { switch (val->av_set_from) { case OMPI_ATTRIBUTE_C: /* Case 2: wrote a C pointer, read a MPI_Fint */ - return (MPI_Fint)*val->av_int_pointer; + return (MPI_Fint)(intptr_t)val->av_value.av_pointer; case OMPI_ATTRIBUTE_INT: /* Case 5: wrote an int, read a MPI_Fint */ - return (MPI_Fint)*val->av_int_pointer; + return (MPI_Fint)val->av_value.av_int; case OMPI_ATTRIBUTE_FINT: /* Case 8: wrote a MPI_Fint, read a MPI_Fint (unity) */ - return *val->av_fint_pointer; + return val->av_value.av_fint; case OMPI_ATTRIBUTE_AINT: /* Case 11: wrote a MPI_Aint, read a MPI_Fint */ - return (MPI_Fint)*val->av_fint_pointer; + return (MPI_Fint)val->av_value.av_aint; default: /* Should never reach here */ @@ -1468,25 +1453,25 @@ static MPI_Fint translate_to_fint(attribute_value_t *val) * This function does not fail -- it is only invoked in "safe" * situations. */ -static MPI_Aint translate_to_aint(attribute_value_t *val) +static MPI_Aint translate_to_aint(attribute_key_value_t *val) { switch (val->av_set_from) { case OMPI_ATTRIBUTE_C: /* Case 3: wrote a C pointer, read a MPI_Aint */ - return (MPI_Aint) val->av_value; + return (MPI_Aint) val->av_value.av_pointer; case OMPI_ATTRIBUTE_INT: /* Case 6: wrote an int, read a MPI_Aint */ - return (MPI_Aint) *val->av_int_pointer; + return (MPI_Aint) val->av_value.av_int; case OMPI_ATTRIBUTE_FINT: /* Case 9: wrote a MPI_Fint, read a MPI_Aint */ - return (MPI_Aint) *val->av_fint_pointer; + return (MPI_Aint) val->av_value.av_fint; case OMPI_ATTRIBUTE_AINT: /* Case 12: wrote a MPI_Aint, read a MPI_Aint (unity) */ - return (MPI_Aint) val->av_value; + return val->av_value.av_aint; default: /* Should never reach here */ @@ -1499,6 +1484,6 @@ static MPI_Aint translate_to_aint(attribute_value_t *val) */ static int compare_attr_sequence(const void *attr1, const void *attr2) { - return (*(attribute_value_t **)attr1)->av_sequence - - (*(attribute_value_t **)attr2)->av_sequence; + return (*(attribute_key_value_t **)attr1)->av_sequence - + (*(attribute_key_value_t **)attr2)->av_sequence; } From 17e92a832d01f25ff2dc77d44c6965fab8d907e6 Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Mon, 2 May 2022 11:37:57 -0400 Subject: [PATCH 2/2] ompi/attribute/attribute.c: update giant comment Correct a few minor mistakes in the large comment at the top of ompi/attribute/attribute.c from when 72cfbb665c355 added several new cases to attribute handling. Signed-off-by: Jeff Squyres (cherry picked from commit cce89e83a20d3bdc25e41ec77b68023b353adc3b) --- ompi/attribute/attribute.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ompi/attribute/attribute.c b/ompi/attribute/attribute.c index e0e428ac7b1..ccf4a23d150 100644 --- a/ompi/attribute/attribute.c +++ b/ompi/attribute/attribute.c @@ -129,15 +129,15 @@ * ompi_set_attr_int(..., foo, ...) * * 4. C reads the attribute value. The value returned is a pointer - * that points to an int that has a value - * of 7. + * that points to an int that has a value of 7. * * Example: int *ret; * MPI_Attr_get(..., &ret); * -> *ret will equal 7. * - * 5. Fortran MPI-1 reads the attribute value. This is the unity - * case; the same value is returned. + * 5. Fortran MPI-1 reads the attribute value. The C int value is + * cast to a fortran INTEGER (i.e., MPI_Fint) -- potentially being + * truncated if sizeof(int) > sizeof(INTEGER). * * Example: INTEGER ret * CALL MPI_ATTR_GET(..., ret, ierr) @@ -163,7 +163,7 @@ * that points to an INTEGER (i.e., an MPI_Fint) that has a value * of 7. * --> NOTE: The external MPI interface does not distinguish between - * this case and case 7. It is the programer's responsibility + * this case and case 10. It is the programer's responsibility * to code accordingly. * * Example: MPI_Fint *ret; @@ -202,7 +202,7 @@ * that points to an INTEGER(KIND=MPI_ADDRESS_KIND) (i.e., a void*) * that has a value of 12. * --> NOTE: The external MPI interface does not distinguish between - * this case and case 4. It is the programer's responsibility + * this case and case 7. It is the programer's responsibility * to code accordingly. * * Example A: MPI_Aint *ret;