Skip to content
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

Parameter Conflict: PMIx_Get vs PMIx_Get_nb #420

Closed
rhc54 opened this issue Aug 18, 2022 · 3 comments · Fixed by #488, #495 or #510
Closed

Parameter Conflict: PMIx_Get vs PMIx_Get_nb #420

rhc54 opened this issue Aug 18, 2022 · 3 comments · Fixed by #488, #495 or #510
Assignees

Comments

@rhc54
Copy link
Member

rhc54 commented Aug 18, 2022

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:

  • you pass a char* string to PMIx_Get, but not if you pass that same string to PMIx_Get_nb.
  • you pass a pmix_key_t to PMIx_Get_nb, but not if you pass that same key to PMIx_Get

So which is it supposed to be?

@rhc54
Copy link
Member Author

rhc54 commented Aug 18, 2022

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
Copy link
Member Author

rhc54 commented Aug 18, 2022

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.

@jjhursey jjhursey added this to the PMIx v4.2 Standard milestone Jan 13, 2023
@abouteiller abouteiller self-assigned this May 11, 2023
@abouteiller abouteiller mentioned this issue Sep 14, 2023
3 tasks
@abouteiller
Copy link
Contributor

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

@abouteiller abouteiller mentioned this issue Apr 12, 2024
7 tasks
abouteiller added a commit to abouteiller/pmix-standard that referenced this issue Apr 29, 2024
@abouteiller abouteiller linked a pull request Apr 30, 2024 that will close this issue
abouteiller added a commit to abouteiller/pmix-standard that referenced this issue Apr 30, 2024
abouteiller added a commit to abouteiller/pmix-standard that referenced this issue Apr 30, 2024
abouteiller added a commit to abouteiller/pmix-standard that referenced this issue May 1, 2024
abouteiller added a commit that referenced this issue May 27, 2024
Correct fix for #420: pmix_key_t param in pmix_get
abouteiller added a commit to abouteiller/pmix-standard that referenced this issue Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment