cea-hpc / wi4mpi

Wrapper interface for MPI
BSD 3-Clause "New" or "Revised" License
80 stars 15 forks source link

Override functions not compiled #42

Closed spoutn1k closed 11 months ago

spoutn1k commented 1 year ago

Hey everyone !

I was looking at overriding some functions to circumvent missing symbols in OpenMPI libraries.

Following the advice of @adrien-cotte, I added my functions in common/override.c in a ifdef directive and added the corresponding definition in override.h.

I ran across two details I cannot explain:

Overrides in preload mode

The preload/gen/mpi_translation_c.c file does not #include "override.h". This means that although it contains ifndef XXX_OVERRIDE directives, they all equate to true, not overriding anything. This is fine if by design, but the CMakeLists.txt recipe for preload includes override.c:

https://github.com/cea-hpc/wi4mpi/blob/1c5ea267002df7875153044b76172c724c6f8f98/src/CMakeLists.txt#L37-L44

On the other hand, interface/gen/mpi_translation_c.c does include the header on line 25.

If this is a mechanism to generate the code once with python and have functions overridden only when in interface mode, why add the overridden function bodies to the preload shared object ? Or it this a typo and preload should also support overridden functions ?

Overrides missing from compilation

When looking at the code in override.c, I noticed the in_w is never defined as the extern __thread int it should be. There is no header included either, and this made me suspicious, so I added a #pragma message "COMPILING XXX_OVERRIDE" in the functions overridden, just after the #ifdef. None of those fired and the following returned nothing:

make clean
make 2>&1 | grep COMPILING

This leads me to think that the functions in override.c are not used by the project, unless I am missing something.

spoutn1k commented 1 year ago

I did some more digging and by renaming OVERRRIDE to OVERRIDE, compiling the project produces binaries with undefined symbols for the overriden functions:

$ nm -u build/install/lib_OMPI/libwi4mpi_OMPI.so | grep A_MPI
                 U A_MPI_Testall
                 U A_MPI_Testany
                 U A_MPI_Testsome
                 U A_MPI_Waitall
                 U A_MPI_Waitany
                 U A_MPI_Waitsome

Let me know if this is the intended behaviour (if the typo was meant to be there).

adrien-cotte commented 1 year ago

For me it's clearly a typo in preload mode. preload/gen/mpi_translation_c.c should include "override.h" as interface/gen/mpi_translation_c.c.

Agree too with extern __thread int in_w.

Your renaming test proves that overriding feature is not finalized... Do you want to contribute and fix it by yourself? You seem on the right way.

Btw, great job for an external developer :+1:

spoutn1k commented 1 year ago

Thank you ! It is a really interesting project to bang your head on. I have a fix and will create a PR for it !