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

[roiedanino]

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]

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

[roiedanino]

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

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]

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]

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]

@roiedanino can you please explain how this line leads to a warning? I understand the warning for size_t but not for int. Same for void.

[roiedanino]

This one does not, but it's incorrect to return a return code as it doesn't match specifications to do so, the caller won't expect a return code and won't interpret it as such

[devreal]

We could internally return ssize_t to be able to encode errors. That would resolve the warning.

[roiedanino]

So we will need to add int* return_value parameter to all of those functions to be able to return both rc and the actual value needed to be returned to the caller by specifications

[roiedanino]

@devreal @MamziB Wdyt?

[devreal]

I thought these functions return size_t in the standard? If you replace size_t with ssize_t (signed size_t) you can return a negative error code and the index/size required by the standard. That will silence the compiler warnings I pointed out (which complained about an unsigned value being tested for negative values)

[MamziB]

So we will need to add int* return_value parameter to all of those functions to be able to return both rc and the actual value needed to be returned to the caller by specifications

I thought these functions return size_t in the standard? If you replace size_t with ssize_t (signed size_t) you can return a negative error code and the index/size required by the standard. That will silence the compiler warnings I pointed out (which complained about an unsigned value being tested for negative values)

@roiedanino I would suggest keeping the changes to a minimum for now, we can think about dealing with error codes for functions that do not return integers once we have the implementation in. I suggest printing an error/warning and breaking or returning SIZE_MAX for now.

@devreal Yes, standard mandates that this function return size_t and I think we'd better not to change the return code.

[MamziB]

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]

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

[github-actions]

Hello! The Git Commit Checker CI bot found a few problems with this PR:

2f0dd13: OSHMEM/MCA/SPML/UCX: reverted some of the changes,...

• check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

[MamziB]

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

@roiedanino did you check this?

[roiedanino]

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

@roiedanino did you check this?

Hi went through the PR, looks like the only issue I see (other than the things mentioned earlier):
OSHMEM_DECLSPEC void pshmem_team_sync(shmem_team_t team);
which is void while in the spec this function return type should be int:
int shmem_team_sync(shmem_team_t team);

[devreal]

Suggest proper scoping using do-while (like above)

Suggested change

	#define RUNTIME_SHMEM_NOT_IMPLEMENTED_API_ABORT_RET_SIZE_T() \
	RUNTIME_SHMEM_NOT_IMPLEMENTED_API_ABORT(); \
	return SIZE_MAX
	#define RUNTIME_SHMEM_NOT_IMPLEMENTED_API_ABORT_RET_SIZE_T() \
	do { \
	RUNTIME_SHMEM_NOT_IMPLEMENTED_API_ABORT(); \
	return SIZE_MAX; \
	} while (0)

[github-actions]

Hello! The Git Commit Checker CI bot found a few problems with this PR:

c4588ca: OSHMEM/RUNTIME: added do while scoping to NOT_IMPL...

• check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

[janjust]

@roiedanino thanks for the PR - please add a sign-off line in the commit