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

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 · 10 comments · Fixed by #12829
Closed

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

bcmundim opened this issue Jul 25, 2024 · 10 comments · Fixed by #12829
Labels
question Target: main

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!
@bcmundim bcmundim mentioned this issue Jul 25, 2024
Open
@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).

@github-actions GitHub Actions
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
@github-actions GitHub Actions
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 devreal mentioned this issue Sep 11, 2024
Closed
@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.

@jsquyres jsquyres reopened this Sep 28, 2024
jsquyres added a commit to jsquyres/ompi that referenced this issue Sep 28, 2024
@jsquyres
Update opal_portable_platform_real.h from upstream gasnet
50c05da 
Pull update from
https://bitbucket.org/berkeleylab/gasnet/src/stable/other/gasnet_portable_platform.h
at git hash a90966674.  This update brings in two things:

1. Remove the need for the #define workarounds in
   opal/include/opal/opal_portable_platform.h
2. Remove all //-style comments, making this file safe to use with
   C89-style applications (per
   open-mpi#12710).

Many thanks to the gasnet team for including these changes in their
upstream repo.

Note that we still need the #ifndef SIZEOF_VOID_P protection in mpi.h.
This commit also makes a minor update in mpi.h[.in] to prefix an
OMPI-specific #define with "OMPI_".  No one has ever complained about
this un-prefixed macro, but prefixing it it felt like the Right Thing
to do while mucking around with other portable_platform stuff.

Signed-off-by: Jeff Squyres <[email protected]>
@jsquyres jsquyres mentioned this issue Sep 28, 2024
Merged
@jsquyres
Copy link
Member

jsquyres commented Sep 28, 2024 •
edited
Loading

I notice that we accidentally closed this issue before resolving it.

I worked with the gasnet team and got the changes put in upstream (https://bitbucket.org/berkeleylab/gasnet/pull-requests/543). I then made #12829 as an alternative to @devreal's PR. Let's figure out which of our 2 PR's we should merge here (and cherry pick over to the v5.0.x series).

jsquyres added a commit to jsquyres/ompi that referenced this issue Sep 29, 2024
@jsquyres
Update opal_portable_platform_real.h from upstream gasnet
d5e2855 
Pull update from
https://bitbucket.org/berkeleylab/gasnet/src/stable/other/gasnet_portable_platform.h
at git hash a90966674.  This update brings in two things:

1. Remove the need for the #define workarounds in
   opal/include/opal/opal_portable_platform.h
2. Remove all //-style comments, making this file safe to use with
   C89-style applications (per
   open-mpi#12710).

Many thanks to the gasnet team for including these changes in their
upstream repo.

Note that we still need the #ifndef SIZEOF_VOID_P protection in mpi.h.
This commit also makes a minor update in mpi.h[.in] to prefix an
OMPI-specific #define with "OMPI_".  No one has ever complained about
this un-prefixed macro, but prefixing it it felt like the Right Thing
to do while mucking around with other portable_platform stuff.

Signed-off-by: Jeff Squyres <[email protected]>
(cherry picked from commit 50c05da)
@jsquyres jsquyres mentioned this issue Sep 29, 2024
Merged
@jsquyres
Copy link
Member

v5.0.x PR is pending.

Probably won't bring this back to v4.1.x -- I did a cherry-pick and it didn't immediately apply. I didn't look into the conflict details, but since this issue was about v5.0.x, unless there's a specific reason to bring it to v4.1.x, I think bringing the update to main and v5.0.x is probably good enough.

@jsquyres jsquyres removed the Stale label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question Target: main
Projects
None yet
Milestone
No milestone
4 participants
@jsquyres @bcmundim @devreal @wenduwan