Skip to content

Fix MPI1 function removal [v4.0.x] Issue 6114 #6359

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.

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

Merged
merged 1 commit into from
Feb 27, 2019

Conversation

gpaulsen
Copy link
Member

@gpaulsen gpaulsen commented Feb 5, 2019

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).

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

    There are 4 cases:

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

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

    • 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

    • 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.

    • 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).

(cherry picked from commit 4214b5a)
Signed-off-by: Geoffrey Paulsen [email protected]

Conflicts:
ompi/include/mpi.h.in

@gpaulsen
Copy link
Member Author

gpaulsen commented Feb 5, 2019

Cherry picked for one of the commits on #6358

@gpaulsen
Copy link
Member Author

gpaulsen commented Feb 5, 2019

Adding work in progress, because the autogenerated profiling API is not correct.

@gpaulsen
Copy link
Member Author

gpaulsen commented Feb 5, 2019

Intended solution for #6278
and #6114

@gpaulsen gpaulsen requested a review from jsquyres February 5, 2019 15:56
@gpaulsen gpaulsen changed the title Fix MPI1 function removal [v4.0.x] Issue 6114 Fix MPI1 function removal [v4.0.x] Feb 5, 2019
@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/996bac17bf0d8dfa2ae1491172c1aee1

@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/c804b26d21d7689725509a6c83243884

@gpaulsen
Copy link
Member Author

@jsquyres @hppritcha, I think this is finally ready to go.
I've tested all of the various test cases on two different platforms with two different compilers.
Once it passed all of that, I then did the same on Master in one commit, and cherry-picked it here, and added a 2nd commit on the master PR #6358 that ACTUALLY removes all of this on master.

@jsquyres
Copy link
Member

Your commit message says that there are 4 cases, but you list 5 cases. One of them seems to be an incomplete duplicate (a copy-n-paste error?). The indenting is weird, too -- are you trying to make 3 of them be sub-bullets?

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.

Requested changes on associated master PR (#6358).

@gpaulsen gpaulsen force-pushed the topic/v4.0.x/mpi1removal branch from d0675c1 to f55db15 Compare February 21, 2019 19:29
@ibm-ompi
Copy link

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

Gist: https://gist.github.com/68faa9761389f8182c21cfa987ba3b2b

@ibm-ompi
Copy link

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

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

@gpaulsen gpaulsen force-pushed the topic/v4.0.x/mpi1removal branch from f55db15 to a4fdaf5 Compare February 21, 2019 20:17
@gpaulsen gpaulsen force-pushed the topic/v4.0.x/mpi1removal branch from a4fdaf5 to 7603dc6 Compare February 21, 2019 21:23
@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.

@gpaulsen gpaulsen changed the title Fix MPI1 function removal [v4.0.x] Fix MPI1 function removal [v4.0.x] Issue 6114 Feb 22, 2019
@jsquyres jsquyres force-pushed the topic/v4.0.x/mpi1removal branch 2 times, most recently from 24d04c2 to b0a7ba1 Compare February 27, 2019 01:45
@jsquyres
Copy link
Member

I updated the commit message on the commit on this PR (and updated the cherry pick hash to match the new hash on 6358) and also added a 2nd commit on this PR with the same suggestions I made in #6358 (comment).

@ibm-ompi
Copy link

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

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

@jsquyres jsquyres force-pushed the topic/v4.0.x/mpi1removal branch from b0a7ba1 to a895bde Compare February 27, 2019 02:04
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]>
(cherry-picked from 3136a17)
@jsquyres jsquyres force-pushed the topic/v4.0.x/mpi1removal branch from a895bde to 6df6a3f Compare February 27, 2019 16:25
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 1 commit; hash updated to match the hash from #6358.

@gpaulsen gpaulsen merged commit c0ee7ad into open-mpi:v4.0.x Feb 27, 2019
@gpaulsen gpaulsen deleted the topic/v4.0.x/mpi1removal branch February 27, 2019 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants