From 22d02b63ac3bc6113e532ec6069afa3dd70a417d Mon Sep 17 00:00:00 2001 From: Joachim Jenke Date: Tue, 29 Oct 2024 20:13:00 +0100 Subject: [PATCH] [OpenMP][OMPT][OMPD] Fix frame flags for OpenMP tool APIs In several cases the flags entries in ompt_frame_t are not initialized. According to @jdelsign the address provided as reenter and exit address is the canonical frame address (cfa) rather than a "framepointer". This patch makes sure that the flags is always initialized and changes the value from ompt_frame_framepointer to ompt_frame_cfa. The assertion in the tests makes sure that the flags are always set, when a tool (callback.h in this case) looks at the value. Fixes #89058 --- openmp/runtime/src/kmp_tasking.cpp | 22 +++------------------- openmp/runtime/src/ompt-general.cpp | 1 + openmp/runtime/src/ompt-internal.h | 2 ++ openmp/runtime/src/ompt-specific.cpp | 2 ++ openmp/runtime/src/ompt-specific.h | 15 +++++++++++++++ openmp/runtime/test/ompt/callback.h | 22 +++++++++++++++++++++- 6 files changed, 44 insertions(+), 20 deletions(-) diff --git a/openmp/runtime/src/kmp_tasking.cpp b/openmp/runtime/src/kmp_tasking.cpp index 932799e133b45..3e229b517cfcd 100644 --- a/openmp/runtime/src/kmp_tasking.cpp +++ b/openmp/runtime/src/kmp_tasking.cpp @@ -714,22 +714,6 @@ static void __kmp_task_start(kmp_int32 gtid, kmp_task_t *task, #if OMPT_SUPPORT //------------------------------------------------------------------------------ -// __ompt_task_init: -// Initialize OMPT fields maintained by a task. This will only be called after -// ompt_start_tool, so we already know whether ompt is enabled or not. - -static inline void __ompt_task_init(kmp_taskdata_t *task, int tid) { - // The calls to __ompt_task_init already have the ompt_enabled condition. - task->ompt_task_info.task_data.value = 0; - task->ompt_task_info.frame.exit_frame = ompt_data_none; - task->ompt_task_info.frame.enter_frame = ompt_data_none; - task->ompt_task_info.frame.exit_frame_flags = - ompt_frame_runtime | ompt_frame_framepointer; - task->ompt_task_info.frame.enter_frame_flags = - ompt_frame_runtime | ompt_frame_framepointer; - task->ompt_task_info.dispatch_chunk.start = 0; - task->ompt_task_info.dispatch_chunk.iterations = 0; -} // __ompt_task_start: // Build and trigger task-begin event @@ -804,7 +788,7 @@ static void __kmpc_omp_task_begin_if0_template(ident_t *loc_ref, kmp_int32 gtid, taskdata->ompt_task_info.frame.exit_frame.ptr = frame_address; current_task->ompt_task_info.frame.enter_frame_flags = taskdata->ompt_task_info.frame.exit_frame_flags = - ompt_frame_application | ompt_frame_framepointer; + OMPT_FRAME_FLAGS_APP; } if (ompt_enabled.ompt_callback_task_create) { ompt_task_info_t *parent_info = &(current_task->ompt_task_info); @@ -1268,8 +1252,7 @@ static void __kmpc_omp_task_complete_if0_template(ident_t *loc_ref, ompt_frame_t *ompt_frame; __ompt_get_task_info_internal(0, NULL, NULL, &ompt_frame, NULL, NULL); ompt_frame->enter_frame = ompt_data_none; - ompt_frame->enter_frame_flags = - ompt_frame_runtime | ompt_frame_framepointer; + ompt_frame->enter_frame_flags = OMPT_FRAME_FLAGS_RUNTIME; } #endif @@ -2010,6 +1993,7 @@ kmp_int32 __kmpc_omp_task_parts(ident_t *loc_ref, kmp_int32 gtid, #if OMPT_SUPPORT if (UNLIKELY(ompt_enabled.enabled)) { parent->ompt_task_info.frame.enter_frame = ompt_data_none; + parent->ompt_task_info.frame.enter_frame_flags = OMPT_FRAME_FLAGS_RUNTIME; } #endif return TASK_CURRENT_NOT_QUEUED; diff --git a/openmp/runtime/src/ompt-general.cpp b/openmp/runtime/src/ompt-general.cpp index 923eea2a563a9..64f83a5cb19ce 100644 --- a/openmp/runtime/src/ompt-general.cpp +++ b/openmp/runtime/src/ompt-general.cpp @@ -497,6 +497,7 @@ void ompt_post_init() { kmp_info_t *root_thread = ompt_get_thread(); ompt_set_thread_state(root_thread, ompt_state_overhead); + __ompt_task_init(root_thread->th.th_current_task, 0); if (ompt_enabled.ompt_callback_thread_begin) { ompt_callbacks.ompt_callback(ompt_callback_thread_begin)( diff --git a/openmp/runtime/src/ompt-internal.h b/openmp/runtime/src/ompt-internal.h index 580a7c2ac7916..0cfab8bfaa190 100644 --- a/openmp/runtime/src/ompt-internal.h +++ b/openmp/runtime/src/ompt-internal.h @@ -111,6 +111,8 @@ void ompt_fini(void); #define OMPT_GET_RETURN_ADDRESS(level) __builtin_return_address(level) #define OMPT_GET_FRAME_ADDRESS(level) __builtin_frame_address(level) +#define OMPT_FRAME_FLAGS_APP (ompt_frame_application | ompt_frame_cfa) +#define OMPT_FRAME_FLAGS_RUNTIME (ompt_frame_runtime | ompt_frame_cfa) int __kmp_control_tool(uint64_t command, uint64_t modifier, void *arg); diff --git a/openmp/runtime/src/ompt-specific.cpp b/openmp/runtime/src/ompt-specific.cpp index 0737c0cdfb160..94ae2e5293875 100644 --- a/openmp/runtime/src/ompt-specific.cpp +++ b/openmp/runtime/src/ompt-specific.cpp @@ -266,6 +266,8 @@ void __ompt_lw_taskteam_init(ompt_lw_taskteam_t *lwt, kmp_info_t *thr, int gtid, lwt->ompt_task_info.task_data.value = 0; lwt->ompt_task_info.frame.enter_frame = ompt_data_none; lwt->ompt_task_info.frame.exit_frame = ompt_data_none; + lwt->ompt_task_info.frame.enter_frame_flags = OMPT_FRAME_FLAGS_RUNTIME; + lwt->ompt_task_info.frame.exit_frame_flags = OMPT_FRAME_FLAGS_RUNTIME; lwt->ompt_task_info.scheduling_parent = NULL; lwt->heap = 0; lwt->parent = 0; diff --git a/openmp/runtime/src/ompt-specific.h b/openmp/runtime/src/ompt-specific.h index 7864ed6126c70..e9e40d43429ea 100644 --- a/openmp/runtime/src/ompt-specific.h +++ b/openmp/runtime/src/ompt-specific.h @@ -54,6 +54,21 @@ int __ompt_get_task_info_internal(int ancestor_level, int *type, ompt_data_t *__ompt_get_thread_data_internal(); +// __ompt_task_init: +// Initialize OMPT fields maintained by a task. This will only be called after +// ompt_start_tool, so we already know whether ompt is enabled or not. + +static inline void __ompt_task_init(kmp_taskdata_t *task, int tid) { + // The calls to __ompt_task_init already have the ompt_enabled condition. + task->ompt_task_info.task_data.value = 0; + task->ompt_task_info.frame.exit_frame = ompt_data_none; + task->ompt_task_info.frame.enter_frame = ompt_data_none; + task->ompt_task_info.frame.exit_frame_flags = + task->ompt_task_info.frame.enter_frame_flags = OMPT_FRAME_FLAGS_RUNTIME; + task->ompt_task_info.dispatch_chunk.start = 0; + task->ompt_task_info.dispatch_chunk.iterations = 0; +} + /* * Unused currently static uint64_t __ompt_get_get_unique_id_internal(); diff --git a/openmp/runtime/test/ompt/callback.h b/openmp/runtime/test/ompt/callback.h index 07d38cf836dff..4dd1db4c4225b 100644 --- a/openmp/runtime/test/ompt/callback.h +++ b/openmp/runtime/test/ompt/callback.h @@ -12,6 +12,8 @@ #include #include #include "ompt-signal.h" +#include +#include // Used to detect architecture #include "../../src/kmp_platform.h" @@ -147,6 +149,22 @@ static ompt_get_proc_id_t ompt_get_proc_id; static ompt_enumerate_states_t ompt_enumerate_states; static ompt_enumerate_mutex_impls_t ompt_enumerate_mutex_impls; +void assert_frame_flags(int enterf, int exitf) { + if (!(enterf == (ompt_frame_application | ompt_frame_cfa) || + enterf == (ompt_frame_runtime | ompt_frame_cfa))) { + printf("enter_frame_flags (%i) is invalid\n", enterf); + fflush(NULL); + } + if (!(exitf == (ompt_frame_application | ompt_frame_cfa) || + exitf == (ompt_frame_runtime | ompt_frame_cfa))) { + printf("exit_frame_flags (%i) is invalid\n", exitf); + fflush(NULL); + } + assert(enterf == (ompt_frame_application | ompt_frame_cfa) || + enterf == (ompt_frame_runtime | ompt_frame_cfa)); + assert(exitf == (ompt_frame_application | ompt_frame_cfa) || + exitf == (ompt_frame_runtime | ompt_frame_cfa)); +} static void print_ids(int level) { int task_type, thread_num; @@ -157,7 +175,7 @@ static void print_ids(int level) &task_parallel_data, &thread_num); char buffer[2048]; format_task_type(task_type, buffer); - if (frame) + if (frame) { printf("%" PRIu64 ": task level %d: parallel_id=%" PRIu64 ", task_id=%" PRIu64 ", exit_frame=%p, reenter_frame=%p, " "task_type=%s=%d, thread_num=%d\n", @@ -165,6 +183,8 @@ static void print_ids(int level) exists_task ? task_parallel_data->value : 0, exists_task ? task_data->value : 0, frame->exit_frame.ptr, frame->enter_frame.ptr, buffer, task_type, thread_num); + assert_frame_flags(frame->enter_frame_flags, frame->exit_frame_flags); + } } #define get_frame_address(level) __builtin_frame_address(level)