If the DSL defines a task_snprintf function, use that function.#603
Merged
therault merged 1 commit intoICLDisco:masterfrom Dec 1, 2023
Merged
Conversation
This feature was lost in a previous merge accident, but it should have been enabled via PR ICLDisco#409
bosilca
approved these changes
Dec 1, 2023
therault
added a commit
to therault/parsec
that referenced
this pull request
Dec 7, 2023
When there are no parameters to a task (as for some sample_tasks() in dtd_test_task_generation), using `GET_HEAD_OF_PARAM_LIST(dtd_ask)` does not return NULL, but there are no parameters and we should not cycle over them. This bug started to manifest after using the DSL specific task_snprintf for all task_snprintfs in PR ICLDisco#603. I'm reading tc->nb_flows in a separate variable at the beginning of the code, so the compiler should be smart enough to store that in a register or realize that if nb_flows is not 0 the first time it does the loop, it will never become 0 and it can only test current_param != NULL
therault
added a commit
to therault/parsec
that referenced
this pull request
Dec 7, 2023
It's possible that parsec_dtd_task_snprintf is called when the `->next` field of the parameters copied at the end of the task are invalid. One should not access them using `->next` as we do when parsing the user input, but knowing they are stored as in an array at the end of the task, starting at byte `GET_HEAD_OF_PARAM_LIST(dtd_task)` This bug started to manifest after using the DSL specific task_snprintf for all task_snprintfs in PR ICLDisco#603. This code also correctly manages the case when count_of_params is 0, but there are no tests like that today.
bosilca
added a commit
to bosilca/parsec
that referenced
this pull request
Dec 8, 2023
And a proper fix for the dtd_task_snprintf. The DTA task's parameters were created as an array but also maintained as a linked list. There was no need for both access modes, so array stayed while linked list dissapeared. Fix all the the uses of the DTD task params to be consistent. This is a more comprehensive replacement for ICLDisco#607, that addresses the real cause of the problem with DTD task_snprintf reported by @abouteiller. Fixes ICLDisco#603. Signed-off-by: George Bosilca <bosilca@icl.utk.edu>
bosilca
added a commit
to bosilca/parsec
that referenced
this pull request
Dec 8, 2023
And a proper fix for the dtd_task_snprintf. The DTA task's parameters were created as an array but also maintained as a linked list. There was no need for both access modes, so array stayed while linked list dissapeared. Fix all the the uses of the DTD task params to be consistent. This is a more comprehensive replacement for ICLDisco#607, that addresses the real cause of the problem with DTD task_snprintf reported by @abouteiller. Fixes ICLDisco#603. Signed-off-by: George Bosilca <bosilca@icl.utk.edu>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This feature was lost in a previous merge accident, but it should have been enabled via PR #409