[OMPT] Make sure that OMPT is enabled in runtime entry points that access internals of the runtime

[sconvent]

Summary

Make sure that OMPT is enabled in runtime entry points that access internals of the runtime.
Else, return an appropiate value indicating an error or that the data is not available.

[Hahnfeld]

runtime/src/ompt-general.cpp

458 ↗

(On Diff #149756)

Should we put an if (ompt_enabled.enabled) return ompt_get_callback_failure at the very beginning of the function? That seems invariant to the switch-case...

runtime/src/ompt-specific.cpp

198 ↗

(On Diff #149756)

Is this needed in internal functions or does it just make things easier? I'm worried that adding these checks to internal functions will degrade performance...

[sconvent]

Address Jonas' comments:

[sconvent]

runtime/src/ompt-general.cpp

458 ↗

(On Diff #149756)

You meant if (!ompt_enabled.enabled) right?

[Hahnfeld]

runtime/src/ompt-general.cpp

458 ↗

(On Diff #149756)

Sure :D

runtime/src/ompt-specific.cpp

235 ↗

(On Diff #150112)

I think you do the same for the other functions in this file as well.

334 ↗

(On Diff #150112)

Same here

[sconvent]

Move ompt_enabled.enabled checks out of internal functions

[sconvent]

Add check to ompt_get_state()

[sconvent]

Add a testcase that calls the runtime entry points when OMPT is not enabled.

[jmellorcrummey]

None of this makes sense. If OMPT is not enabled, there should be no call to the ompt_initializer. For that reason, no tool should get pointers to OMPT runtime entry points if OMPT is not enabled. Therefore, the OMPT runtime entry points don’t need to check if OMPT is enabled.

[protze.joachim]

The OMPT tool can decide at multiple points to be inactive, here we look at:

4.2.1.2 Initializing an OMPT Tool

If a tool initializer returns a non-zero value, the tool will be activated for the execution; otherwise, the tool will be inactive.

This tool behavior is represented in line 136 of the test-code. Similar behavior is also expect after the ompt_finalize_tool is added: the tool should be inactive, but the runtime should still not crash or reboot if the tool calls some of the runtime entry points after calling ompt_finalize_tool.

[jmellorcrummey]

Commenting from my phone without a spec (flying without a net): I think we should change the name of the bit ompt_enabled.enabled to ompt_enabled.activated. That would make joachim’s distinction clear. I was commenting on the setting of OMP_TOOL=enabled. There is a difference between enabled and activated. Simon’s commentary in and about the test case muddled the activated vs. enabled distinction.

[protze.joachim]

In D47717#1135481, @jmellorcrummey wrote:

I think we should change the name of the bit ompt_enabled.enabled to ompt_enabled.activated. That would make joachim’s distinction clear. I was commenting on the setting of OMP_TOOL=enabled. There is a difference between enabled and activated.

I think, we should first make this distinction clear in the OpenMP spec. Then we can change the name of this bit.

[protze.joachim]

I would like to commit this patch before the 7.0 branch.
Changing the name of the bit can be done in a later patch.
@jmellorcrummey do you agree, can you remove your request for changes?

[protze.joachim]

The patch in general looks good to me.

If the name of the variable is an issue, I look forward for a patch fixing this name :)

openmp/trunk/runtime/src/ompt-general.cpp

#undef ompt_event_macro
 
   default:
     return ompt_set_error;
   }
 }
 
 OMPT_API_ROUTINE int ompt_get_callback(ompt_callbacks_t which,
                                        ompt_callback_t *callback) {
+  if (!ompt_enabled.enabled)
+    return ompt_get_callback_failure;
+
   switch (which) {
 
 #define ompt_event_macro(event_name, callback_type, event_id)                  \
   case event_name:                                                             \
     if (ompt_event_implementation_status(event_name)) {                        \
       ompt_callback_t mycb =                                                   \
           (ompt_callback_t)ompt_callbacks.ompt_callback(event_name);           \
-      if (mycb) {                                                              \
+      if (ompt_enabled.event_name && mycb) {                                   \
         *callback = mycb;                                                      \
         return ompt_get_callback_success;                                      \
       }                                                                        \
     }                                                                          \
     return ompt_get_callback_failure;
 
     FOREACH_OMPT_EVENT(ompt_event_macro)
 
 #undef ompt_event_macro
 
   default:
     return ompt_get_callback_failure;
   }
 }
 
 /*****************************************************************************
  * parallel regions
  ****************************************************************************/
 
 OMPT_API_ROUTINE int ompt_get_parallel_info(int ancestor_level,
                                             ompt_data_t **parallel_data,
                                             int *team_size) {
+  if (!ompt_enabled.enabled)
+    return 0;
   return __ompt_get_parallel_info_internal(ancestor_level, parallel_data,
                                            team_size);
 }
 
 OMPT_API_ROUTINE int ompt_get_state(ompt_wait_id_t *wait_id) {
+  if (!ompt_enabled.enabled)
+    return omp_state_work_serial;
   int thread_state = __ompt_get_state_internal(wait_id);
 
   if (thread_state == ompt_state_undefined) {
     thread_state = ompt_state_work_serial;
   }
 
   return thread_state;
 }
 
 /*****************************************************************************
  * tasks
  ****************************************************************************/
 
 OMPT_API_ROUTINE ompt_data_t *ompt_get_thread_data(void) {
+  if (!ompt_enabled.enabled)
+    return NULL;
   return __ompt_get_thread_data_internal();
 }
 
 OMPT_API_ROUTINE int ompt_get_task_info(int ancestor_level, int *type,
                                         ompt_data_t **task_data,
                                         ompt_frame_t **task_frame,
                                         ompt_data_t **parallel_data,
                                         int *thread_num) {
+  if (!ompt_enabled.enabled)
+    return 0;
   return __ompt_get_task_info_internal(ancestor_level, type, task_data,
                                        task_frame, parallel_data, thread_num);
 }
 
 OMPT_API_ROUTINE int ompt_get_task_memory(void **addr, size_t *size,
                                           int block) {
   // stub
   return 0;

 #endif
 }
 
 OMPT_API_ROUTINE int ompt_get_place_num(void) {
 // copied from kmp_ftn_entry.h (but modified)
 #if !KMP_AFFINITY_SUPPORTED
   return -1;
 #else
-  if (__kmp_get_gtid() < 0)
+  if (!ompt_enabled.enabled || __kmp_get_gtid() < 0)
     return -1;
 
   int gtid;
   kmp_info_t *thread;
   if (!KMP_AFFINITY_CAPABLE())
     return -1;
   gtid = __kmp_entry_gtid();
   thread = __kmp_thread_from_gtid(gtid);
   if (thread == NULL || thread->th.th_current_place < 0)
     return -1;
   return thread->th.th_current_place;
 #endif
 }
 
 OMPT_API_ROUTINE int ompt_get_partition_place_nums(int place_nums_size,
                                                    int *place_nums) {
 // copied from kmp_ftn_entry.h (but modified)
 #if !KMP_AFFINITY_SUPPORTED
   return 0;
 #else
-  if (__kmp_get_gtid() < 0)
+  if (!ompt_enabled.enabled || __kmp_get_gtid() < 0)
     return 0;
 
   int i, gtid, place_num, first_place, last_place, start, end;
   kmp_info_t *thread;
   if (!KMP_AFFINITY_CAPABLE())
     return 0;
   gtid = __kmp_entry_gtid();
   thread = __kmp_thread_from_gtid(gtid);

 #endif
 }
 
 /*****************************************************************************
  * places
  ****************************************************************************/
 
 OMPT_API_ROUTINE int ompt_get_proc_id(void) {
-  if (__kmp_get_gtid() < 0)
+  if (!ompt_enabled.enabled || __kmp_get_gtid() < 0)
     return -1;
 #if KMP_OS_LINUX
   return sched_getcpu();
 #elif KMP_OS_WINDOWS
   PROCESSOR_NUMBER pn;
   GetCurrentProcessorNumberEx(&pn);
   return 64 * pn.Group + pn.Number;
 #else

openmp/trunk/runtime/test/ompt/misc/api_calls_without_ompt.c

+// RUN: %libomp-compile-and-run | FileCheck %s
+// REQUIRES: ompt
+
+#define _BSD_SOURCE
+#define _DEFAULT_SOURCE
+
+#include <stdio.h>
+#include <inttypes.h>
+#include <omp.h>
+#include <ompt.h>
+
+static ompt_set_callback_t ompt_set_callback;
+static ompt_get_callback_t ompt_get_callback;
+static ompt_get_state_t ompt_get_state;
+static ompt_get_task_info_t ompt_get_task_info;
+static ompt_get_thread_data_t ompt_get_thread_data;
+static ompt_get_parallel_info_t ompt_get_parallel_info;
+static ompt_get_unique_id_t ompt_get_unique_id;
+static ompt_get_num_procs_t ompt_get_num_procs;
+static ompt_get_num_places_t ompt_get_num_places;
+static ompt_get_place_proc_ids_t ompt_get_place_proc_ids;
+static ompt_get_place_num_t ompt_get_place_num;
+static ompt_get_partition_place_nums_t ompt_get_partition_place_nums;
+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;
+
+int main() {
+  // Call OpenMP API function to force initialization of OMPT.
+  // (omp_get_thread_num() does not work because it just returns 0 if the
+  // runtime isn't initialized yet...)
+  omp_get_num_threads();
+
+  ompt_data_t *tdata = ompt_get_thread_data();
+  uint64_t tvalue = tdata ? tdata->value : 0;
+
+  printf("%" PRIu64 ": ompt_get_num_places()=%d\n", tvalue,
+         ompt_get_num_places());
+
+  printf("%" PRIu64 ": ompt_get_place_proc_ids()=%d\n", tvalue,
+         ompt_get_place_proc_ids(0, 0, NULL));
+
+  printf("%" PRIu64 ": ompt_get_place_num()=%d\n", tvalue,
+         ompt_get_place_num());
+
+  printf("%" PRIu64 ": ompt_get_partition_place_nums()=%d\n", tvalue,
+         ompt_get_partition_place_nums(0, NULL));
+
+  printf("%" PRIu64 ": ompt_get_proc_id()=%d\n", tvalue, ompt_get_proc_id());
+
+  printf("%" PRIu64 ": ompt_get_num_procs()=%d\n", tvalue,
+         ompt_get_num_procs());
+
+  ompt_callback_t callback;
+  printf("%" PRIu64 ": ompt_get_callback()=%d\n", tvalue,
+         ompt_get_callback(ompt_callback_thread_begin, &callback));
+
+  printf("%" PRIu64 ": ompt_get_state()=%d\n", tvalue, ompt_get_state(NULL));
+
+  int state = omp_state_undefined;
+  const char *state_name;
+  printf("%" PRIu64 ": ompt_enumerate_states()=%d\n", tvalue,
+         ompt_enumerate_states(state, &state, &state_name));
+
+  int impl = ompt_mutex_impl_unknown;
+  const char *impl_name;
+  printf("%" PRIu64 ": ompt_enumerate_mutex_impls()=%d\n", tvalue,
+         ompt_enumerate_mutex_impls(impl, &impl, &impl_name));
+
+  printf("%" PRIu64 ": ompt_get_thread_data()=%p\n", tvalue,
+         ompt_get_thread_data());
+
+  printf("%" PRIu64 ": ompt_get_parallel_info()=%d\n", tvalue,
+         ompt_get_parallel_info(0, NULL, NULL));
+
+  printf("%" PRIu64 ": ompt_get_task_info()=%d\n", tvalue,
+         ompt_get_task_info(0, NULL, NULL, NULL, NULL, NULL));
+
+  // Check if libomp supports the callbacks for this test.
+
+  // CHECK: 0: NULL_POINTER=[[NULL:.*$]]
+
+  // CHECK: {{^}}[[MASTER_ID:[0-9]+]]: ompt_get_num_places()={{[0-9]+}}
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_get_place_proc_ids()={{[0-9]+}}
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_get_place_num()=-1
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_get_partition_place_nums()=0
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_get_proc_id()=-1
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_get_num_procs()={{[0-9]+}}
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_get_callback()=0
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_get_state()=0
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_enumerate_states()=1
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_enumerate_mutex_impls()=1
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_get_thread_data()=[[NULL]]
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_get_parallel_info()=0
+
+  // CHECK: {{^}}[[MASTER_ID]]: ompt_get_task_info()=0
+
+  return 0;
+}
+
+int ompt_initialize(ompt_function_lookup_t lookup, ompt_data_t *tool_data) {
+  ompt_set_callback = (ompt_set_callback_t)lookup("ompt_set_callback");
+  ompt_get_callback = (ompt_get_callback_t)lookup("ompt_get_callback");
+  ompt_get_state = (ompt_get_state_t)lookup("ompt_get_state");
+  ompt_get_task_info = (ompt_get_task_info_t)lookup("ompt_get_task_info");
+  ompt_get_thread_data = (ompt_get_thread_data_t)lookup("ompt_get_thread_data");
+  ompt_get_parallel_info =
+      (ompt_get_parallel_info_t)lookup("ompt_get_parallel_info");
+  ompt_get_unique_id = (ompt_get_unique_id_t)lookup("ompt_get_unique_id");
+
+  ompt_get_num_procs = (ompt_get_num_procs_t)lookup("ompt_get_num_procs");
+  ompt_get_num_places = (ompt_get_num_places_t)lookup("ompt_get_num_places");
+  ompt_get_place_proc_ids =
+      (ompt_get_place_proc_ids_t)lookup("ompt_get_place_proc_ids");
+  ompt_get_place_num = (ompt_get_place_num_t)lookup("ompt_get_place_num");
+  ompt_get_partition_place_nums =
+      (ompt_get_partition_place_nums_t)lookup("ompt_get_partition_place_nums");
+  ompt_get_proc_id = (ompt_get_proc_id_t)lookup("ompt_get_proc_id");
+  ompt_enumerate_states =
+      (ompt_enumerate_states_t)lookup("ompt_enumerate_states");
+  ompt_enumerate_mutex_impls =
+      (ompt_enumerate_mutex_impls_t)lookup("ompt_enumerate_mutex_impls");
+
+  printf("0: NULL_POINTER=%p\n", (void *)NULL);
+  return 0; // no success -> OMPT not enabled
+}
+
+void ompt_finalize(ompt_data_t *tool_data) {
+  printf("0: ompt_event_runtime_shutdown\n");
+}
+
+ompt_start_tool_result_t *ompt_start_tool(unsigned int omp_version,
+                                          const char *runtime_version) {
+  static ompt_start_tool_result_t ompt_start_tool_result = {&ompt_initialize,
+                                                            &ompt_finalize, 0};
+  return &ompt_start_tool_result;
+}