bugfix: Setting OMPI_MPI_THREAD_LEVEL to a value different than requested crashes #13211
Conversation
| char *env; | ||
|
|
||
| ompi_hook_base_mpi_init_thread_top(argc, argv, required, provided); | ||
|
|
||
| /* Detect an incorrect thread support level, but dont report until we have the minimum | ||
| * infrastructure setup. | ||
| */ | ||
| if( (MPI_THREAD_SINGLE == required) || (MPI_THREAD_SERIALIZED == required) || |
There was a problem hiding this comment.
This is inconsistent with the way this is tested in MPI_INIT. To make it consistent, I picked the resolution with an explicit individual check per value (in MPI_INIT), this should be more in-tune with ABI values when that becomes relevant.
abouteiller
force-pushed
the
bugfix/env-thread-level-ignored
branch
3 times, most recently
from
7a81ed0 to
b71461f
Compare
`requested` in `MPI_Init_thread` would invoke the error handler, even though it is an useful override in some threaded library use cases. Signed-off-by: Aurelien Bouteiller <abouteil@amd.com>
abouteiller
force-pushed
the
bugfix/env-thread-level-ignored
branch
from
b71461f to
b481893
Compare
| @@ -46,7 +47,8 @@ PROTOTYPE INT init(INT_OUT argc, ARGV argv) | |||
|
|
|||
| if (NULL != (env = getenv("OMPI_MPI_THREAD_LEVEL"))) { | |||
| required = atoi(env); | |||
| if (required < MPI_THREAD_SINGLE || required > MPI_THREAD_MULTIPLE) { | |||
| if (required != MPI_THREAD_SINGLE && required != MPI_THREAD_FUNNELED && | |||
| required != MPI_THREAD_SERIALIZED && required != MPI_THREAD_MULTIPLE) { | |||
There was a problem hiding this comment.
Just curious: what's the need for this? Doesn't MPI specify the relationship between all the MPI_THREAD_* values such that < SINGLE and > MULTIPLE is guaranteed to be invalid?
| @@ -39,22 +40,32 @@ | |||
| PROTOTYPE ERROR_CLASS init_thread(INT_OUT argc, ARGV argv, INT required, | |||
| INT_OUT provided) | |||
| { | |||
| int err, safe_required = MPI_THREAD_SERIALIZED; | |||
| int err, err_arg_required = false, safe_required = MPI_THREAD_SERIALIZED; | |||
There was a problem hiding this comment.
If err_arg_required is going to only take on boolean values and be used as a boolean value, it should be of type bool.
| safe_required = required; | ||
| /* check for environment overrides for required thread level. If | ||
| * there is, check to see that it is a valid/supported thread level. | ||
| * if it is lower than argument required, set it anyway, program can |
There was a problem hiding this comment.
The comment says that you are checking for a lower value, but the code is not checking lower vs. higher.
I personally think it's fine to have the env variable always override (regardless of lower vs. higher); please adjust the comment.
| */ | ||
| if (NULL != (env = getenv("OMPI_MPI_THREAD_LEVEL"))) { | ||
| int env_required = atoi(env); | ||
| err_arg_required = err_arg_required || |
Setting OMPI_MPI_THREAD_LEVEL to a value different than
requestedinMPI_Init_threadwould invoke the error handler, even though it is an useful override in some threaded library use cases.