[OMPT] Windows Support for OMPT

[jlpeyton]

Summary

The problem is that the ompt_tool() function (which must be implemented by a performance tool) should be defined in the RTL as well to cover the case when the tool is not present in the address space of the process. This functionality is accomplished with weak symbols in Unices. Unfortunately, Windows does not support weak symbols.

The solution in these changes is to grab the list of all modules loaded by the process and then search for symbol "ompt_tool()" within them. The function ompt_tool_windows() performs the search of the ompt_tool symbol. If ompt_tool is found, then its return value is used to initialize the tool. If ompt_tool is not found, then ompt_tool_windows() returns NULL and OMPT is thus, disabled.

While doing these changes, the OMPT_SUPPORT detection in CMake was changed to test for the required featuers for OMPT_SUPPORT, namely: builtin_frame_address() existence, weak attribute existence and psapi.dll existence. For LIBOMP_HAVE_OMPT_SUPPORT to be true, it must be that the builtin_frame_address() intrinsic exists
AND one of: either weak attributes exist or psapi.dll exists.

Also, since Process Status API is used I had to add new dependency -- psapi.dll to the library dependency micro test.

[jmellorcrummey]

While the changes to the build system seem fine to me, the changes to ompt-general.c are awkward. I don't like the "#if KMP_HAVE_PSAPI" special casing inside ompt_pre_init. Also, there is no need for a function ompt_aux_tool. I recommend the following instead:

In ompt_pre_init, I recommend removing all ifdefs around the line

ompt_initialize_fn = ompt_tool();

Instead, at the top of the file I recommend that under "#elif KMP_HAVE_PSAPI" you eliminate the code for ompt_aux_tool and instead have

#define ompt_tool ompt_windows_ompt_tool

ompt_initialize_t ompt_tool_windows()
{

...

}

The implementation of ompt_tool_windows should be based on the implementation for what you call ompt_symbol_lookup with two changes. First, the name of the symbol to look up "ompt_tool" should be "baked in". OMPT relies only on one public symbol by design; there will never be a need to look up another function in a tool, so the functionality provided by ompt_symbol_lookup is overkill. Second, in all cases where __ompt_symbol_lookup returns &ompt_aux_tool, ompt_tool_windows should just return NULL. Third, rather than breaking and returning the address of the function fn returned by

fn = GetProcAddress(modules[i], "ompt_tool")

ompt_tool_windows should just return the value returned by invoking fn.

These changes eliminate the need for the useless function ompt_aux_tool, eliminate the excess functionality provided by __ompt_symbol_lookup, and eliminate special case handling for Windows inside ompt_pre_init.

[jmellorcrummey]

Two more things:

First, I made some name changes in the midst of writing my prior comment. I didn't change one occurrence. The corrected #define for the prior comment should be

#define ompt_tool ompt_tool_windows

Second, I note that the implementation

#if KMP_HAVE_WEAK_ATTRIBUTE

#elif KMP_HAVE_PSAPI

#endif

will fail with an undefined symbol for ompt_tool if neither KMP_HAVE_WEAK_ATTRIBUTE or KMP_HAVE_PSAPI are defined. Should there be an #else case with a #error that says that either weak symbols or libpsapi are required.

[jmellorcrummey]

One final comment:

I designed ompt-general.c as a source file that could be used by anyone any runtime system to implement OMPT. Before this revision, there was no dependence of this file on anything to do with Intel runtime. This change introduces several KMP defines to this runtime. I would prefer that the names of defines used in this file have the OMPT prefix, e.g. OMPT_WEAK_ATTRIBUTE, OMPT_PSAPI, and OMPT_DEBUG. These defines could be set in ompt-specific.h (which is intended provide details for tailoring OMPT for any particular runtime) as

#define OMPT_WEAK_ATTRIBUTE KMP_WEAK_ATTRIBUTE
#define OMPT_PSAPI KMP_PSAPI
#define OMPT_DEBUG KMP_TRACE

[jmellorcrummey]

I was wrong. I had one more thing to say :-).

Changing strcasecmp for argument matching to __kmp_str_match is another change that binds the ompt-general.c implementation to the Intel runtime.

I would prefer adding a layer of abstraction to this if you want to use __kmp_str_match. I recommend that ompt-specific.h have the following line added

#define OMPT_STR_MATCH(haystack, needle) __kmp_str_match(haystack, 0, needle)

and then in ompt-general.c, include the following line

#ifndef OMPT_STR_MATCH
#define OMPT_STR_MATCH(haystack, needle) (!strcasecmp(haystack, needle))
#endif

[jlpeyton]

Addressed all of John Mellor-Crummey's suggestions. (Did them all)

[jmellorcrummey]

I have one more minor comment

Lines 104 and 105 of ompt-general.c should be reversed.

_OMP_EXTERN
#if OMPT_HAVE_WEAK_ATTRIBUTE

should be

#if OMPT_HAVE_WEAK_ATTRIBUTE
_OMP_EXTERN

because the _OMP_EXTERN does not apply to the #elseif case.

Besides that, everything now looks good to me.

openmp/trunk/runtime/CMakeLists.txt

if(LIBOMP_STATS AND (NOT LIBOMP_HAVE_STATS))
   libomp_error_say("Stats-gathering functionality requested but not available")
 endif()
 # The stats functionality requires the std c++ library
 if(LIBOMP_STATS)
   set(LIBOMP_USE_STDCPPLIB TRUE)
 endif()
 
 # OMPT-support
-# TODO: Make this a real feature check
 set(LIBOMP_OMPT_SUPPORT FALSE CACHE BOOL
   "OMPT-support?")
 set(LIBOMP_OMPT_BLAME TRUE CACHE BOOL
   "OMPT-blame?")
 set(LIBOMP_OMPT_TRACE TRUE CACHE BOOL
   "OMPT-trace?")
 if(LIBOMP_OMPT_SUPPORT AND (NOT LIBOMP_HAVE_OMPT_SUPPORT))
   libomp_error_say("OpenMP Tools Interface requested but not available")

openmp/trunk/runtime/cmake/LibompMicroTests.cmake

 if(CMAKE_SYSTEM_NAME MATCHES "FreeBSD")
   set(libomp_expected_library_deps libc.so.7 libthr.so.3)
 elseif(CMAKE_SYSTEM_NAME MATCHES "NetBSD")
   set(libomp_expected_library_deps libc.so.12 libpthread.so.1 libm.so.0)
 elseif(APPLE)
   set(libomp_expected_library_deps /usr/lib/libSystem.B.dylib)
 elseif(WIN32)
   set(libomp_expected_library_deps kernel32.dll)
+  libomp_append(libomp_expected_library_deps psapi.dll LIBOMP_OMPT_SUPPORT)
 else()
   if(${MIC})
     set(libomp_expected_library_deps libc.so.6 libpthread.so.0 libdl.so.2)
     if("${LIBOMP_MIC_ARCH}" STREQUAL "knf")
       libomp_append(libomp_expected_library_deps ld-linux-l1om.so.2)
       libomp_append(libomp_expected_library_deps libgcc_s.so.1)
     elseif("${LIBOMP_MIC_ARCH}" STREQUAL "knc")
       libomp_append(libomp_expected_library_deps ld-linux-k1om.so.2)

openmp/trunk/runtime/cmake/config-ix.cmake

 #
 #//===----------------------------------------------------------------------===//
 #//
 #//                     The LLVM Compiler Infrastructure
 #//
 #// This file is dual licensed under the MIT and the University of Illinois Open
 #// Source Licenses. See LICENSE.txt for details.
 #//
 #//===----------------------------------------------------------------------===//
 #
 
 include(CheckCCompilerFlag)
 include(CheckCSourceCompiles)
 include(CheckCXXCompilerFlag)
 include(CheckLibraryExists)
+include(CheckIncludeFiles)
 include(LibompCheckLinkerFlag)
 include(LibompCheckFortranFlag)
 
 # Check for versioned symbols
 function(libomp_check_version_symbols retval)
   set(source_code
     "#include <stdio.h>
     void func1() { printf(\"Hello\"); }

 # Check if stats-gathering is available
 if(NOT (WIN32 OR APPLE) AND (${IA32} OR ${INTEL64} OR ${MIC}))
   set(LIBOMP_HAVE_STATS TRUE)
 else()
   set(LIBOMP_HAVE_STATS FALSE)
 endif()
 
 # Check if OMPT support is available
-if(NOT WIN32)
+# Currently, __builtin_frame_address() is required for OMPT
+# Weak attribute is required for Unices, LIBPSAPI is used for Windows
+check_c_source_compiles("int main(int argc, char** argv) {
+  void* p = __builtin_frame_address(0);
+  return 0;}" LIBOMP_HAVE___BUILTIN_FRAME_ADDRESS)
+check_c_source_compiles("__attribute__ ((weak)) int foo(int a) { return a*a; }
+  int main(int argc, char** argv) {
+  return foo(argc);}" LIBOMP_HAVE_WEAK_ATTRIBUTE)
+check_include_files("windows.h;psapi.h" LIBOMP_HAVE_PSAPI_H)
+check_library_exists(psapi EnumProcessModules "" LIBOMP_HAVE_LIBPSAPI)
+if(LIBOMP_HAVE_PSAPI_H AND LIBOMP_HAVE_LIBPSAPI)
+  set(LIBOMP_HAVE_PSAPI TRUE)
+endif()
+if(NOT LIBOMP_HAVE___BUILTIN_FRAME_ADDRESS)
+  set(LIBOMP_HAVE_OMPT_SUPPORT FALSE)
+else()
+  if(LIBOMP_HAVE_WEAK_ATTRIBUTE OR LIBOMP_HAVE_PSAPI)
     set(LIBOMP_HAVE_OMPT_SUPPORT TRUE)
   else()
     set(LIBOMP_HAVE_OMPT_SUPPORT FALSE)
   endif()
+endif()
+

openmp/trunk/runtime/src/kmp_config.h.cmake

 #define USE_ITT_NOTIFY LIBOMP_USE_ITT_NOTIFY
 #if ! LIBOMP_USE_ITT_NOTIFY
 # define INTEL_NO_ITTNOTIFY_API
 #endif
 #cmakedefine01 LIBOMP_USE_VERSION_SYMBOLS
 #if LIBOMP_USE_VERSION_SYMBOLS
 # define KMP_USE_VERSION_SYMBOLS
 #endif
+#cmakedefine01 LIBOMP_HAVE_WEAK_ATTRIBUTE
+#define KMP_HAVE_WEAK_ATTRIBUTE LIBOMP_HAVE_WEAK_ATTRIBUTE
+#cmakedefine01 LIBOMP_HAVE_PSAPI
+#define KMP_HAVE_PSAPI LIBOMP_HAVE_PSAPI
 #cmakedefine01 LIBOMP_STATS
 #define KMP_STATS_ENABLED LIBOMP_STATS
 #cmakedefine01 LIBOMP_USE_DEBUGGER
 #define USE_DEBUGGER LIBOMP_USE_DEBUGGER
 #cmakedefine01 LIBOMP_OMPT_SUPPORT
 #define OMPT_SUPPORT LIBOMP_OMPT_SUPPORT
 #cmakedefine01 LIBOMP_OMPT_BLAME
 #define OMPT_BLAME LIBOMP_OMPT_BLAME

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

 #include <string.h>
 
 
 
 /*****************************************************************************
  * ompt include files
  ****************************************************************************/
 
-#include "kmp_config.h"
 #include "ompt-internal.h"
 #include "ompt-specific.c"
 
 
 
 /*****************************************************************************
  * macros
  ****************************************************************************/
 
 #define ompt_get_callback_success 1
 #define ompt_get_callback_failure 0
 
 #define no_tool_present 0
 
 #define OMPT_API_ROUTINE static
 
+#ifndef OMPT_STR_MATCH
+#define OMPT_STR_MATCH(haystack, needle) (!strcasecmp(haystack, needle))
+#endif
 
 
 /*****************************************************************************
  * types
  ****************************************************************************/
 
 typedef struct {
     const char *state_name;

  * forward declarations
  ****************************************************************************/
 
 static ompt_interface_fn_t ompt_fn_lookup(const char *s);
 
 OMPT_API_ROUTINE ompt_thread_id_t ompt_get_thread_id(void);
 
 
-
 /*****************************************************************************
  * initialization and finalization (private operations)
  ****************************************************************************/
 
-_OMP_EXTERN __attribute__ (( weak ))
+/* On Unix-like systems that support weak symbols the following implementation
+ * of ompt_tool() will be used in case no tool-supplied implementation of
+ * this function is present in the address space of a process.
+ *
+ * On Windows, the ompt_tool_windows function is used to find the
+ * ompt_tool symbol across all modules loaded by a process. If ompt_tool is
+ * found, ompt_tool's return value is used to initialize the tool. Otherwise,
+ * NULL is returned and OMPT won't be enabled */
+#if OMPT_HAVE_WEAK_ATTRIBUTE
+_OMP_EXTERN 
+__attribute__ (( weak ))
 ompt_initialize_t ompt_tool()
 {
+#if OMPT_DEBUG
+    printf("ompt_tool() is called from the RTL\n");
+#endif
     return NULL;
 }
 
+#elif OMPT_HAVE_PSAPI
+
+#include <psapi.h>
+#pragma comment(lib, "psapi.lib")
+#define ompt_tool ompt_tool_windows
+
+// The number of loaded modules to start enumeration with EnumProcessModules()
+#define NUM_MODULES 128
+
+static
+ompt_initialize_t ompt_tool_windows()
+{
+    int i;
+    DWORD needed, new_size;
+    HMODULE *modules;
+    HANDLE  process = GetCurrentProcess();
+    modules = (HMODULE*)malloc( NUM_MODULES * sizeof(HMODULE) );
+    ompt_initialize_t (*ompt_tool_p)() = NULL;
+
+#if OMPT_DEBUG
+    printf("ompt_tool_windows(): looking for ompt_tool\n");
+#endif
+    if( !EnumProcessModules( process, modules, NUM_MODULES * sizeof(HMODULE),
+                             &needed ) ) {
+        // Regardless of the error reason use the stub initialization function
+        return NULL;
+    }
+    // Check if NUM_MODULES is enough to list all modules
+    new_size = needed / sizeof(HMODULE);
+    if( new_size > NUM_MODULES ) {
+#if OMPT_DEBUG
+    printf("ompt_tool_windows(): resize buffer to %d bytes\n", needed);
+#endif
+        modules = (HMODULE*)realloc( modules, needed );
+        // If resizing failed use the stub function.
+        if( !EnumProcessModules( process, modules, needed, &needed ) ) {
+            return NULL;
+        }
+    }
+    for( i = 0; i < new_size; ++i ) {
+        (FARPROC &)ompt_tool_p = GetProcAddress(modules[i], "ompt_tool");
+        if( ompt_tool_p ) {
+#if OMPT_DEBUG
+            TCHAR modName[MAX_PATH];
+            if( GetModuleFileName(modules[i], modName, MAX_PATH))
+                printf("ompt_tool_windows(): ompt_tool found in module %s\n",
+                       modName);
+#endif
+            return ompt_tool_p();
+        }
+#if OMPT_DEBUG
+        else {
+            TCHAR modName[MAX_PATH];
+            if( GetModuleFileName(modules[i], modName, MAX_PATH) )
+                printf("ompt_tool_windows(): ompt_tool not found in module %s\n",
+                       modName);
+        }
+#endif
+    }
+    return NULL;
+}
+#else
+# error Either __attribute__((weak)) or psapi.dll are required for OMPT support
+#endif // OMPT_HAVE_WEAK_ATTRIBUTE
 
 void ompt_pre_init()
 {
     //--------------------------------------------------
     // Execute the pre-initialization logic only once.
     //--------------------------------------------------
     static int ompt_pre_initialized = 0;
 
     if (ompt_pre_initialized) return;
 
     ompt_pre_initialized = 1;
 
     //--------------------------------------------------
     // Use a tool iff a tool is enabled and available.
     //--------------------------------------------------
     const char *ompt_env_var = getenv("OMP_TOOL");
     tool_setting_e tool_setting = omp_tool_error;
 
     if (!ompt_env_var  || !strcmp(ompt_env_var, ""))
         tool_setting = omp_tool_unset;
-    else if (!strcasecmp(ompt_env_var, "disabled"))
+    else if (OMPT_STR_MATCH(ompt_env_var, "disabled"))
         tool_setting = omp_tool_disabled;
-    else if (!strcasecmp(ompt_env_var, "enabled"))
+    else if (OMPT_STR_MATCH(ompt_env_var, "enabled"))
         tool_setting = omp_tool_enabled;
 
+#if OMPT_DEBUG
+    printf("ompt_pre_init(): tool_setting = %d\n", tool_setting);
+#endif
     switch(tool_setting) {
     case omp_tool_disabled:
         break;
 
     case omp_tool_unset:
     case omp_tool_enabled:
         ompt_initialize_fn = ompt_tool();
         if (ompt_initialize_fn) {
             ompt_enabled = 1;
         }
         break;
 
     case omp_tool_error:
         fprintf(stderr,
             "Warning: OMP_TOOL has invalid value \"%s\".\n"
             "  legal values are (NULL,\"\",\"disabled\","
             "\"enabled\").\n", ompt_env_var);
         break;
     }
-
+#if OMPT_DEBUG
+    printf("ompt_pre_init():ompt_enabled = %d\n", ompt_enabled);
+#endif
 }
 
 
 void ompt_post_init()
 {
     //--------------------------------------------------
     // Execute the post-initialization logic only once.
     //--------------------------------------------------

openmp/trunk/runtime/src/ompt-specific.h

 
 int __ompt_get_parallel_team_size_internal(int ancestor_level);
 
 ompt_task_id_t __ompt_get_task_id_internal(int depth);
 
 ompt_frame_t *__ompt_get_task_frame_internal(int depth);
 
 
+/*****************************************************************************
+ * macros
+ ****************************************************************************/
+#define OMPT_DEBUG KMP_DEBUG
+#define OMPT_HAVE_WEAK_ATTRIBUTE KMP_HAVE_WEAK_ATTRIBUTE
+#define OMPT_HAVE_PSAPI KMP_HAVE_PSAPI
+#define OMPT_STR_MATCH(haystack, needle) __kmp_str_match(haystack, 0, needle)
+
 
 //******************************************************************************
 // inline functions
 //******************************************************************************
 
 inline ompt_thread_t *
 ompt_get_thread_gtid(int gtid)
 {

openmp/trunk/runtime/src/ompt-specific.c

 //   sort out how to implement a higher performance version in their runtime.
 
 // when using fetch_and_add to generate the IDs, there isn't any reason to waste
 // bits for thread id.
 #if 0
 #define NEXT_ID(id_ptr,tid) \
   ((KMP_TEST_THEN_INC64(id_ptr) << OMPT_THREAD_ID_BITS) | (tid))
 #else
-#define NEXT_ID(id_ptr,tid) (KMP_TEST_THEN_INC64(id_ptr))
+#define NEXT_ID(id_ptr,tid) (KMP_TEST_THEN_INC64((volatile kmp_int64 *)id_ptr))
 #endif
 
 //******************************************************************************
 // private operations
 //******************************************************************************
 
 //----------------------------------------------------------
 // traverse the team and task hierarchy