communicator: add errhandler_type back #11818

wzamazon · 2023-07-11T14:08:32Z

Previous commit 2d68804 removed "errhandler_type" from communicator, and replaced it with
"errhandler->eh_mpi_object_type".

However, for an errhandler to be invoked on a communicator, errhandler_type must always be OMPI_ERRHANDLER_TYPE_COMM. But, errhandler->eh_mpi_object_type can be
OMPI_ERRHANDLER_TYPE_PREDEFINED for predefined error handlers such as MPI_ERRORS_ARE_FATAL.

This patch added "errhandler_type" back to communicator to address the issue.

Refs #11817

edgargabriel · 2023-07-11T14:17:32Z

ompi/debuggers/predefined_gap_test.c

@@ -71,6 +71,7 @@ int main(int argc, char **argv) {
 #else
    GAP_CHECK("error_handler", test_comm, error_handler, c_f_to_c_index, 1);
 #endif
+    GAP_CHECK("errhandler_type", test_comm, errhandler_type, error_handler, 1);


I think there is another line that needs to be added back (in fact, looking at it now I am not sure the code was correct before, but I am not completely sure I understand how the debugger interfaces work). It seems that we are always checking for the distance from the previous element of the structure.
GAP_CHECK("c_pml_comm", test_comm, c_pml_comm, errhandler_type, 1);

Thank you!

added in new revision

edgargabriel · 2023-07-11T14:17:59Z

ompi/debuggers/predefined_gap_test.c

@@ -125,7 +126,7 @@ int main(int argc, char **argv) {
    GAP_CHECK("w_keyhash", test_win, w_keyhash, w_flags, 1);
    GAP_CHECK("w_f_to_c_index", test_win, w_f_to_c_index, w_keyhash, 1);
    GAP_CHECK("error_handler", test_win, error_handler, w_f_to_c_index, 1);
-
+    GAP_CHECK("errhandler_type", test_win, errhandler_type, error_handler, 1);


Here as well:
GAP_CHECK("w_osc_module", test_win, w_osc_module, errhandler_type, 1);

edgargabriel · 2023-07-11T14:19:11Z

ompi/debuggers/predefined_gap_test.c

@@ -145,6 +146,7 @@ int main(int argc, char **argv) {
    GAP_CHECK("f_flags", test_file, f_flags, f_amode,  1);
    GAP_CHECK("f_f_to_c_index", test_file, f_f_to_c_index, f_flags, 1);
    GAP_CHECK("error_handler", test_file, error_handler, f_f_to_c_index, 1);
+    GAP_CHECK("errhandler_type", test_file, errhandler_type, error_handler, 1);


and here too
GAP_CHECK("f_io_version", test_file, f_io_version, errhandler_type, 1);

edgargabriel

looks good as far as I can tell

bosilca · 2023-07-11T15:16:53Z

Wait a little, I don't think we need to bring this back. Let me check something in the code and I'll get back to you later today.

jsquyres · 2023-07-11T15:38:00Z

Even though it's probably obvious, let me state here for the record that this also affects 5.0 as well.

jsquyres · 2023-07-12T10:22:15Z

@wzamazon You cited in #11817 that this issue stemmed from 2d68804, where we made some adjustments to the communicator struct. One of the foals of 2d68804 was:

This allows us to reduce the size of PREDEFINED_COMMUNICATOR_PAD back to 512 to maintain backwards compatibility with the ompi 4.1.x release series.

Does adding the errhandler_type back into the communicator struct jeopardize the ability to keep PREDEFINED_COMMUNICATOR_PAD at 512?

wzamazon · 2023-07-12T13:04:12Z

Does adding the errhandler_type back into the communicator struct jeopardize the ability to keep PREDEFINED_COMMUNICATOR_PAD at 512?

No. The original commit 2d68804 removed 2 fields from ompi_communicator_t to make it below 512. One is c_name which is 64 bytes, the other is errhandler_type.

After 2d68804, sizeof(ompi_communicator_t) is 448 byte.

The proposed commit increased the sizeof(ompi_communicator_t) to 456 bytes, which is still below 512.

Previous commit 2d68804 removed "errhandler_type" from communicator, and replaced it with "errhandler->eh_mpi_object_type". However, for an errhandler to be invoked on a communicator, errhandler_type must always be OMPI_ERRHANDLER_TYPE_COMM. But, errhandler->eh_mpi_object_type can be OMPI_ERRHANDLER_TYPE_PREDEFINED for predefined error handlers such as MPI_ERRORS_ARE_FATAL. This patch added "errhandler_type" back to communicator to address the issue. Signed-off-by: Wei Zhang <wzam@amazon.com>

jsquyres · 2023-07-12T14:26:04Z

Wait a little, I don't think we need to bring this back. Let me check something in the code and I'll get back to you later today.

@bosilca Thoughts?

bosilca · 2023-07-12T16:16:12Z

Yes, it took me a little longer to come up with a patch. Here is what I propose instead of this PR:

diff --git a/ompi/errhandler/errhandler.c b/ompi/errhandler/errhandler.c
index bcdd2f345c..33346f1574 100644
--- a/ompi/errhandler/errhandler.c
+++ b/ompi/errhandler/errhandler.c
@@ -282,7 +282,7 @@ ompi_errhandler_t *ompi_errhandler_create(ompi_errhandler_type_t object_type,
 
             new_errhandler->eh_mpi_object_type = object_type;
             new_errhandler->eh_lang = lang;
-            switch (object_type ) {
+            switch (object_type & OMPI_ERRHANDLER_TYPE_OBJECT) {
             case OMPI_ERRHANDLER_TYPE_COMM:
                 new_errhandler->eh_comm_fn = (MPI_Comm_errhandler_function *)func;
                 break;
@@ -388,10 +388,10 @@ int ompi_errhandler_proc_failed_internal(ompi_proc_t* ompi_proc, int status, boo
                              OMPI_NAME_PRINT(&ompi_proc->super.proc_name),
                              ompi_comm_print_cid(comm),
                              proc_rank,
-                             (OMPI_ERRHANDLER_TYPE_PREDEFINED == comm->error_handler->eh_mpi_object_type ? "P" :
-                              (OMPI_ERRHANDLER_TYPE_COMM == comm->error_handler->eh_mpi_object_type ? "C" :
-                               (OMPI_ERRHANDLER_TYPE_WIN == comm->error_handler->eh_mpi_object_type ? "W" :
-                                (OMPI_ERRHANDLER_TYPE_FILE == comm->error_handler->eh_mpi_object_type ? "F" : "U") ) ) )
+                             (ompi_errhandler_is_of_type(comm->error_handler, OMPI_ERRHANDLER_TYPE_PREDEFINED) ? "P" :
+                              (ompi_errhandler_is_of_type(comm->error_handler, OMPI_ERRHANDLER_TYPE_COMM) ? "C" :
+                               (ompi_errhandler_is_of_type(comm->error_handler, OMPI_ERRHANDLER_TYPE_WIN) ? "W" :
+                                (ompi_errhandler_is_of_type(comm->error_handler, OMPI_ERRHANDLER_TYPE_FILE) ? "F" : "U") ) ) )
                              ));
     }
 
diff --git a/ompi/errhandler/errhandler.h b/ompi/errhandler/errhandler.h
index 519a84d996..a26e5e1c89 100644
--- a/ompi/errhandler/errhandler.h
+++ b/ompi/errhandler/errhandler.h
@@ -82,11 +82,15 @@ typedef enum ompi_errhandler_lang_t ompi_errhandler_lang_t;
  * Enum used to describe what kind MPI object an error handler is used for
  */
 enum ompi_errhandler_type_t {
-    OMPI_ERRHANDLER_TYPE_PREDEFINED,
-    OMPI_ERRHANDLER_TYPE_COMM,
-    OMPI_ERRHANDLER_TYPE_WIN,
-    OMPI_ERRHANDLER_TYPE_FILE,
-    OMPI_ERRHANDLER_TYPE_INSTANCE,
+    OMPI_ERRHANDLER_TYPE_PREDEFINED = 0x0001,
+    OMPI_ERRHANDLER_TYPE_COMM       = 0x0010,
+    OMPI_ERRHANDLER_TYPE_WIN        = 0x0020,
+    OMPI_ERRHANDLER_TYPE_FILE       = 0x0040,
+    OMPI_ERRHANDLER_TYPE_INSTANCE   = 0x0080,
+    OMPI_ERRHANDLER_TYPE_OBJECT     = OMPI_ERRHANDLER_TYPE_COMM |
+                                      OMPI_ERRHANDLER_TYPE_WIN |
+                                      OMPI_ERRHANDLER_TYPE_FILE |
+                                      OMPI_ERRHANDLER_TYPE_INSTANCE
 };
 typedef enum ompi_errhandler_type_t ompi_errhandler_type_t;
 
@@ -442,10 +446,22 @@ OMPI_DECLSPEC void ompi_errhandler_registration_callback(int status,
  */
 static inline bool ompi_errhandler_is_intrinsic(ompi_errhandler_t *errhandler)
 {
-    if ( OMPI_ERRHANDLER_TYPE_PREDEFINED == errhandler->eh_mpi_object_type )
-	return true;
+    return (OMPI_ERRHANDLER_TYPE_PREDEFINED & errhandler->eh_mpi_object_type);
+}
 
-    return false;
+/**
+ * Check to see if the errorhandler is of the specified type
+ *
+ * @param errhandle The errhandler to check
+ * @param type      The type to be checked upon
+ *
+ * @return true  If the errhandler is of the specified type
+ * @return false otherwise
+ */
+static inline bool ompi_errhandler_is_of_type(ompi_errhandler_t *errhandler,
+                                              ompi_errhandler_type_t type)
+{
+    return (type & errhandler->eh_mpi_object_type);
 }
 
 #if OPAL_ENABLE_FT_MPI
diff --git a/ompi/errhandler/errhandler_invoke.c b/ompi/errhandler/errhandler_invoke.c
index fc9a0818e1..42ba2abaea 100644
--- a/ompi/errhandler/errhandler_invoke.c
+++ b/ompi/errhandler/errhandler_invoke.c
@@ -79,7 +79,7 @@ int ompi_errhandler_invoke(ompi_errhandler_t *errhandler, void *mpi_object,
     /* Figure out what kind of errhandler it is, figure out if it's
        fortran or C, and then invoke it */
 
-    switch (object_type) {
+    switch (object_type & OMPI_ERRHANDLER_TYPE_OBJECT) {
     case OMPI_ERRHANDLER_TYPE_COMM:
         comm = (ompi_communicator_t *) mpi_object;
         switch (errhandler->eh_lang) {
diff --git a/ompi/mpi/c/comm_set_errhandler.c b/ompi/mpi/c/comm_set_errhandler.c
index 0be31b7640..1c865845e5 100644
--- a/ompi/mpi/c/comm_set_errhandler.c
+++ b/ompi/mpi/c/comm_set_errhandler.c
@@ -57,10 +57,9 @@ int MPI_Comm_set_errhandler(MPI_Comm comm, MPI_Errhandler errhandler)
                                           FUNC_NAME);
         } else if (NULL == errhandler ||
                    MPI_ERRHANDLER_NULL == errhandler ||
-                   ( OMPI_ERRHANDLER_TYPE_COMM != errhandler->eh_mpi_object_type &&
-                     OMPI_ERRHANDLER_TYPE_PREDEFINED != errhandler->eh_mpi_object_type) ) {
-            return OMPI_ERRHANDLER_INVOKE(comm, MPI_ERR_ARG,
-                                          FUNC_NAME);
+                   (!ompi_errhandler_is_of_type(errhandler, OMPI_ERRHANDLER_TYPE_COMM) &&
+                    !ompi_errhandler_is_intrinsic(errhandler)) ) {
+            return OMPI_ERRHANDLER_INVOKE(comm, MPI_ERR_ARG, FUNC_NAME);
         }
     }
 
diff --git a/ompi/mpi/c/file_set_errhandler.c b/ompi/mpi/c/file_set_errhandler.c
index 7590ab6c85..9dd95f8c7a 100644
--- a/ompi/mpi/c/file_set_errhandler.c
+++ b/ompi/mpi/c/file_set_errhandler.c
@@ -57,8 +57,8 @@ int MPI_File_set_errhandler( MPI_File file, MPI_Errhandler errhandler)
                                           FUNC_NAME);
         } else if (NULL == errhandler ||
                    MPI_ERRHANDLER_NULL == errhandler ||
-                   (OMPI_ERRHANDLER_TYPE_FILE != errhandler->eh_mpi_object_type &&
-		    OMPI_ERRHANDLER_TYPE_PREDEFINED != errhandler->eh_mpi_object_type) ) {
+                   (!ompi_errhandler_is_of_type(errhandler, OMPI_ERRHANDLER_TYPE_FILE)  &&
+                    !ompi_errhandler_is_intrinsic(errhandler)) ) {
             return OMPI_ERRHANDLER_INVOKE(file, MPI_ERR_ARG, FUNC_NAME);
         }
     }
diff --git a/ompi/mpi/c/session_set_errhandler.c b/ompi/mpi/c/session_set_errhandler.c
index bb6c12af62..8b9da3adb8 100644
--- a/ompi/mpi/c/session_set_errhandler.c
+++ b/ompi/mpi/c/session_set_errhandler.c
@@ -52,7 +52,7 @@ int MPI_Session_set_errhandler(MPI_Session session, MPI_Errhandler errhandler)
         } else if (NULL == errhandler ||
                    MPI_ERRHANDLER_NULL == errhandler ||
                    ( OMPI_ERRHANDLER_TYPE_INSTANCE != errhandler->eh_mpi_object_type &&
-                     OMPI_ERRHANDLER_TYPE_PREDEFINED != errhandler->eh_mpi_object_type) ) {
+                     !ompi_errhandler_is_intrinsic(errhandler)) ) {
             return OMPI_ERRHANDLER_INVOKE(session, MPI_ERR_ARG,
                                           FUNC_NAME);
         }
diff --git a/ompi/mpi/c/win_set_errhandler.c b/ompi/mpi/c/win_set_errhandler.c
index 820ba51b33..ffd0884ed7 100644
--- a/ompi/mpi/c/win_set_errhandler.c
+++ b/ompi/mpi/c/win_set_errhandler.c
@@ -52,8 +52,8 @@ int MPI_Win_set_errhandler(MPI_Win win, MPI_Errhandler errhandler)
                                           FUNC_NAME);
         } else if (NULL == errhandler ||
                    MPI_ERRHANDLER_NULL == errhandler ||
-                   (OMPI_ERRHANDLER_TYPE_WIN != errhandler->eh_mpi_object_type &&
-                    OMPI_ERRHANDLER_TYPE_PREDEFINED != errhandler->eh_mpi_object_type) ) {
+                   (!ompi_errhandler_is_of_type(errhandler, OMPI_ERRHANDLER_TYPE_WIN) &&
+                    !ompi_errhandler_is_intrinsic(errhandler)) ) {
             return OMPI_ERRHANDLER_INVOKE(win, MPI_ERR_ARG, FUNC_NAME);
         }
     }

wzamazon · 2023-07-12T17:16:52Z

@bosilca

Can you explain how your patch solved the problem?

The issue is that OMPI_ERRHANDLER_INVOKE did not pass the correct errhandler_type to ompi_errhandler_invoke.

I do not understand how your patch solved the issue without modifying OMPI_ERRHANDLER_INVOKE

bosilca · 2023-07-12T17:52:18Z

predefined error handlers are not tagged with a type, and can be called on any objects (communicator, windows and files). We just need to allow a distinction between these. My patch does modify ompi_errhandler_invoke by making sure we are testing with the correct part of the eh_mpi_object_type.

Do you have a test that still fails ?

wzamazon · 2023-07-12T17:56:36Z

Do you have a test that still fails ?

Let me test

wzamazon · 2023-07-12T18:20:54Z

@bosilca

I applied your patch and tested using ompi-tests/intel_tests/MPI_Errhandler_fatal_c, and the test still fails.

From what I can see.

Whe predefined error handler was used, the object_type passed from OMPI_ERRHANDLER_INVOKE to ompi_errhander_invoke is 1, which is OMPI_ERRHANDLER_TYPE_PREDEFINED.

In function ompi_errhandler_invoke, there is a switch case loop like the following

switch(object_type & OMPI_ERRHANDLER_TYPE_OBJECT) {
case OMPI_ERRHANDLER_TYPE_COMM:
  invoke comm_fn;
  break;
case OMPI_ERRHANDLER_TYPE_WIN:
  invoke win_fn;
  break;
case OMPI_ERRHANDLER_TYPE_FILE:
  invoke win_fn;
  break;
}

For the code to work, the result of object_type & OMPI_ERRHANDLER_TYPE_OBJECT must be OMPI_ERRHANDLER_TYPE_COMM (which is 0x10) when object is a communicator.

But because object_type is 1, the value of object_type & OMPI_ERRHANDLER_TYPE_OBJECT is 0, so comm_fn was not called.

wzamazon · 2023-07-12T18:24:34Z

On a higher level, because the predefined error handler can be applied to 3 types of object: communicator, win, file. There is no way we can use a variable inside the err handler to determine the MPI object type. Unless, we make a clone of the predefined errhandler each time it is used in MPI_Errhandler_set, and set the errhandler_type of the cloned error handler.

bosilca · 2023-07-12T19:12:40Z

ok, I misunderstood this whole issue here. You are correct we need to have the type of the error handler associated with each object, to know how to manipulate the error handler when necessary.

bosilca · 2023-07-12T19:30:23Z

To conclude this we are using a field in the MPI object to avoid having to specialize the call to the OMPI_ERRHANDLER_INVOKE macro. Laziness at its best.

wzamazon · 2023-07-13T00:56:37Z

@bosilca

Thank you for the summarization!

@jsquyres Can you help with merging?

gpaulsen · 2023-07-25T15:10:53Z

We need this in v5.0.x

wenduwan · 2023-08-01T15:12:24Z

@gpaulsen @edgargabriel Backported in 1f98cd3

wzamazon requested a review from edgargabriel July 11, 2023 14:08

github-actions bot added the Target: main label Jul 11, 2023

wzamazon mentioned this pull request Jul 11, 2023

Predefined error handler such as MPI_ERRORS_ARE_FATAL does not work with main branch #11817

Closed

edgargabriel requested changes Jul 11, 2023

View reviewed changes

wzamazon force-pushed the communicator_add_errhandler_type branch from c6b8751 to 1022d0e Compare July 11, 2023 14:49

wzamazon requested a review from edgargabriel July 11, 2023 14:49

edgargabriel approved these changes Jul 11, 2023

View reviewed changes

jsquyres added the Target: v5.0.x label Jul 11, 2023

jsquyres mentioned this pull request Jul 11, 2023

communicator: add errhandler_type back and debugger GAP issue #11819

Closed

github-actions bot removed the Target: v5.0.x label Jul 12, 2023

wzamazon force-pushed the communicator_add_errhandler_type branch from 1022d0e to 216c221 Compare July 12, 2023 13:04

bosilca merged commit a8fbb4d into open-mpi:main Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

communicator: add errhandler_type back #11818

communicator: add errhandler_type back #11818

wzamazon commented Jul 11, 2023 •

edited by jsquyres

Loading

edgargabriel Jul 11, 2023

wzamazon Jul 11, 2023

edgargabriel Jul 11, 2023

wzamazon Jul 11, 2023

edgargabriel Jul 11, 2023

wzamazon Jul 11, 2023

edgargabriel left a comment

bosilca commented Jul 11, 2023

jsquyres commented Jul 11, 2023

jsquyres commented Jul 12, 2023

wzamazon commented Jul 12, 2023

jsquyres commented Jul 12, 2023

bosilca commented Jul 12, 2023

wzamazon commented Jul 12, 2023 •

edited

Loading

bosilca commented Jul 12, 2023

wzamazon commented Jul 12, 2023

wzamazon commented Jul 12, 2023 •

edited

Loading

wzamazon commented Jul 12, 2023

bosilca commented Jul 12, 2023

bosilca commented Jul 12, 2023

wzamazon commented Jul 13, 2023

gpaulsen commented Jul 25, 2023

wenduwan commented Aug 1, 2023

communicator: add errhandler_type back #11818

communicator: add errhandler_type back #11818

Conversation

wzamazon commented Jul 11, 2023 • edited by jsquyres Loading

edgargabriel Jul 11, 2023

Choose a reason for hiding this comment

wzamazon Jul 11, 2023

Choose a reason for hiding this comment

edgargabriel Jul 11, 2023

Choose a reason for hiding this comment

wzamazon Jul 11, 2023

Choose a reason for hiding this comment

edgargabriel Jul 11, 2023

Choose a reason for hiding this comment

wzamazon Jul 11, 2023

Choose a reason for hiding this comment

edgargabriel left a comment

Choose a reason for hiding this comment

bosilca commented Jul 11, 2023

jsquyres commented Jul 11, 2023

jsquyres commented Jul 12, 2023

wzamazon commented Jul 12, 2023

jsquyres commented Jul 12, 2023

bosilca commented Jul 12, 2023

wzamazon commented Jul 12, 2023 • edited Loading

bosilca commented Jul 12, 2023

wzamazon commented Jul 12, 2023

wzamazon commented Jul 12, 2023 • edited Loading

wzamazon commented Jul 12, 2023

bosilca commented Jul 12, 2023

bosilca commented Jul 12, 2023

wzamazon commented Jul 13, 2023

gpaulsen commented Jul 25, 2023

wenduwan commented Aug 1, 2023

wzamazon commented Jul 11, 2023 •

edited by jsquyres

Loading

wzamazon commented Jul 12, 2023 •

edited

Loading

wzamazon commented Jul 12, 2023 •

edited

Loading