Skip to content

Make C prototypes #25

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

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Make C prototypes #25

wants to merge 2 commits into from

Conversation

MakisH
Copy link
Member

@MakisH MakisH commented Mar 19, 2024

With this PR, make prototype will generate a precice-prototype.h file. This includes warnings for non-interoperable types, as, for example:

void precicef_set_vertex_ (char *meshname, float *position /* WARNING: non-interoperable KIND */ , float *vertexid /* WARNING: non-interoperable KIND */ , int meshnamelength);

Not sure where the float comes from, I don't see it in any of the involved files:

  • precice.f90
  • precice/precice/extras/bindings/fortran/
  • precice/precice/extras/bindings/c/

Applies https://fortran-lang.discourse.group/t/compilers-supporting-generation-of-c-function-prototypes/2688 suggested by @ivan-pi.

Not sure at the moment what would be the best way to use the generated prototype to detect inconsistencies. I need more time to investigate, which I currently do not have.

When complete, it should close #15.

@MakisH MakisH self-assigned this Mar 19, 2024
@ivan-pi
Copy link
Collaborator

ivan-pi commented Mar 19, 2024

There is an interface mismatch:

    subroutine precicef_set_vertex(meshName, position, vertexID, meshNameLength) &
      &  bind(c, name='precicef_set_vertex_')

      use, intrinsic :: iso_c_binding
      character(kind=c_char), dimension(*) :: meshName
      real(kind=c_double) :: coordinates(3)             !<--- this should be named position
      integer(kind=c_int) :: id                         !<--- this should be named vertexID
      integer(kind=c_int), value :: meshNameLength
    end subroutine precicef_set_vertex

Because of Fortran language rules (...) the implicit none at module level, does not apply to interface blocks. Hence, the compiler assumed the variable position is of type real, and same for vertexID, per the default implicit typing rules.

With gfortran you can use -fimplicit-none to detect these errors: https://godbolt.org/z/5brssxEz4

@ivan-pi
Copy link
Collaborator

ivan-pi commented Mar 19, 2024

To detect inconsistencies, you would need to compile preciceFortran.cpp, but replace the true header precice/preciceFortran.hpp with the gfortran-generated one. ( Technically, this header is not needed it all; it shouldn't be used by any C or C++ users. )

Afterward, if you compile with -Wbuiltin-declaration-mismatch it should flag any inconsistencies, between the Fortran bind(c) interfaces, and the extern "C" procedures.

Here's an example of what happens: https://godbolt.org/z/EMWPExjjn. It appears like you need to have extern "C" on both the declaration and definition of the procedure for this to work.

This means that either the CI in this repository should check-out the precice trunk, or the precice trunk checks out this repository, and does the consistency check.

@MakisH
Copy link
Member Author

MakisH commented Mar 19, 2024

There is an interface mismatch:

Hm... I did not think of checking the names of the variables... 😅

Because of Fortran language rules (...) the implicit none at module level, does not apply to interface blocks.

That's very useful information, thank you!

With gfortran you can use -fimplicit-none to detect these errors: https://godbolt.org/z/5brssxEz4

Very good suggestion, I added that to our Makefiles, thanks!

It appears like you need to have extern "C" on both the declaration and definition of the procedure for this to work.

I will then update the bindings in preCICE itself as well, but then this will need to wait.

(context: we are preparing releases of several components this week, I will continue with anything that needs changes in preCICE after that)

@ivan-pi
Copy link
Collaborator

ivan-pi commented Mar 22, 2024

Discussion summary:

  • preciceFortran.hpp (can be removed to prevent accidental misuse)
  • current Fortran bindings (with compiler-specific name mangling) should remain
    for backward compatibility
  • the Fortran module can be rebased on top of the C bindings
    • gfortran header <-> preciceC.hpp
  • new object-oriented bindings (F2008/F2018) (needs more work and testing)

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

Successfully merging this pull request may close these issues.

Automatically check linking of the interface
2 participants