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.

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

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

Closed
jsquyres opened this issue Jan 14, 2019 · 10 comments

Comments

@jsquyres
Copy link
Member

From #6120, @gpaulsen noticed that both v4.0.x and master only use the compiler "deprecated" attribute to mark removed MPI functions. Take the following example program:

#include <mpi.h>

int main(int argc, char** argv[])
{
    int a;
    void *address;

    MPI_Address(&a, &address);
    return 0;
}

Case 1: master, with MPI-1 compatibility disabled.

  • Functions like MPI_Address are marked with the __deprecated__ compiler attribute.
  • Functions like MPI_Address are not included in the MPI library.

Meaning:

  • The deprecating warning will be emitted
    • Including the word "deprecated", which may be a bit confusing (because MPI_Address is not deprecated -- it's removed)
  • The example program will compile successfully
  • But it will fail to link

I think it is anti-social to allow the compile to succeed when we know that the link will fail. I think we should somehow make the compile fail (see below).

Case 2: v4.0.x, with MPI-1 compatibility disabled

  • Functions like MPI_Address are marked with the __deprecated__ compiler attribute.
  • Functions like MPI_Address are included in the MPI library.

Meaning:

  • The deprecating warning will be emitted
    • Including the word "deprecated", which may be a bit confusing (because MPI_Address is not deprecated -- it's removed)
  • The example program will compile successfully
  • The example program will also link successfully.

I'm not sure that this is the message that we want to convey to the community (i.e., that it's still ok to use the removed symbols -- i.e., if it's just a compiler warning, people will ignore it). I thought our intent was to raise the pain level (just a little) so that the greater MPI application community became aware of this issue. As such, my gut reaction is that we should make the compile fail.

Applications that were compiled against v3.0.x or v3.1.x (or even v4.0.x) with MPI_Address (etc.) will still work because MPI_Address (etc.) are still in the MPI library. -- which is desirable.

How to make the compile fail?

If we elect to make the compile fail, it's an open question on how to do this.

  1. The __deprecated___ function attribute is just a warning.
  2. @gpaulsen and I played with several ways to try to make a compile fail, but yet also still give a helpful error message (e.g., "Stop using MPI_Address. Use MPI_Get_address instead" yadda yadda yadda). This is surprisingly difficult. The only real way to make this happen is to introduce a syntax error somehow (e.g., via #define MPI_Address(a,b) some kind of syntax error containing the helpful message, but that's... tricky).

Are these cases really what we want?

  1. On master: I think it's anti-social to allow the compile to succeed when we know the link will fail.
  2. On v4.0.x: it's different there, because we know the link will succeed. ...but do we really want the compile to succeed?
@jsquyres jsquyres added this to the v4.0.1 milestone Jan 14, 2019
@jsquyres jsquyres changed the title master and v4.0.x: what should happen when an app compiles a removed MPI interface? master and v4.0.x: what should happen when an app compiles with a removed MPI interface? Jan 14, 2019
@jsquyres
Copy link
Member Author

jsquyres commented Jan 15, 2019

We talked about this on the 2019-01-15 webex. Here's what we decided...

There are 3 cases:

  1. User wants MPI-1 compatibility.
    1. MPI_Address (and friends) are declared in mpi.h with the deprecation notice
  2. User does not want MPI-1 compatibility, and has a C11-capable compiler
    1. Declare an MPI_Address (etc.) macro in mpi.h like this (which will cause a compile-time error):
#if __STDC_VERSION__ >= 201101L
#define MPI_Address(a, b) _Static_assert (0, "MPI_Address was removed in MPI-3.0. Use MPI_Get_address instead")
#endif
  1. User does not want MPI-1 compatibility, and does not have a C11-capable compiler
    1. Do not declare MPI_Address (etc.) in mpi.h at all.
    2. 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).
    3. 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).
    4. 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).

@ggouaillardet
Copy link
Contributor

in the third case, how do you expect the user can compile his code if MPI_Address is not declared in mpi.h ? C99 does not allow missing prototypes. should 3.i be "declare MPI_Address with the deprecation warning on v4.0.x" ?

@jsquyres
Copy link
Member Author

In the 3rd case (where MPI_Address is not in mpi.h), the user's application will either compile or it won't -- depending on the compiler and whether un-prototyped functions are considered an error or not (remember that we don't guarantee that the same compiler is used to build Open MPI as is used to build the user's MPI application). With v4.0.x, the application will even link (but it won't with master/v5.0.x).

For example:

$ mpicc --showme
gcc -I/home/jsquyres/bogus/include -pthread -Wl,-rpath -Wl,/home/jsquyres/bogus/lib -Wl,--enable-new-dtags -L/home/jsquyres/bogus/lib -lmpi
$ mpicc --version
gcc (GCC) 8.2.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

# address.c is the example code from the beginning of this issue
$ mpicc -c address.c
address.c: In function ‘main’:
address.c:8:5: warning: implicit declaration of function ‘MPI_Address’; did you mean ‘MPI_Test’? [-Wimplicit-function-declaration]
     MPI_Address(&a, &address);
     ^~~~~~~~~~~
     MPI_Test
$ mpicc address.o -o address
$

@bertwesarg
Copy link
Member

There are two more options:

  1. Use the error attribute. It gives this message:
void MPI_Address(void*,void*)
__attribute__((error ("MPI_Address was removed from this MPI, use MPI_Get_address")));
error: call to ‘MPI_Address’ declared with attribute error: MPI_Address was removed from this MPI, use MPI_Get_address
     MPI_Address(&a, &address);
     ^~~~~~~~~~~~~~~~~~~~~~~~~
  1. Declare the removed function as a variable:
extern int MPI_Address;

But the error message is not that nice:

error: called object ‘MPI_Address’ is not a function or function pointer
     MPI_Address(&a, &address);
     ^~~~~~~~~~~
note: declared here
 extern int MPI_Address;
            ^~~~~~~~~~~

@jsquyres
Copy link
Member Author

@bertwesarg Oh neat -- I didn't know about the error attribute. I see this as far back as gcc 4.3.6 (see https://gcc.gnu.org/onlinedocs/gcc-4.3.6/gcc/Function-Attributes.html#Function-Attributes).

Do you know how portable is this with other compilers?

@gpaulsen This could be a 2nd option:

  1. If C11 static assert is available, use that.
  2. If the error attribute is available, use that.
  3. Otherwise, just don't declare the function at all.

@bertwesarg
Copy link
Member

@bertwesarg Oh neat -- I didn't know about the error attribute. I see this as far back as gcc 4.3.6 (see https://gcc.gnu.org/onlinedocs/gcc-4.3.6/gcc/Function-Attributes.html#Function-Attributes).

Do you know how portable is this with other compilers?

Neither PGI nor Clang accept it. No Intel at hand right now.

gpaulsen added a commit to gpaulsen/ompi that referenced this issue Jan 23, 2019
  Addresses open-mpi#6278

  In v4.0.x we intended that if the user did not configure with
  --enable-mpi1-compatibility, that they would get a Fatal error,
  even though we are building those removed symbols into libmpi
  (to support MPI applications compiled and linked with v3.x)

  This commit changes the deprectated function attribute to instead
  use the error function attribute supported by many (but not all)
  compilers.  If the compiler doesn't support this attribute they
  will just get an error trying to use a function that wasn't
  declared.

Signed-off-by: Geoffrey Paulsen <[email protected]>
gpaulsen added a commit to gpaulsen/ompi that referenced this issue Jan 29, 2019
  Addresses open-mpi#6278

  In v4.0.x we intended that if the user did not configure with
  --enable-mpi1-compatibility, that they would get a Fatal error,
  even though we are building those removed symbols into libmpi.
  (to support MPI applications compiled and linked with v3.x)

  This commit changes removed MPI APIs to try to use either the
  error function attribute (if the compiler supports it) or to use
  use the C11 Static Assert (also if supported by the compiler)
  If the compiler doesn't offer this support, the user will instead
  just get an error trying to use a function that wasn't declared.

Signed-off-by: Geoffrey Paulsen <[email protected]>
@gpaulsen
Copy link
Member

I've posted what I think are "final" solutions to master PR #6359 and v4.0.1 PR #6358.

@jsquyres
Copy link
Member Author

jsquyres pushed a commit to gpaulsen/ompi that referenced this issue Feb 27, 2019
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]>
jsquyres pushed a commit to gpaulsen/ompi that referenced this issue Feb 27, 2019
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]>
jsquyres pushed a commit to gpaulsen/ompi that referenced this issue Feb 27, 2019
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 3d76b23)
jsquyres pushed a commit to gpaulsen/ompi that referenced this issue Feb 27, 2019
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]>
jsquyres pushed a commit to gpaulsen/ompi that referenced this issue Feb 27, 2019
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)
@gpaulsen
Copy link
Member

Now that these PRs have been merged, can we close this issue?

@jsquyres
Copy link
Member Author

🎉

markalle pushed a commit to markalle/ompi that referenced this issue Sep 12, 2020
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants