pmix / pmix-standard

PMIx Standard Document
https://pmix.org
Other
22 stars 22 forks source link

Parameter Conflict: PMIx_Get vs PMIx_Get_nb #420

Open rhc54 opened 1 year ago

rhc54 commented 1 year ago

PMIx_Get has a signature that includes const pmix_key_t key.

PMIx_Get_nb has a signature that includes const char key[] for the same argument.

The problem is that these conflict in the eyes of today's picky compilers such as gcc12, which will generate a warning if:

So which is it supposed to be?

rhc54 commented 1 year ago

Further info: one concern I have with the PMIx_Get signature's use of pmix_key_t is that you cannot pass a standard attribute in the PMIx_Get call without generating a warning. In other words, a call like

PMIx_Get(&proc, PMIX_JOB_SIZE, NULL, 0, &val)

will return a warning:

error: 'PMIx_Get' reading 512 bytes from a region of size 14 [-Werror=stringop-overread]

because PMIx_Get uses pmix_key_t in its parameter list (which is 512 bytes in size) while PMIX_JOB_SIZE is a string of 14 characters. This means that a programmer must use PMIX_LOAD_KEY to place the attribute in a pmix_key_t struct before using it - which is a little tiresome.

On the other hand, use of pmix_key_t does limit the size of the input string, which has some advantages.

Does anyone have thoughts on this? The change is going to cause pain either way we go. I believe PMIx_Get is used far more than PMIx_Get_nb, so in some ways using the PMIx_Get version is likely to create the smaller amount of code change, if that is a consideration.

rhc54 commented 1 year ago

Dug a little deeper. It seems that OpenPMIx shows both APIs as const char key[], which is why we weren't seeing this before (there are now a couple of internal inconsistencies as we got confused by the Standard). So this largely is a case of correcting the Standard, and I'd advise going with the const char key[] option as this has the least impact on current users.

abouteiller commented 2 months ago

488 implemented this errata backward, we need to rework this for both v4.2 and v5.0