Skip to content
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

Jump to bottom

Fix MPI1 function removal [master] Issue 6114 [API Change] #6358

Merged
merged 2 commits into from
Feb 27, 2019
Merged

Fix MPI1 function removal [master] Issue 6114 [API Change] #6358

merged 2 commits into from
Feb 27, 2019

Conversation

gpaulsen
Copy link
Member

@gpaulsen gpaulsen commented Feb 5, 2019 •
edited by jsquyres
Loading

This is the master PR to remove MPI1 functions and datatypes as specified by MPI 3.0 standard.

On Master (i.e. destined for Open MPI v5.0) we plan to remove the --enable-mpi1-compatibilty configure param, and this PR does that. with the 2nd commit.
This PR also fixes the removed APIs for a cherry-pick to v4.0.x with the 1st commit.

NOTE: The intent of this PR is to fulfill the plan discussed in #6278 (comment), with the addition of the error attribute suggested in #6278 (comment).

@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch from 546eb5c to 1b205ef Compare February 5, 2019 15:51
@gpaulsen gpaulsen changed the title Fix MPI1 function removal - Issue 6114 Fix MPI1 function removal [master] Issue 6114 Feb 5, 2019
@gpaulsen gpaulsen mentioned this pull request Feb 5, 2019
Merged
@ibm-ompi
Copy link

ibm-ompi commented Feb 5, 2019

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/bfbdaa16f861d5677284b96b5694f3a2

@ibm-ompi
Copy link

ibm-ompi commented Feb 5, 2019

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/f93c7e9e4139587aa290c958416924f9

@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch 2 times, most recently from cdb9729 to a620328 Compare February 8, 2019 19:47
@ibm-ompi
Copy link

ibm-ompi commented Feb 8, 2019

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/3bf7fee099a6b8e96a4c3adb1e1b6364

@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch from a620328 to a310b4d Compare February 8, 2019 19:52
@ibm-ompi
Copy link

ibm-ompi commented Feb 8, 2019

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/91f27190d50e15e0d3f5e8e2fc09a3a6

@ibm-ompi
Copy link

ibm-ompi commented Feb 8, 2019

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/8025f07c0a154e876aa20b4497db87f9

@ibm-ompi
Copy link

ibm-ompi commented Feb 8, 2019

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/e739351289fb04bfc387cd4243239f77

@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch 2 times, most recently from ed18e56 to b97065b Compare February 11, 2019 20:05
@ibm-ompi
Copy link

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/0d45438f6655e30607c8d7debd36c025

@ibm-ompi
Copy link

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/92e2f4ab1d76d77c12a05023464d4d6f

@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch from b97065b to 44d2976 Compare February 12, 2019 00:00
@ibm-ompi
Copy link

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/a6465e9935d5bcd3ee386cccb0d3456d

@ibm-ompi
Copy link

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/1a9ea3095a0d3b8d0aa69e3da351aee2

@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch from 44d2976 to 3101339 Compare February 12, 2019 14:28
@gpaulsen
Copy link
Member Author

I'd like the 1st commit to be the same or similar to the commit I cherry-pick to v4.0.x via PR #6359, and that one is still in test. Added WIP-DNM until that's finished (hopefully just a few hours to finish testing all of the cases).

@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch from 3101339 to 97f51e4 Compare February 15, 2019 23:41
@ibm-ompi
Copy link

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/91544986c0b2f860f5ef19aee9f71d6a

@ibm-ompi
Copy link

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/c3dbf85bd032a0d5ae8a45747402cc7f

@gpaulsen gpaulsen requested a review from jsquyres February 15, 2019 23:52
@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch from 97f51e4 to 76c085f Compare February 16, 2019 00:00
@ibm-ompi
Copy link

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/8c975f100dc8d47d2a42b9ef7ec78639

@ibm-ompi
Copy link

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/498f7fed1ad1af9d3f221989362cf5dd

@gpaulsen gpaulsen changed the title Fix MPI1 function removal [master] Issue 6114 Fix MPI1 function removal [master] Issue 6114 [API Change] Feb 16, 2019
@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch from 76c085f to 0589597 Compare February 16, 2019 00:13
@gpaulsen gpaulsen mentioned this pull request Feb 19, 2019
Closed
jsquyres
jsquyres requested changes Feb 19, 2019
View reviewed changes
@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch from 0589597 to 23d4b82 Compare February 21, 2019 19:12
@ibm-ompi
Copy link

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/2b49c3756420572936915ff0681bf9b6

@ibm-ompi
Copy link

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/333695f5c3c796f0ca9654bcc15481da

@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch from 23d4b82 to 5700f4f Compare February 21, 2019 19:20
@ibm-ompi
Copy link

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/292ef02006fb0179ecc214043b523799

@ibm-ompi
Copy link

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/d3373b1d7e4c01e4debf5fa923905db5

@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch 2 times, most recently from 180bd3a to 1a8bbbc Compare February 21, 2019 20:10
@gpaulsen gpaulsen force-pushed the topic/master/mpi1removal branch from 1a8bbbc to edf334d Compare February 21, 2019 21:21
@gpaulsen gpaulsen requested a review from jsquyres February 21, 2019 22:25
@gpaulsen
Copy link
Member Author

@jsquyres I've addressed all of the issues, please re-review.

@jsquyres jsquyres force-pushed the topic/master/mpi1removal branch from edf334d to fa4ac1c Compare February 27, 2019 01:42
@jsquyres
Copy link
Member

jsquyres commented Feb 27, 2019 •
edited
Loading

@gpaulsen All issues are fixed; thanks.

I did make some suggested changes, though:

  1. I edited your commit messages to be a bit more explicit. I apologize; I am being super pedantic about this because this is super-confusing code, and it's a Moment (in the Dr. Who sense of the word): I predict that we will be coming back to look at this git commit in the future. Meaning: we need to be as explicit as possible so that our Future Selves don't hate us.
  2. Additionally, I added a 2nd commit in the middle with a few suggested code changes to mpi.h.in (and updated the 3rd commit to compensate):
    1. Use less confusing indenting of the #if blocks
    2. Change OMPI_REMOVED_STATIC_ASSERT_MSG to THIS_SYMBOL_WAS_REMOVED_IN_MPI30 because I observed that gcc 8.x (and possibly others?) shows the macro name in when the static_assert code is activated and the user uses one of the old MPI-1 functions in their code -- like this:
// bogus.c
#include <mpi.h>

int main(int argc, char *argv[]) {
    int i; void *a;
    MPI_Address(&i, &a);
    return 0;
}
$ mpicc bogus.c
In file included from bogus.c:1:
bogus.c: In function ‘main’:
/home/jsquyres/bogus/include/mpi.h:320:57: error: static assertion failed: "MPI_Address was removed in MPI-3.0.  Use MPI_Get_address instead."
 #define THIS_SYMBOL_WAS_REMOVED_IN_MPI30(func, newfunc) _Static_assert(0, #func " was removed in MPI-3.0.  Use " #newfunc " instead.")
                                                         ^~~~~~~~~~~~~~
/home/jsquyres/bogus/include/mpi.h:2814:27: note: in expansion of macro ‘THIS_SYMBOL_WAS_REMOVED_IN_MPI30’
 #define MPI_Address(...)  THIS_SYMBOL_WAS_REMOVED_IN_MPI30(MPI_Address, MPI_Get_address)
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
bogus.c:5:5: note: in expansion of macro ‘MPI_Address’
     MPI_Address(&i, &a);
     ^~~~~~~~~~~

I figure that we might get fewer false bug reports from users if the name THIS_SYMBOL_WAS_REMOVED_IN_MPI30 is staring them in the face.

If you are kosher with the changes, I will squash the 2nd commit into the 1st commit, and then this PR is good to go, IMHO.

@gpaulsen
Copy link
Member Author

Thanks for your pedantic-ness. Yes, I agree about the pivotal nature of this, hopefully not, but who knows.

Okay, yes I like the THIS_SYMBOL_WAS_REMOVED_IN_MPI30 idea. Feel free to squish, or I can.

@gpaulsen @jsquyres
mpi.h.in: Revamp MPI-1 removed function warnings
3136a17 
Refs open-mpi#6278.

This commit is intended to be cherry-picked to v4.0.x and
the following commit will ammend to this functionality for
master's removal.

Changes the prototypes for MPI removed functions in the
following ways:

There are 4 cases:

 1) User wants MPI-1 compatibility (--enable-mpi1-compatibility)

    MPI_Address (and friends) are declared in mpi.h with
    deprecation notice

 2) User does not want MPI-1 compatibility, and has a C11-capable
    compiler

    Declare an MPI_Address (etc.) macro in mpi.h, which will
    cause a compile-time error using _Static_assert C11 feature

 3) User does not want MPI-1 compatibility, and does not have a
    C11-capable compiler, but the compiler supports error function
    attributes.

    Declare an MPI_Address (etc.) macro in mpi.h, which will
    cause a compile-time error using error function attribute.

 4) User does not want MPI-1 compatibility, and does not have a
    C11-capable compiler, or a compiler that supports error
    function attributes.

    Do not declare MPI_Address (etc.) in mpi.h at all.
    Unless the user is compiling with something like -Werror,
    this will allow the user's code to compile. We are
    choosing this because it seems like a losing battle to
    make some kind of compile time error that is friendly to
    the user (and doesn't make it look like mpi.h itself is broken).

    On v4.0.x, this will allow the user code to both compile
    (albeit with a warning) and link (because the MPI_Address
    will be in the MPI library because we are preserving ABI
    back to 3.0.x).

    On master/v5.0.x, this will allow the user code to compile,
    but it will fail to link (because the MPI_Address symbol will
    not be in the MPI library).

Signed-off-by: Geoffrey Paulsen <[email protected]>
@gpaulsen @jsquyres
mpi.h.in: delete removed MPI1 functions/datatypes (API change!)
a6d6be2 
This commit DELETES the removed MPI1 functions and datatypes from
both the mpi.h header and from the library (they were deleted from the
MPI standard in MPI-3.0).

WARNING: This changes the MPI API in a non-backwards compatible way.
         This also removes the configure option that was added in Open
         MPI v4.0.x, requiring users to change their apps if they are
         using any of these almost 20 year old APIs.

This commit removes the following MPI1 removed functions and datatypes:

         MPI_Address
         MPI_Errhandler_create
         MPI_Errhandler_get
         MPI_Errhandler_set
         MPI_Type_extent
         MPI_Type_hindexed
         MPI_Type_hvector
         MPI_Type_struct
         MPI_Type_UB
         MPI_Type_LB

Signed-off-by: Geoffrey Paulsen <[email protected]>
@jsquyres jsquyres force-pushed the topic/master/mpi1removal branch from fa4ac1c to a6d6be2 Compare February 27, 2019 16:24
jsquyres
jsquyres approved these changes Feb 27, 2019
View reviewed changes
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

Squashed down to 2 commits. Good to go when CI completes.

@gpaulsen gpaulsen merged commit 45fb0c1 into open-mpi:master Feb 27, 2019
@gpaulsen gpaulsen deleted the topic/master/mpi1removal branch February 27, 2019 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsquyres jsquyres jsquyres approved these changes

Assignees
No one assigned
Labels
Target: main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gpaulsen @ibm-ompi @jsquyres