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

Non-compliant C90 comments at mpi.h file leads to compilation failure for codes compiled with gcc -ansi or -std=c90 flags #12710

Closed
bcmundim opened this issue Jul 25, 2024 · 8 comments · May be fixed by #12811

Comments

@bcmundim
Copy link

Background information

What version of Open MPI are you using? (e.g., v4.1.6, v5.0.1, git branch name and hash, etc.)

v5.0.2

Describe how Open MPI was installed (e.g., from a source/distribution tarball, from a git clone, from an operating system distribution package, etc.)

From a source/distribution tarball

Please describe the system on which you are running

  • Operating system/version: Linux CentOS/7.9
  • Computer hardware: Lenovo SR630 server
  • Network type: Infiniband

Details of the problem

While trying to compile one of Visit software dependencies, IceT, the compilation was interrupted with an error while parsing OpenMPI mpi.h file included in one of their files. I tracked the problem down to non C90 compliant comments in the mpi.h. IceT uses the gcc -ansi flag and that triggers this bug. You can easily reproduce it by trying to compile the hello_c example you ship with the OpenMPI source code:

me@somewhere:~/src/openmpi/openmpi-5.0.5/examples$ CFLAGS=-ansi make hello_c
mpicc -ansi -g  hello_c.c  -o hello_c
In file included from /path/to/software/2022a/opt/gcc-11.3.0/openmpi/5.0.2+ucx-1.15.0/include/mpi.h:285,
                 from hello_c.c:11:
/path/to/software/2022a/opt/gcc-11.3.0/openmpi/5.0.2+ucx-1.15.0/include/mpi_portable_platform.h:294:30: error: operator '/' has no right operand
  294 | #elif defined(__NVCOMPILER) // Must occur prior to PGI and CLANG
      |                              ^
make: *** [hello_c] Error 1

This could be tracked down further at two comments on the header file ./opal/include/opal/opal_portable_platform_real.h at lines 294 and 593. I am not sure if there are other places that eventually will end up in the public facing mpi.h file though. Would it be possible to replace those comments with the C90 ones? At least on the public facing header files such as mpi.h?

Thanks!

@jsquyres
Copy link
Member

@bcmundim Sorry for the delay; this question slipped by me.

Open MPI has required a C99 compiler for many years. It's a bit surprising that a IceT is requiring a 30+ year old C standard, especially if it requires mpi.h. Can IceT really not be compiled with a C99 or newer C compiler (i.e., something that is only 25+ years old)? If so, that seems like the real issue.

@devreal
Copy link
Contributor

devreal commented Aug 13, 2024

The MPI standard is silent about specific C versions and we intentionally have not used any post-C90 features so it could be argued that the header mpi.h should work with any C standard (Open MPI required C99 for compiling its code, which is different from application codes).

@jsquyres
Copy link
Member

@devreal True, but the MPI standard is silent about compiler support, too -- and Open MPI only supports a specific set of compilers.

FWIW: It looks like the portable_platform file was last imported from Gasnet in 2021. I won't object if someone wants to post a PR to update these things, but it seems like codes that require compiling with -ansi (i.e., C89) are making choices that potentially limit their applicability.

@jsquyres
Copy link
Member

More specifically: since we import that portable_platform.h from Gasnet, we would really like to see the fix put upstream so that we can pull it down here again (including the fix). Otherwise, we'll have to keep track of local edits the next time we pull this down (i.e., we'll forget).

Copy link

It looks like this issue is expecting a response, but hasn't gotten one yet. If there are no responses in the next 2 weeks, we'll assume that the issue has been abandoned and will close it.

@github-actions github-actions bot added the Stale label Aug 27, 2024
Copy link

Per the above comment, it has been a month with no reply on this issue. It looks like this issue has been abandoned.

I'm going to close this issue. If I'm wrong and this issue is not abandoned, please feel free to re-open it. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 10, 2024
@bcmundim
Copy link
Author

@jsquyres What is gasnet? Why is this issue being abandoned when the fix is really simple? If there is no plan to change this could you please at least announce the lack of openmpi support for C90 compiler options? Unfortunately I don't have control over IceT. My workaround was to use mpich instead of openmpi. But I would like to avoid that workaround as much as possible. Thanks.

@devreal
Copy link
Contributor

devreal commented Sep 11, 2024

@bcmundim Please see #12811 for a PR changing the header. I don't think we pull this is from gasnet regularly so we can patch it ourselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants