open-mpi / ompi

Open MPI main development repository
https://www.open-mpi.org
Other
2.07k stars 844 forks source link

OSHMEM/SHMEM: fix warnings regarding types and unused variables in shmem #12648

Open roiedanino opened 4 days ago

roiedanino commented 4 days ago

When compiling OpenShmem multiple warnings appear regarding a condition inside RUNTIME_CHECK_IMPL_RC macro as described in #12636

The solution would be changing the return types of the following functions from size_t to int:

prefix##type_name##_test_any

prefix##type_name##_test_some

prefix##type_name##_test_any_vector

prefix##type_name##_test_some_vector

prefix##type_name##_wait_until_any

prefix##type_name##_wait_until_some

prefix##type_name##_wait_until_any_vector

prefix##type_name##_wait_until_some_vector

Also removed an unused variable in void prefix##type_name##_wait_until_all_vector which caused multiple warnings as well

yosefe commented 4 days ago

@roiedanino the fixes should be according to openshmem specification, see http://openshmem.org/site/sites/default/site_files/OpenSHMEM-1.5.pdf for example: image

roiedanino commented 4 days ago

@roiedanino the fixes should be according to openshmem specification, see http://openshmem.org/site/sites/default/site_files/OpenSHMEM-1.5.pdf for example: image

So as I see it, those functions return an index and not a return code (like implied by the current implementation), what would be the way to indicate that some function is not implemented yet? logging an error message?

yosefe commented 3 days ago

So as I see it, those functions return an index and not a return code (like implied by the current implementation), what would be the way to indicate that some function is not implemented yet? logging an error message?

@manjugv @janjust @MamziB WDYT? These unimplemented functions came from #9631

devreal commented 2 days ago

From what I can see, there is no error-checking in any of this API. Simply running through a program printing warnings on each call is a bad idea. Bailing out (with a proper message) is probably the right way to go here. The cast to int is incorrect since it potentially truncates the size_t returned by confirming implementations and breaks the API.

MamziB commented 1 day ago

Thanks to @roiedanino for working on fixing the warnings. As @devreal mentioned, printing a message before returning is a good idea. Are you covering all the functions that are not implemented in this PR?

roiedanino commented 1 day ago

Thanks to @roiedanino for working on fixing the warnings. As @devreal mentioned, printing a message before returning is a good idea. Are you covering all the functions that are not implemented in this PR?

I will go through them and fix where the implementation doesn't match the specifications