Skip to content

fortran: Fix many Fortran binding bugs #1538

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
Merged
merged 9 commits into from
Apr 14, 2016
Merged

fortran: Fix many Fortran binding bugs #1538

merged 9 commits into from
Apr 14, 2016

Conversation

kawashima-fj
Copy link
Member

@jsquyres Please review.

This PR fixes many Fortran binding issues. Please see each commit message.

@kawashima-fj
fortran: Add missing MPI_DUP_FN subroutine
10c11ff 
Though the `MPI_DUP_FN` subroutine is depricated, it is not yet removed
as of MPI-3.1.
@kawashima-fj
fortran: Change subroutine declaration order
334c63c 
Same order for `comm`, `type`, and `win`.

No code change.
@kawashima-fj
fortran: Sort declared subroutins in alphabetical order
96e93a9 
And insert necessary empty lines and remove unnecessary empty lines.

No code change.
@kawashima-fj
fortran: Fix an incorrect interface name
b6cb0bc
@kawashima-fj
fortran: Add missing PMPI interfaces
f3b9a49
@kawashima-fj
fortran: Add missing interfaces
6f09d53
@kawashima-fj
fortran: Add missing interfaces (part 2)
5d32a60
@kawashima-fj
fortran: Fix incorrect weak symbol names
d48c852
@kawashima-fj
fortran: Add missing (P)MPI_Alloc_mem_cptr_{f,f08} symbols
17d32ac 
This commit adds the following symbols

  MPI_Alloc_mem_cptr_f
  MPI_Alloc_mem_cptr_f08
  PMPI_Alloc_mem_cptr_f
  PMPI_Alloc_mem_cptr_f08

These are implemented in the same way as other `_cptr` routines.
@kawashima-fj kawashima-fj added this to the v2.0.0 milestone Apr 12, 2016
jsquyres
jsquyres reviewed Apr 14, 2016
View reviewed changes
ompi/mpi/fortran/mpif-h/alloc_mem_f.c
#pragma weak MPI_ALLOC_MEM_CPTR = ompi_alloc_mem_f
#pragma weak mpi_alloc_mem_cptr = ompi_alloc_mem_f
#pragma weak mpi_alloc_mem_cptr_ = ompi_alloc_mem_f
#pragma weak mpi_alloc_mem_cptr__ = ompi_alloc_mem_f

#pragma weak MPI_Alloc_mem_f = ompi_alloc_mem_f
#pragma weak MPI_Alloc_mem_f08 = ompi_alloc_mem_f

#pragma weak MPI_Alloc_mem_cptr_f = ompi_alloc_mem_f
#pragma weak MPI_Alloc_mem_cptr_f08 = ompi_alloc_mem_f
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor nit: in the above section, it goes:

  • pragmas for alloc_mem
  • pragmas for alloc_mem_f/_f08
  • ...and then all the cptr pragmas

But in this section, it's a different order:

  • pragmas for alloc_mem
  • pragmas for cptr
  • pragmas for alloc_mem_f/f08
  • more pragmas for cptr

Can you make the order between these two sections be the same?

(again, I realize this is a super-picky nit -- thank you for your patience!)

@jsquyres
Copy link
Member

@kawashima-fj Thank you for cleaning up all these issues. Can you please PR this stuff to v2.0.0?

This PR shows how badly we need to overhaul our Fortran bindings 😦

Specifically: our Fortran bindings are functionally correct, but whenever a dev adds a new MPI interface, it's super easy to leave out one of the X (where X=large) necessary Fortran implementations of that MPI interface.

A long time ago, I did the first steps of generating all the Fortran bindings at configure/make time. Slightly less than a long time ago, I gave that code to @ggouaillardet -- @ggouaillardet have you been able to make any progress with it? Is it something you could possibly collaborate with @kawashima-fj to make some progress on, perchance?

@jsquyres jsquyres merged commit 4566286 into open-mpi:master Apr 14, 2016
@kawashima-fj
Copy link
Member Author

I pushed a commit to fix the order issue. Thanks.
I'll make a PR to v2.0.0 soon.

@jsquyres @ggouaillardet
I looked only mpif-h and use-mpi-ignore-tkr this time.
use-mpi-f08 etc. may have similar issues possibly.
I also think auto-generating is desired.
Yes, possibly I can collaborate.

@kawashima-fj kawashima-fj deleted the pr/fortran-binding-fix branch April 15, 2016 06:25
@jsquyres
Copy link
Member

@kawashima-fj I think something was wrong with this PR. Last night, I got a build failure with MTT:

$ ./configure "CFLAGS=-g -pipe" --enable-picky --enable-debug --enable-mpirun-prefix-by-default --disable-dlopen --disable-weak-symbols

...lots of output...

$ make -j 32

... lots of output...

  ^profile/.libs/libmpi_mpifh_pmpi.a(palloc_mem_f.o): In function `ompi_alloc_mem_cptr_f':
/home/mpiteam/scratches/community/2016-04-20cron/Wniq/mpi-install/rAXy/src/openmpi-v1.10.2-154-gfe1dc36/ompi/mpi/fortran/mpif-h/profile/palloc_mem_f.c:115:
multiple definition of `ompi_alloc_mem_cptr_f'
.libs/alloc_mem_f.o:/home/mpiteam/scratches/community/2016-04-20cron/Wniq/mpi-install/rAXy/src/openmpi-v1.10.2-154-gfe1dc36/ompi/mpi/fortran/mpif-h/alloc_mem_f.c:115:
first defined herecollect2: error: ld returned 1 exit status
make[3]: *** [libmpi_mpifh.la] Error 1
make[2]: *** [all-recursive] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all-recursive] Error 1

It looks like something is wrong in alloc_mem_f.f.

Can you investigate?

@jsquyres
Copy link
Member

@kawashima-fj Here's the link to the MTT failure: https://mtt.open-mpi.org/index.php?do_redir=2292

@kawashima-fj
Copy link
Member Author

I'll take a look.

@kawashima-fj kawashima-fj mentioned this pull request Apr 22, 2016
Merged
@kawashima-fj
Copy link
Member Author

@jsquyres I opened a PR to fix this issue. Please review open-mpi/ompi-release#1091.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@kawashima-fj @jsquyres