Page MenuHomePhabricator

[OpenMP][OMPT] Unnecessary execution of the while loop inside __ompt_get_task_info_internal
Needs ReviewPublic

Authored by vladaindjic on Sep 30 2021, 4:09 AM.

Details

Summary

__ompt_get_task_info_internal now recognizes if the task at the
requested ancestor_level doesn’t exist. This is done by recognizing
whether all tasks have been exhausted (“taskdata” doesn’t exist) during
the iteration over the hierarchy of active tasks. If so, there’s no need
to continue executing the while loop. Break the loop and return 0 code
instead.

This patch prevents the SEGFAULT that may arise if the the loop unnecessarily
continues instead (the code before the patch). Assume that the
__ompt_get_task_info_internal is called with the argument “ancestor_level == 2”,
while the initial master thread is executing the initial task.
After the while loop finishes, both “prev_lwt” and “prev_team” are equal to NULL.
If the called function should determine the “thread_num”, it will do that by
executing the assignment “*thread_num = prev_team->t.t_master_tid”.
Since “prev_team” is NULL, dereferencing it leads to a SEGFAULT.

Also, this patch may be considered as a simple performance optimization.
Assume that the hierarchy of active tasks contains two tasks.
If the __ompt_get_task_info_internal is called with the argument
“ancestor_level==1000”, there’s no need to execute more that two iterations
of the while before detecting that the task at the requested “ancestor_level”
doesn’t exist.

This patch provides the simple test cases that validates the behaviour of
the __ompt_get_task_info_internal function in case when the task doesn’t exist
at the requested “ancestor_level”.

Diff Detail

Event Timeline

vladaindjic created this revision.Sep 30 2021, 4:09 AM
vladaindjic requested review of this revision.Sep 30 2021, 4:09 AM
Herald added a project: Restricted Project. · View Herald Transcript
vladaindjic updated this revision to Diff 376465.EditedOct 1 2021, 3:44 AM

@protze.joachim According to the standard specification which claims the following: “If no task region exists at the specified ancestor level or the information is unavailable then the values of variables passed by reference to the entry point are undefined when ompt_get_task_info returns.”, it is not safe to use “task_data” and “parallel_data” before previously asserting that “ret == 2” inside ompt-multiplex.h test sections.

This check ("ret == 2") prevents failing the following test cases:

  1. multiplex.custom_data_storage::custom_data_storage.c
  2. OMPT multiplex.print::print.c

Please update the diff with context (-U9999)

The diffs have been updated through arc diff tool instead of web form.

@protze.joachim I was using the following command "arc diff --update <revision_id>".
Is that enought to supply the diff context?