From 0ebe687ed88f0e4331b5d184ce0174dec9965ba6 Mon Sep 17 00:00:00 2001 From: Jeff Squyres Date: Tue, 17 Oct 2006 00:18:35 +0000 Subject: [PATCH] Refs trac:502, #503. This commit as a result of review of r12122. * Update comments in some MPI_FILE_* functions to reflect that the MPI specs have different page numbers in the ps and pdf (woof!). * Update comments to say "Retain" where we meant retain (not "return) * Add a check in MPI_ERRHANDLER_FREE to raise an MPI exception if the user attempts to free an intrinsic errhandler *and* the refcount is 1 (meaning that it would actually free the intrinsic). This protects erroneous programs from segv'ing. * Remove lengthy comment from comm_get_errhandler.c which is no longer valid (because of the MPI-2 errata that says that users *do* have to call MPI_ERRHANDLER_FREE). This commit was SVN r12128. The following SVN revision numbers were found above: r12122 --> open-mpi/ompi@407b3cb78819d363e7c2661a648459ae19a797e4 The following Trac tickets were found above: Ticket 502 --> https://svn.open-mpi.org/trac/ompi/ticket/502 --- ompi/mpi/c/comm_get_errhandler.c | 25 ++----------------------- ompi/mpi/c/errhandler_free.c | 24 ++++++++++++++++++++++-- ompi/mpi/c/file_delete.c | 11 ++++++----- ompi/mpi/c/file_get_errhandler.c | 10 +++++----- ompi/mpi/c/file_open.c | 11 ++++++----- ompi/mpi/c/file_set_errhandler.c | 5 +++-- ompi/mpi/c/win_get_errhandler.c | 4 ++-- 7 files changed, 46 insertions(+), 44 deletions(-) diff --git a/ompi/mpi/c/comm_get_errhandler.c b/ompi/mpi/c/comm_get_errhandler.c index fffd4d3e00..219344695c 100644 --- a/ompi/mpi/c/comm_get_errhandler.c +++ b/ompi/mpi/c/comm_get_errhandler.c @@ -48,29 +48,8 @@ int MPI_Comm_get_errhandler(MPI_Comm comm, MPI_Errhandler *errhandler) } } - /* Return the errhandler. A quandry. Should we increase the - refcount here? - - - Consider that if we *get* an errhandler, we don't have to free - it. It's just a handle that was returned to the user. If they - never free it (and we increased the refcount), then it'll never - be freed. - - - However, if we *don't* increase it and the user *does* free it, - then this could cause the refcount to go to 0 prematurely, and - a communicator could be left with a stale error handler. - - Add to the mix that MPI-1:196:8-11 says that MPI_ERRHANDLER_FREE - will only free the error handler when all the communicators using - it have been freed. - - All in all, it seems like we should increase the refcount to be - safe here. We're still conformant -- error handlers won't be - freed until all the communicators (or other objects using them) - are freed *and* any outstanding handles returned by this function - (or its peers) are also freed. - */ - + /* Retain the errhandler, corresponding to object refcount + decrease in errhandler_free.c. */ OBJ_RETAIN(comm->error_handler); *errhandler = comm->error_handler; diff --git a/ompi/mpi/c/errhandler_free.c b/ompi/mpi/c/errhandler_free.c index 87d8978e14..3d1653291c 100644 --- a/ompi/mpi/c/errhandler_free.c +++ b/ompi/mpi/c/errhandler_free.c @@ -37,13 +37,33 @@ int MPI_Errhandler_free(MPI_Errhandler *errhandler) if (MPI_PARAM_CHECK) { OMPI_ERR_INIT_FINALIZE(FUNC_NAME); - if (NULL == errhandler) { + /* Raise an MPI exception if we got NULL or if we got an intrinsic + *and* the reference count is 1 (meaning that this FREE would + actually free the underlying intrinsic object). This is ugly + but necessary -- see below. */ + if (NULL == errhandler || + (ompi_errhandler_is_intrinsic(*errhandler) && + 1 == (*errhandler)->super.obj_reference_count)) { return OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_ARG, "MPI_Errhandler_free"); } } - /* We have a valid errhandler, release it */ + /* Return the errhandler. According to MPI-2 errata, any errhandler + obtained by MPI_*_GET_ERRHANDLER or MPI_ERRHANDLER_GET must also + be freed by MPI_ERRHANDLER_FREE (including intrinsic error + handlers). For example, this is valid: + + int main() { + MPI_Errhandler errhdl; + MPI_Init(NULL, NULL); + MPI_Comm_get_errhandler(MPI_COMM_WORLD, &errhdl); + MPI_Errhandler_free(&errhdl); + MPI_Finalize(); + return 0; + } + + So decrease the refcount here. */ OBJ_RELEASE(*errhandler); *errhandler = MPI_ERRHANDLER_NULL; diff --git a/ompi/mpi/c/file_delete.c b/ompi/mpi/c/file_delete.c index ae5af54eac..e549fe213c 100644 --- a/ompi/mpi/c/file_delete.c +++ b/ompi/mpi/c/file_delete.c @@ -50,11 +50,12 @@ int MPI_File_delete(char *filename, MPI_Info info) OMPI_ERRHANDLER_CHECK(rc, MPI_FILE_NULL, rc, FUNC_NAME); } - /* Note that MPI-2:9.7 (p261) says that errors in MPI_FILE_OPEN - (before the file handle is created) should invoke the default - error handler on MPI_FILE_NULL. Hence, if we get a file handle - out of ompi_file_open(), invoke the error handler on that. If - not, invoke the error handler on MPI_FILE_NULL. */ + /* Note that MPI-2:9.7 (p265 in the ps; 261 in the pdf) says that + errors in MPI_FILE_OPEN (before the file handle is created) + should invoke the default error handler on MPI_FILE_NULL. + Hence, if we get a file handle out of ompi_file_open(), invoke + the error handler on that. If not, invoke the error handler on + MPI_FILE_NULL. */ /* The io framework is only initialized lazily. If it hasn't already been initialized, do so now (note that MPI_FILE_OPEN diff --git a/ompi/mpi/c/file_get_errhandler.c b/ompi/mpi/c/file_get_errhandler.c index d35415011e..5b5730db4f 100644 --- a/ompi/mpi/c/file_get_errhandler.c +++ b/ompi/mpi/c/file_get_errhandler.c @@ -39,8 +39,9 @@ int MPI_File_get_errhandler( MPI_File file, MPI_Errhandler *errhandler) if (MPI_PARAM_CHECK) { OMPI_ERR_INIT_FINALIZE(FUNC_NAME); - /* Note that MPI-2:9.7 (p261) explicitly says that you are - allowed to set the error handler on MPI_FILE_NULL */ + /* Note that MPI-2:9.7 (p265 in the ps; 261 in the pdf) explicitly + says that you are allowed to set the error handler on + MPI_FILE_NULL */ if (NULL == file) { return OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_FILE, @@ -51,9 +52,8 @@ int MPI_File_get_errhandler( MPI_File file, MPI_Errhandler *errhandler) } } - /* Return the errhandler. See lengthy comment in - comm_get_errhandler.c about why we increment the refcount. */ - + /* Retain the errhandler, corresponding to object refcount + decrease in errhandler_free.c. */ OBJ_RETAIN(file->error_handler); *errhandler = file->error_handler; diff --git a/ompi/mpi/c/file_open.c b/ompi/mpi/c/file_open.c index 534154265c..ac2062ac83 100644 --- a/ompi/mpi/c/file_open.c +++ b/ompi/mpi/c/file_open.c @@ -56,11 +56,12 @@ int MPI_File_open(MPI_Comm comm, char *filename, int amode, } - /* Note that MPI-2:9.7 (p261) says that errors in MPI_FILE_OPEN - (before the file handle is created) should invoke the default - error handler on MPI_FILE_NULL. Hence, if we get a file handle - out of ompi_file_open(), invoke the error handler on that. If - not, invoke the error handler on MPI_FILE_NULL. */ + /* Note that MPI-2:9.7 (p265 in the ps; p261 in the pdf) says that + errors in MPI_FILE_OPEN (before the file handle is created) + should invoke the default error handler on MPI_FILE_NULL. + Hence, if we get a file handle out of ompi_file_open(), invoke + the error handler on that. If not, invoke the error handler on + MPI_FILE_NULL. */ /* The io framework is only initialized lazily. If it hasn't already been initialized, do so now (note that MPI_FILE_OPEN diff --git a/ompi/mpi/c/file_set_errhandler.c b/ompi/mpi/c/file_set_errhandler.c index 54152d25a3..24c087ab76 100644 --- a/ompi/mpi/c/file_set_errhandler.c +++ b/ompi/mpi/c/file_set_errhandler.c @@ -39,8 +39,9 @@ int MPI_File_set_errhandler( MPI_File file, MPI_Errhandler errhandler) if (MPI_PARAM_CHECK) { OMPI_ERR_INIT_FINALIZE(FUNC_NAME); - /* Note that MPI-2:9.7 (p261) explicitly says that you are - allowed to set the error handler on MPI_FILE_NULL */ + /* Note that MPI-2:9.7 (p265 in the ps; p261 in the pdf) + explicitly says that you are allowed to set the error + handler on MPI_FILE_NULL */ if (NULL == file) { return OMPI_ERRHANDLER_INVOKE(MPI_FILE_NULL, MPI_ERR_FILE, diff --git a/ompi/mpi/c/win_get_errhandler.c b/ompi/mpi/c/win_get_errhandler.c index a3e568dcdc..aa39b01f15 100644 --- a/ompi/mpi/c/win_get_errhandler.c +++ b/ompi/mpi/c/win_get_errhandler.c @@ -45,8 +45,8 @@ int MPI_Win_get_errhandler(MPI_Win win, MPI_Errhandler *errhandler) } } - /* Return the errhandler. See lengthy comment in - comm_get_errhandler.c about why we increment the refcount. */ + /* Retain the errhandler, corresponding to object refcount + decrease in errhandler_free.c. */ OBJ_RETAIN(win->error_handler); *errhandler = win->error_handler;