Fix opal arg parsing overhead #11827
Conversation
|
Discussion about this PR occurring on #11826. |
nivShpak
force-pushed
the
pr/opal_args_addr_sanitizer
branch
from
1717930 to
20866f0
Compare
There was a problem hiding this comment.
Per our discussion on #11826 (sorry, it's a little weird that our discussion of this PR got put on the corresponding issue -- not this PR), I'm not a fan of adding an otherwise-meaningless enum, and not explaining it at all in either the git commit or a comment in the code.
I outlined some alternatives on the issue.
|
#11828 might be a more maintainable fix. |
nivShpak
force-pushed
the
pr/opal_args_addr_sanitizer
branch
from
20866f0 to
e3a0f7b
Compare
|
@jsquyres the PR is updated as discussed in the issue |
opal/runtime/opal_params_core.c
Outdated
| @@ -89,10 +89,11 @@ static bool opal_register_util_done = false; | |||
|
|
|||
| static char *opal_var_dump_color_string = NULL; | |||
|
|
|||
| static char *opal_var_dump_color_keys[OPAL_VAR_DUMP_COLOR_KEY_COUNT] = { | |||
| static char *opal_var_dump_color_keys[OPAL_VAR_DUMP_COLOR_KEY_COUNT + 1] = { | |||
There was a problem hiding this comment.
The size should also be changed in the header file:
ompi/opal/runtime/opal_params_core.h
Line 49 in e3a0f7b
There was a problem hiding this comment.
Thanks! I was thinking of that, but forgot to check! Good catch
There was a problem hiding this comment.
@devreal should be fixed now
There was a problem hiding this comment.
@gkatev What is the relation between opal_var_dump_color_keys and opal_var_dump_color? Is a larger fix needed here?
There was a problem hiding this comment.
Ahh shoot, those are two different arrays 🤦 Not sure opal_var_dump_color needs to change, thanks for pointing that out @jsquyres
There was a problem hiding this comment.
looks like the similarity in the array names caused some confusion. opal_var_dump_color_keys is the one being fixed in this PR, and opal_var_dump_color is the one in the header file
There was a problem hiding this comment.
@gkatev What is the relation between
opal_var_dump_color_keysandopal_var_dump_color? Is a larger fix needed here?
I think the opal_var_dump_color_keys is for key names and opal_var_dump_color for values
There was a problem hiding this comment.
color values do not seem considered NULL-terminated, so maybe they don't need an extra element
There was a problem hiding this comment.
No, as far as I can tell only the keys array is affected. The keys/values arrays weren't originally meant to be NULL-terminated. Apart from fixing this bug, there might not be another benefit to making them so.
Unless I'm missing something, the only points that the two are passed to opal_argv functions are the two I mentioned in #11826 (comment).
Now, if it proves too peculiar trying to shape the arrays into NULL terminated ones, I would suggest just replacing the two offending calls. I can put together a patch tomorrow and offer it for consideration.
nivShpak
force-pushed
the
pr/opal_args_addr_sanitizer
branch
2 times, most recently
from
6d10769 to
005bf61
Compare
nivShpak
force-pushed
the
pr/opal_args_addr_sanitizer
branch
from
005bf61 to
deb3eb1
Compare
Allocate missing element for opal_var_dump_color to ensure it is NULL-terminated. Signed-off-by: nshpak <[email protected]>
Refs. open-mpi#11826 Signed-off-by: Gilles Gouaillardet <[email protected]>
nivShpak
force-pushed
the
pr/opal_args_addr_sanitizer
branch
from
deb3eb1 to
007adc2
Compare
|
@gkatev Thank you for identifying the issue and working through the conversation to identify the correct fix. I am closing this PR in favor of #11828, just so that we can close this issue and finally get the fix in. You are correctly identified and credited with finding the issue and the initial fix. |
reported issue:
Issues running OMPI with address sanitizer #11826