Skip to content

Allow name query for MPI_DATATYPE_NULL. #10029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Allow name query for MPI_DATATYPE_NULL. #10029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ompi/mpi/c/comm_get_name.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Copyright (c) 2004-2007 The Trustees of Indiana University and Indiana
* University Research and Technology
* Corporation. All rights reserved.
* Copyright (c) 2004-2005 The University of Tennessee and The University
* Copyright (c) 2004-2022 The University of Tennessee and The University
* of Tennessee Research Foundation. All rights
* reserved.
* Copyright (c) 2004-2008 High Performance Computing Center Stuttgart,
Expand Down Expand Up @@ -52,7 +52,11 @@ int MPI_Comm_get_name(MPI_Comm comm, char *name, int *length)
if ( MPI_PARAM_CHECK ) {
OMPI_ERR_INIT_FINALIZE(FUNC_NAME);

if ( ompi_comm_invalid ( comm ) )
/* Do not use ompi_comm_invalid, it prevent returning
* the name for the predefined MPI_COMM_NULL.
*/
if ((NULL == comm) ||
(OMPI_COMM_IS_FREED(comm)) || (OMPI_COMM_IS_INVALID(comm)) )
return OMPI_ERRHANDLER_INVOKE ( MPI_COMM_WORLD, MPI_ERR_COMM,
FUNC_NAME);

Expand Down
2 changes: 1 addition & 1 deletion ompi/mpi/c/type_get_name.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ int MPI_Type_get_name(MPI_Datatype type, char *type_name, int *resultlen)

if ( MPI_PARAM_CHECK ) {
OMPI_ERR_INIT_FINALIZE(FUNC_NAME);
if (NULL == type || MPI_DATATYPE_NULL == type) {
if (NULL == type) {
return OMPI_ERRHANDLER_NOHANDLE_INVOKE(MPI_ERR_TYPE,
FUNC_NAME );
} else if (NULL == type_name || NULL == resultlen) {
Expand Down
9 changes: 7 additions & 2 deletions ompi/mpi/c/win_get_name.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Copyright (c) 2004-2007 The Trustees of Indiana University and Indiana
* University Research and Technology
* Corporation. All rights reserved.
* Copyright (c) 2004-2020 The University of Tennessee and The University
* Copyright (c) 2004-2022 The University of Tennessee and The University
* of Tennessee Research Foundation. All rights
* reserved.
* Copyright (c) 2004-2005 High Performance Computing Center Stuttgart,
Expand Down Expand Up @@ -44,7 +44,12 @@ int MPI_Win_get_name(MPI_Win win, char *win_name, int *resultlen)
if (MPI_PARAM_CHECK) {
OMPI_ERR_INIT_FINALIZE(FUNC_NAME);

if (ompi_win_invalid(win)) {
/* Do not use ompi_comm_invalid, it prevent returning
* the name for the predefined MPI_COMM_NULL.
*/
if (NULL == win ||
(OMPI_WIN_INVALID & win->w_flags) ||
(OMPI_WIN_FREED & win->w_flags)) {
return OMPI_ERRHANDLER_NOHANDLE_INVOKE(MPI_ERR_WIN, FUNC_NAME);
} else if (NULL == win_name || NULL == resultlen) {
return OMPI_ERRHANDLER_INVOKE(win, MPI_ERR_ARG, FUNC_NAME);
Expand Down
3 changes: 1 addition & 2 deletions ompi/win/win.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Copyright (c) 2004-2005 The Trustees of Indiana University and Indiana
* University Research and Technology
* Corporation. All rights reserved.
* Copyright (c) 2004-2017 The University of Tennessee and The University
* Copyright (c) 2004-2022 The University of Tennessee and The University
* of Tennessee Research Foundation. All rights
* reserved.
* Copyright (c) 2004-2005 High Performance Computing Center Stuttgart,
Expand Down Expand Up @@ -124,7 +124,6 @@ int ompi_win_init (void)

/* Setup MPI_WIN_NULL */
OBJ_CONSTRUCT(&ompi_mpi_win_null.win, ompi_win_t);
ompi_mpi_win_null.win.w_flags = OMPI_WIN_INVALID;
Copy link
Member

@jsquyres jsquyres Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe to do? Are there other places that rely on this flag being set upon construction?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only use I could find is in https://github.com/open-mpi/ompi/blob/master/ompi/win/win.h#L155 where the window is explicitly checked for MPI_WIN_NULL as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All functions check the validity of the window using ompi_win_invalid, a functions that specifically checks for MPI_WIN_NULL (in addition to checking the OMPI_WIN_INVALID flag). No other usages of OMPI_WIN_INVALID exists in the code.

It is in fact the same story with all the _INVALID flags so it would be legitimate to wonder why do we need the _INVALID flag at all, but I don't think we want to have this discussion in the context of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If OMPI_WIN_INVALID is only used in 1) the constructor and 2) in ompi_win_invalid(), and you just removed it out of the constructor, then we should also remove it out of ompi_win_invalid(), too. Otherwise it's a dangling check for something that can never be true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly what I said above (and it is true for all uses of _INVALID). But I don't think it should be part of this PR.

ompi_mpi_win_null.win.w_group = &ompi_mpi_group_null.group;
OBJ_RETAIN(&ompi_mpi_group_null);
ompi_win_set_name(&ompi_mpi_win_null.win, "MPI_WIN_NULL");
Expand Down