[OpenMP][OMPT][OMPD] Fix frame flags for OpenMP tool APIs (llvm#114118)

jprotze · web-flow · commit 12a9e2adc384 · 2025-02-27T18:47:57.000+01:00
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 entry 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 llvm#89058
diff --git 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
@@ -498,6 +498,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
@@ -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
@@ -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
@@ -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
@@ -12,6 +12,8 @@
 #include <omp.h>
 #include <omp-tools.h>
 #include "ompt-signal.h"
+#include <stdlib.h>
+#include <assert.h>
 
 // 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,14 +175,16 @@ 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",
            ompt_get_thread_data()->value, 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)
