v4.0.x: Re-add removed deprecate-only MPI-2.0 symbols #6120

gpaulsen · 2018-11-26T21:28:19Z

I restored the APIs in mpi.h in commit 2d3b4bb, but forgot the Makefile changes. :(

Fixes #6114

Signed-off-by: Bert Wesarg bert.wesarg@tu-dresden.de

jsquyres

@gpaulsen This is not enough. Per #6114 (comment), we need to always include all the symbols in libmpi.

gpaulsen · 2018-11-26T21:50:35Z

Thanks I'll take a look

ggouaillardet · 2018-11-27T00:03:54Z

is there any reason why this is not a cherry-pick/backport of #6118 ?

ggouaillardet · 2018-11-27T04:43:56Z

you will also need to cherry-pick the commit from #6122 once merged into master

bertwesarg · 2018-11-27T11:55:47Z

BTW, if it comes to the release time, I don't think that this fix should effect the shared library version at all. As 4.0.0 broke the c:r:a anyway.

jsquyres · 2018-11-27T18:25:07Z

@bertwesarg Agreed -- the major .so version should not change in this release. I know we'll change r (because we always have to change r when the library changes); I haven't thought through whether c and a need to change. We might need to change them for a different reason (e.g., #6110) -- i.e., bump both c and a.

gpaulsen · 2018-11-29T12:25:58Z

@ggouaillardet, This is a cherry-pick of #6118, but I forgot the -x. Redoing.

I've also cherry-picked #6122 here too.

I've searched through all of the removed symbols, and the C/Fortran symbols are all present in the Makefiles, though the removed functions are only built when the configure option --enable-mpi1-compatibility is set. I believe I'll need to do a bit of further work on v4.0.x branch to always build the removed functions in the library (for v4.0.x), based on this text: https://github.com/open-mpi/ompi/blob/v4.0.x/README#L510

gpaulsen · 2018-11-29T15:23:39Z

Okay, previous CI passed on two commits on this branch.

I've now pushed the 3rd and final commit to this branch, which restores the removed functions to the mpi library (but not to the header, unless the configure flag is thrown).

@jsquyres, @hjelmn please re-review.

jsquyres · 2018-11-29T15:33:31Z

ompi/mpi/c/Makefile.am

@@ -437,7 +437,8 @@ libmpi_c_mpi_la_SOURCES = \
        win_wait.c


-if OMPI_ENABLE_MPI1_COMPAT


How about putting these files just back up (alphabetically) in the main list of files?

Also, this specific commit was not a cherry pick, right? Can you please put a comment about that in the commit message? (i.e., why this is not a cherry pick: because on master, we are no longer building those symbols by default, but on v4.0 we are still building those symbols by default, yadda yadda yadda)

I kinda liked having them separated out at the bottom to help indicate that they're going away.

I will rework the commit message to indicate why we want/need this v4.0.x specific patch (instead of a cherry-pick from master).

jsquyres

@gpaulsen This PR fails to compile.

gpaulsen · 2018-11-29T15:42:41Z

Drat, I just saw that too... working.

ibm-ompi · 2018-11-29T15:56:37Z

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

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

ibm-ompi · 2018-11-29T15:59:06Z

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

Gist: https://gist.github.com/30c8322f19b427bcad29721be9a5e53c

ibm-ompi · 2018-11-29T16:10:53Z

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

Gist: https://gist.github.com/4f23e242e8c62b305455a39c368a37f4

ibm-ompi · 2018-11-29T16:54:49Z

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

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

bertwesarg · 2018-11-29T18:30:38Z

I think OMPI_OMIT_MPI1_COMPAT_DECLS must be 0 if OMPI_BUILDING is 1. But it looks like it's not:

#if !defined(OMPI_OMIT_MPI1_COMPAT_DECLS)
#    define OMPI_OMIT_MPI1_COMPAT_DECLS !OMPI_ENABLE_MPI1_COMPAT
#endif

ibm-ompi · 2018-11-29T19:50:49Z

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

Gist: https://gist.github.com/062b323ebc9bef2b179451e13b8c8a06

gpaulsen · 2018-11-29T23:31:45Z

@bertwesarg, I just repushed. And instead in the Fortran implementations I set OMPI_OMIT_MPI1_COMPAT_DECLS to 0.
Isn't that what we want, instead of !OMPI_ENABLE_MPI1_COMPAT ?

bertwesarg · 2018-11-30T06:25:46Z

Isn't that what we want, instead of !OMPI_ENABLE_MPI1_COMPAT ?

That is the current code in master, not a proposal from me.

From my point of view (I'm from the Score-P measurement infrastructure team), I would like to have a hook to enable these prototypes also past 'make install', something like being able to skip the C++ interface.

Let me elaborate:

Score-P wants to intercepts all function calls from the application. We don't care about deprecations at all. If the application calls an MPI function, we want to know it. Now that the functions are in the library but no prototype, we skip our wrappers in our build process as we don't find the declarations.

so having a hook like OMPI_PLEASE_NO_HIDDEN_PROTOTYPES in the mpi.h header would allow us to intercept all functions again. Would that something you could consider. As you actually need the same thing now anyway. It looks like a perfect fit to me. Thanks.

PS: we also would like to skip any deprecation warnings from the mpi.h header.

jsquyres

Something still isn't right. When I compile without --enable-mpi1-compatibility, I still see a bunch of warnings like this:

perrhandler_create.c:35:31: warning: no previous prototype for ‘PMPI_Errhandler_create’ [-Wmissing-prototypes]
 #define MPI_Errhandler_create PMPI_Errhandler_create
                               ^~~~~~~~~~~~~~~~~~~~~~
perrhandler_create.c:38:5: note: in expansion of macro ‘MPI_Errhandler_create’
 int MPI_Errhandler_create(MPI_Comm_errhandler_function *function,
     ^~~~~~~~~~~~~~~~~~~~~

gpaulsen · 2018-12-11T17:06:06Z

:( I'll take a look

gpaulsen · 2018-12-11T20:12:37Z

I've got a fix, and will push soon.

gpaulsen · 2018-12-11T20:34:11Z

@jsquyres please review. I can squish the last two commits if you'd like.

bertwesarg · 2018-12-11T20:47:27Z

ompi/mpi/c/address.c

+ * to the newer MPI calls, we are defining OMPI_OMIT_MPI1_COMPAT_DECLS
+ * to 0 before including the c bindings.
+ */
+#define OMPI_OMIT_MPI1_COMPAT_DECLS 0


Shouldn't the OMPI_BUILDING #define be sufficient here? If not, why?

So, there's currently no logic to affect OMPI_OMIT_MPI1_COMPAT_DECLS via OMPI_BUILDING. Are you proposing we add that?

OMPI_BUILDING already overrules OMPI_WANT_MPI_INTERFACE_WARNING, where the latter is for users. Why shouldn't OMPI_BUILDING also overrule OMPI_OMIT_MPI1_COMPAT_DECLS, and leave OMPI_OMIT_MPI1_COMPAT_DECLS to the users?

What do you mean by "leave OMPI_OMIT_MPI1_COMPAT_DECLS to the users?" Users can control that already via the --enable-mpi-compatibility configure flag. This is just silently unsetting during the build of a few files in the .c files (not mpi.h) to allow those files to build.

As long as the symbols are in the library, I think its valid to access them by there prototype. I can understand that by default you hide them, but having a way to reveal the prototypes is very convenient. As not all users can wait until the used software project changes the software to not use deprecated APIs.

As a second user case, we the tools developers nevertheless want to wrap as many API as possible. If we know, that if we set some magic variable and get all prototypes for all available symbols. We certainly will use it.

If OMPI_OMIT_MPI1_COMPAT_DECLS is that hook, to get all prototypes, than its something for the users.

@gpaulsen I think the point here is that OMPI_BUILDING is a flag we use internally -- it means that additional things should be available because we are building Open MPI itself (vs. a user MPI application). I think @bertwesarg is saying that OMPI_BUILDING should probably also imply that the MPI1 compatibility prototypes should be available if OMPI_BUILDING==1. That way, OMPI_OMIT_MPI1_COMPAT_DECLS is purely tied to --enable-mpi-compatibility (i.e., solely "for the users").

I see, let me see if I can do that and repush, for your review.

Adding the implementations of the functions that were removed from the MPI standard to the build list, regardless of the state of the OMPI_ENABLE_MPI1_COMPAT. According to the README, we want the OMPI_ENABLE_MPI1_COMPAT configure flag to control which MPI prototypes are exposed in mpi.h, NOT, which are built into the mpi library. Those will remain in the mpi library until a future major release (5.0?) NOTE: for the Fortran implementations, we instead define OMPI_OMIT_MPI1_COMPAT_DECLS to 0 instead of OMPI_ENABLE_MPI1_COMPAT to 1. I'm not sure why, but this seems to work correctly. Also changing the removed MPI_Errhandler_create implementation to use the non removed MPI_Comm_errhandler_function prototype (prototype remains unchanged from MPI_Comm_errhandler_fn) NOTE: This commit is *NOT* a cherry-pick from master, because on master, we are no longer building those symbols by default, but on v4.0.x we _ARE_ still building these symbols by default. This is because the v4.0.x branch is to remain backwards compatible with v3.0.x, while at the same time removing the "removed" symbols from mpi.h (unless the user configures with --enable-mpi1-compatibility) Signed-off-by: Geoffrey Paulsen <gpaulsen@us.ibm.com>

hppritcha · 2019-01-03T16:28:34Z

@jsquyres @gpaulsen says he pushed final changes to this PR on 12/26. Could you double check and sign off if it looks good now?

jsquyres · 2019-01-08T00:53:17Z

ompi/mpi/fortran/mpif-h/type_ub_f.c

@@ -12,6 +12,7 @@
 * Copyright (c) 2011-2012 Cisco Systems, Inc.  All rights reserved.
 * Copyright (c) 2015      Research Organization for Information Science
 *                         and Technology (RIST). All rights reserved.
+ * Copyright (c) 2018      IBM Corporation.  All rights reserved.


I think you should remove the copyright addition from the files where you didn't make any changes (I count 10 files like this).

jsquyres · 2019-01-08T00:55:14Z

ompi/mpi/c/address.c

+ *
+ * To prevent having to port these implementations of removed functions
+ * to the newer MPI calls, we are defining ENABLE_MPI1_COMPAT to 1
+ * before including the c bindings.


I cannot parse this statement -- what exactly does it mean? Why would we port MPI_ADDRESS to be implemented on top of MPI_GET_ADDRESS?

The intent was to communicate that we are NOT porting the implementations of these removed mpi1 functions to use the NEWer APIs, and instead we're doing Maco magic to allow us to build the older functions for the library only.
This latest push changed the mechanism that we do this, and no longer do we SET ENABLE_MPI1_COMPAT internally when we build these few routines, but instead this PR adds a || OMPI_BUILDING to the cpp logic around the prototypes of these removed functions to allow us to build them into the library.

I suspect all of this is clear ( || OMPI_BUILDING is much more straight forward), so I've just removed this paragraph.

hppritcha · 2019-01-10T03:08:31Z

I'm getting tired of this . @jsquyres we are going to take this as is.

getting tired of nitpicks

gpaulsen · 2019-01-10T13:51:44Z

Sorry, I put work in progress on here because There was a real change that was still needed. I'll add another commit on top of this in a separate PR.

hppritcha · 2019-01-10T13:52:18Z

oops I didn't notice the DWI tag. @gpaulsen revert an reopen if there was more to go in. sorry. shouldn't do these with my iphone.

jsquyres · 2019-01-10T15:37:51Z

This PR should be reverted. It was not ready to be merged.

gpaulsen · 2019-01-10T15:52:21Z

Okay, I can revert this.
Do I then open another PR for the revert, and another for the final?

jsquyres · 2019-01-10T15:52:58Z

Just click the "revert" button on this PR.

bertwesarg · 2019-03-02T21:36:51Z

@bertwesarg if you did not configure --enable-mpi1-compatibility, you can use the removed symbols in mpi.h by adding -DOMPI_OMIT_MPI1_COMPAT_DECLS=0 to your CPPFLAGS.

This does not work with the 4.0.1rc1, as I only get a

include/mpi.h:315:0: warning: "OMPI_OMIT_MPI1_COMPAT_DECLS" redefined

but not the hidden prototypes. The following patch to v4.0.1 gives me the desired behavior:

diff --git i/ompi/include/mpi.h.in w/ompi/include/mpi.h.in
index 27feae4ad8..03f095013b 100644
--- i/ompi/include/mpi.h.in
+++ w/ompi/include/mpi.h.in
@@ -306,7 +306,8 @@
           * Don't do MACRO magic for building Profiling library as it
           * interferes with the above.
           */
-#        if (OMPI_ENABLE_MPI1_COMPAT || OMPI_BUILDING)
+#        if defined(OMPI_OMIT_MPI1_COMPAT_DECLS)
+#        elif (OMPI_ENABLE_MPI1_COMPAT || OMPI_BUILDING)
 #            define OMPI_OMIT_MPI1_COMPAT_DECLS 0
 #            define OMPI_REMOVED_USE_STATIC_ASSERT 0
 #            define __mpi_interface_removed__(func, newfunc) __mpi_interface_deprecated__(#func " was removed in MPI-3.0.  Use " #newfunc " instead.  continuing...")

@ggouaillardet

As mentioned in [1], it may be desirable to nevertheless get the hidden MPI 1 prototypes, for users who know what they are doing, i.e., the tools guys. @ggouaillardet mentioned in [2], that `-DOMPI_OMIT_MPI1_COMPAT_DECLS=0` should work, but it does not, as than we only get redefinition warnings. See [3]. This topic does not relate to master, as we can remove the actual symbols there, but here in v4.0.x land, the symbols are always there. [1] open-mpi#6120 (comment) [2] open-mpi#6120 (comment) [3] open-mpi#6120 (comment)

@ggouaillardet

Follow-up to open-mpi#6120. As mentioned in [1], it may be desirable to nevertheless get the hidden MPI 1 prototypes, for users who know what they are doing, i.e., the tools guys. @ggouaillardet mentioned in [2], that `-DOMPI_OMIT_MPI1_COMPAT_DECLS=0` should work, but it does not, as than we only get redefinition warnings. See [3]. This topic does not relate to master, as we can remove the actual symbols there, but here in v4.0.x land, the symbols are always there. [1] open-mpi#6120 (comment) [2] open-mpi#6120 (comment) [3] open-mpi#6120 (comment) Signed-off-by: Bert Wesarg <bert.wesarg@tu-dresden.de>

@ggouaillardet

Follow-up to open-mpi#6120. As mentioned in [1], it may be desirable to nevertheless get the hidden MPI 1 prototypes, for users who know what they are doing, i.e., the tools guys. @ggouaillardet mentioned in [2], that `-DOMPI_OMIT_MPI1_COMPAT_DECLS=0` should work, but it does not, as than we only get redefinition warnings. See [3]. This topic does not relate to master, as we can remove the actual symbols there, but here in v4.0.x land, the symbols are always there. [1] open-mpi#6120 (comment) [2] open-mpi#6120 (comment) [3] open-mpi#6120 (comment) Signed-off-by: Bert Wesarg <bert.wesarg@tu-dresden.de>

@ggouaillardet

Follow-up to open-mpi#6120. As mentioned in [1], it may be desirable to nevertheless get the hidden MPI 1 prototypes, for users who know what they are doing, i.e., the tools guys. @ggouaillardet mentioned in [2], that `-DOMPI_OMIT_MPI1_COMPAT_DECLS=0` should work, but it does not, as than we only get redefinition warnings. See [3]. [1] open-mpi#6120 (comment) [2] open-mpi#6120 (comment) [3] open-mpi#6120 (comment) Signed-off-by: Bert Wesarg <bert.wesarg@tu-dresden.de> Signed-off-by: Brian Barrett <bbarrett@amazon.com>

@ggouaillardet

Follow-up to open-mpi#6120. As mentioned in [1], it may be desirable to nevertheless get the hidden MPI 1 prototypes, for users who know what they are doing, i.e., the tools guys. @ggouaillardet mentioned in [2], that `-DOMPI_OMIT_MPI1_COMPAT_DECLS=0` should work, but it does not, as than we only get redefinition warnings. See [3]. [1] open-mpi#6120 (comment) [2] open-mpi#6120 (comment) [3] open-mpi#6120 (comment) BWB Note: This patch was originally applied directly to v4.0.x in 73134ab, but now needs to be added to master, as we are keeping the 4.x mpi1-compat behavior. Signed-off-by: Bert Wesarg <bert.wesarg@tu-dresden.de> Signed-off-by: Brian Barrett <bbarrett@amazon.com>

@ggouaillardet

Follow-up to open-mpi#6120. As mentioned in [1], it may be desirable to nevertheless get the hidden MPI 1 prototypes, for users who know what they are doing, i.e., the tools guys. @ggouaillardet mentioned in [2], that `-DOMPI_OMIT_MPI1_COMPAT_DECLS=0` should work, but it does not, as than we only get redefinition warnings. See [3]. [1] open-mpi#6120 (comment) [2] open-mpi#6120 (comment) [3] open-mpi#6120 (comment) BWB Note: This patch was originally applied directly to v4.0.x in 73134ab, but now needs to be added to master, as we are keeping the 4.x mpi1-compat behavior. Signed-off-by: Bert Wesarg <bert.wesarg@tu-dresden.de> Signed-off-by: Brian Barrett <bbarrett@amazon.com> (cherry picked from commit b934c90)

gpaulsen requested review from hjelmn and jsquyres November 26, 2018 21:28

hjelmn approved these changes Nov 26, 2018

View reviewed changes

hjelmn added bug Target: v4.0.x labels Nov 26, 2018

gpaulsen added this to the v4.0.1 milestone Nov 26, 2018

jsquyres requested changes Nov 26, 2018

View reviewed changes

jsquyres changed the title ~~Re-add removed deprecate-only MPI-2.0 symbols~~ v4.0.x: Re-add removed deprecate-only MPI-2.0 symbols Nov 26, 2018

gpaulsen force-pushed the topic/v4.0.x/re-add-deprecated-oops branch from fee6e54 to f7d0a7d Compare November 29, 2018 12:10

jsquyres reviewed Nov 29, 2018

View reviewed changes

jsquyres requested changes Nov 29, 2018

View reviewed changes

gpaulsen force-pushed the topic/v4.0.x/re-add-deprecated-oops branch from 41a506f to b3f00bc Compare November 29, 2018 16:41

gpaulsen force-pushed the topic/v4.0.x/re-add-deprecated-oops branch from b3f00bc to ea1d2fb Compare November 29, 2018 19:36

gpaulsen force-pushed the topic/v4.0.x/re-add-deprecated-oops branch from ea1d2fb to 4f2f6ef Compare November 29, 2018 23:30

jsquyres mentioned this pull request Dec 11, 2018

Default Open MPI 4.0.0 does not provide functions deprecated in MPI-2.0 #6114

Closed

jsquyres requested changes Dec 11, 2018

View reviewed changes

bertwesarg reviewed Dec 11, 2018

View reviewed changes

gpaulsen force-pushed the topic/v4.0.x/re-add-deprecated-oops branch from 94d7479 to 4aa91e1 Compare December 26, 2018 15:46

jsquyres previously requested changes Jan 8, 2019

View reviewed changes

gpaulsen added the ⚠️ WIP-DNM! label Jan 9, 2019

hppritcha merged commit bc58e22 into open-mpi:v4.0.x Jan 10, 2019

gpaulsen mentioned this pull request Jan 10, 2019

Revert "v4.0.x: Re-add removed deprecate-only MPI-2.0 symbols" #6268

Closed

jsquyres mentioned this pull request Jan 14, 2019

master and v4.0.x: what should happen when an app compiles with a removed MPI interface? #6278

Closed

bertwesarg mentioned this pull request Mar 7, 2019

v4.0.x: Allow user to overwrite OMPI_ENABLE_MPI1_COMPAT #6469

Merged

gpaulsen deleted the topic/v4.0.x/re-add-deprecated-oops branch June 5, 2019 13:25

		@@ -437,7 +437,8 @@ libmpi_c_mpi_la_SOURCES = \
		win_wait.c


		if OMPI_ENABLE_MPI1_COMPAT

v4.0.x: Re-add removed deprecate-only MPI-2.0 symbols #6120

v4.0.x: Re-add removed deprecate-only MPI-2.0 symbols #6120

Conversation

gpaulsen commented Nov 26, 2018 • edited by jsquyres Loading

jsquyres left a comment

Choose a reason for hiding this comment

gpaulsen commented Nov 26, 2018

ggouaillardet commented Nov 27, 2018

ggouaillardet commented Nov 27, 2018

bertwesarg commented Nov 27, 2018

jsquyres commented Nov 27, 2018

gpaulsen commented Nov 29, 2018

gpaulsen commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsquyres left a comment

Choose a reason for hiding this comment

gpaulsen commented Nov 29, 2018

ibm-ompi commented Nov 29, 2018

ibm-ompi commented Nov 29, 2018

ibm-ompi commented Nov 29, 2018

ibm-ompi commented Nov 29, 2018

bertwesarg commented Nov 29, 2018

ibm-ompi commented Nov 29, 2018

gpaulsen commented Nov 29, 2018 • edited Loading

bertwesarg commented Nov 30, 2018

jsquyres left a comment

Choose a reason for hiding this comment

gpaulsen commented Dec 11, 2018

gpaulsen commented Dec 11, 2018

gpaulsen commented Dec 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hppritcha commented Jan 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hppritcha commented Jan 10, 2019 • edited Loading

gpaulsen commented Jan 10, 2019

hppritcha commented Jan 10, 2019

jsquyres commented Jan 10, 2019

gpaulsen commented Jan 10, 2019

jsquyres commented Jan 10, 2019

bertwesarg commented Mar 2, 2019

gpaulsen commented Nov 26, 2018 •

edited by jsquyres

Loading

gpaulsen commented Nov 29, 2018 •

edited

Loading

hppritcha commented Jan 10, 2019 •

edited

Loading