Switch the Windows OpenMP import library to import by name rather than ordinal.

[vadikp-intel]

Summary

The current OpenMP build script generates the Windows import library that imports by ordinal. This requires ordinal numbering coordination between contributors adding new APIs to the runtime and has in the past caused collisions requiring breaking changes to resolve them. Switching the library to import by name helps to avoid such collisions while also increasing compatibility between the different Windows versions of the OpenMP runtime (e.g. stock, MSVC's, etc.). The change keeps the existing ordinal ordering in the runtime to maintain compatibility of applications built with previous OpenMP releases. Note: due to incomplete 'llvm-lib' compatibility with 'lib' currently the solution only works with MSVC or ICC based Windows builds.

[tianshilei1992]

Can you update the patch with context?

[jdoerfert]

Please add the MSVC folks as reviewers too.

[jlpeyton]

Can you comment on the removal of kmp_import.cpp? Is it not needed anymore because Microsoft is moving away from the vcomp interface? If it is indeed unnecessary, then shouldn't we just remove the file entirely?

[vadikp-intel]

kmp_import.cpp turned up as a random side-effect of this change as the latter piggy-backed on the existing build step that rebuilt the import library dragging kmp_import in (for unrelated reasons, apparently to enforce some rules for code built with a toolset mix) With this change, kmp_imports is no longer necessary because the intent looks to already be covered elsewhere in the code now being linked (kmp_import might be some workaround from days past). I did not include its removal here for two reasons a) it is not directly related to the intent of the ordinals change and b) we (MSVC and Intel) are discussing what the desired behavior here should going forward for LLVM OpenMP flavors. I will remove kmp_import (if it is indeed no longer required) with the check-in that effects what we want this to be.

[vadikp-intel]

Jonathan, if all looks good could you commit on my behalf?

[mstorsjo]

This patch broke building in mingw mode, with errors like this:

ninja: error: build.ninja:875: multiple rules generate runtime/src/libomp.dll.a [-w dupbuild=err]

I've got a patch I can submit to fix that. But this patch also has a couple of other issues.

openmp/runtime/src/CMakeLists.txt

278

This patch adds a new nice define LIBOMP_GENERATED_DEF_FILE here, but on line 240, we still refer to it by hardcoding its value, add_custom_target(libomp-needed-windows-files DEPENDS ${LIBOMP_LIB_NAME}.def). It would be good if both references would use the same cmake variable, since right now, it seems like you can adjust its name, but that only causes breakage.

288

Here, you're generating two files with one single cmake add_custom_command. This means that cmake essentially doesn't know that the second file, LIBOMPIMP_GENERATED_DEF_FILE_IMP is generated. When cleaning, it doesn't get removed, and if you just build ninja runtime/src/libomp.lib without having built the rest before, then cmake/ninja won't know about generating the def file before. So I'd suggest generating that second file with a separate command so that cmake knows and can track the file (and probably adding a suitable depenency for the ompimp target so that it generates the def file as needed).

289

Previously, the finally installed import library contained the compiled kmp_import.cpp too, which contains symbols like _You_must_link_with_exactly_one_OpenMP_library. These are now lost.

293

By adding all source files here, you end up compiling all the object files twice - even if they aren't really strictly used for the second invocation (where we just invoke lib.exe to regenerate an import library). Not sure if cmake is ok with that, or if it would need a dummy source file at least.

Compiling all files twice feels wasteful; the openmp runtime isn't that big so it's not a huge deal, but it seems quite redundant and inelegant.

[mstorsjo]

openmp/runtime/src/CMakeLists.txt

289

Sorry, I see that this particular aspect was actually already discussed.

openmp/runtime/cmake/LibompMicroTests.cmake

 #  - Fails if sees a dependence not listed in td_exp variable below
 #  - Program dependencies: perl, (unix)readelf, (mac)otool[64], (windows)link.exe
 #  - Available for Unix,Mac,Windows, Intel(R) MIC Architecture dynamic builds and Windows
 #    static builds. Not available otherwise.
 
 # get library location
 if(WIN32)
   get_target_property(LIBOMP_OUTPUT_DIRECTORY omp RUNTIME_OUTPUT_DIRECTORY)
-  get_target_property(LIBOMPIMP_OUTPUT_DIRECTORY ${LIBOMP_IMP_LIB_TARGET} ARCHIVE_OUTPUT_DIRECTORY)
+  get_target_property(LIBOMPIMP_OUTPUT_DIRECTORY ompimp ARCHIVE_OUTPUT_DIRECTORY)
   if(NOT LIBOMPIMP_OUTPUT_DIRECTORY)
     set(LIBOMPIMP_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
   endif()
 else()
   get_target_property(LIBOMP_OUTPUT_DIRECTORY omp LIBRARY_OUTPUT_DIRECTORY)
 endif()
 if(NOT LIBOMP_OUTPUT_DIRECTORY)
   set(LIBOMP_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})

openmp/runtime/src/CMakeLists.txt

if(WIN32)
   libomp_append(LIBOMP_MASM_DEFINITIONS "-D_M_AMD64" IF_TRUE INTEL64)
   libomp_append(LIBOMP_MASM_DEFINITIONS "-DOMPT_SUPPORT" IF_TRUE_1_0 LIBOMP_OMPT_SUPPORT)
   libomp_append(LIBOMP_MASM_DEFINITIONS "-DOMPD_SUPPORT" IF_TRUE_1_0 LIBOMP_OMPD_SUPPORT)
   libomp_list_to_string("${LIBOMP_MASM_DEFINITIONS}" LIBOMP_MASM_DEFINITIONS)
   set_property(SOURCE z_Windows_NT-586_asm.asm APPEND_STRING PROPERTY COMPILE_FLAGS " ${LIBOMP_MASM_DEFINITIONS}")
   set_source_files_properties(thirdparty/ittnotify/ittnotify_static.cpp PROPERTIES COMPILE_DEFINITIONS "UNICODE")
 
   # Create Windows import library
-  # the import library is "re-linked" to include kmp_import.cpp which prevents
-  # linking of both Visual Studio OpenMP and newly built OpenMP
-  set_source_files_properties(kmp_import.cpp PROPERTIES COMPILE_FLAGS "${LIBOMP_CONFIGURED_CXXFLAGS}")
-
   # On debug LIBOMP_IMP_LIB_FILE should be LIBOMP_IMP_LIB_FILE_DBG = ${LIBOMP_DEFAULT_LIB_NAME_DBG}${CMAKE_STATIC_LIBRARY_SUFFIX}
   #                   ${LIBOMP_DEFAULT_LIB_NAME}d${CMAKE_STATIC_LIBRARY_SUFFIX}
   # and the ARCHIVE_OUTPUT_NAME of ompdbg should be ${LIBOMP_DEFAULT_LIB_NAME_DBG}${LIBOMP_LIBRARY_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX}
 
   if(OPENMP_MSVC_NAME_SCHEME)
     if(uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
       set(LIBOMP_IMP_LIB_FILE ${LIBOMP_DEFAULT_LIB_NAME}d${CMAKE_STATIC_LIBRARY_SUFFIX})
       set(LIBOMP_GENERATED_IMP_LIB_FILENAME ${LIBOMP_DEFAULT_LIB_NAME}d${LIBOMP_LIBRARY_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX})
     else()
       set(LIBOMP_IMP_LIB_FILE ${LIBOMP_DEFAULT_LIB_NAME}${CMAKE_IMPORT_LIBRARY_SUFFIX})
       set(LIBOMP_GENERATED_IMP_LIB_FILENAME ${LIBOMP_DEFAULT_LIB_NAME}${LIBOMP_LIBRARY_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX})
     endif()
   else()
     set(LIBOMP_IMP_LIB_FILE ${LIBOMP_LIB_NAME}${CMAKE_IMPORT_LIBRARY_SUFFIX})
     set(LIBOMP_GENERATED_IMP_LIB_FILENAME ${LIBOMP_LIB_FILE}${CMAKE_STATIC_LIBRARY_SUFFIX})
   endif()
   set_target_properties(omp PROPERTIES
     VERSION ${LIBOMP_VERSION_MAJOR}.${LIBOMP_VERSION_MINOR} # uses /version flag
     IMPORT_PREFIX "" IMPORT_SUFFIX "" # control generated import library name when building omp
     ARCHIVE_OUTPUT_NAME ${LIBOMP_GENERATED_IMP_LIB_FILENAME}
   )
 
-  if(MSVC)
-    # Get generated import library from creating omp
-    get_target_property(LIBOMP_IMPORT_LIB_DIRECTORY omp ARCHIVE_OUTPUT_DIRECTORY)
-    if(LIBOMP_IMPORT_LIB_DIRECTORY)
-      set(LIBOMP_GENERATED_IMP_LIB ${LIBOMP_IMPORT_LIB_DIRECTORY}/${LIBOMP_GENERATED_IMP_LIB_FILENAME})
-    else()
-      set(LIBOMP_GENERATED_IMP_LIB ${CMAKE_CURRENT_BINARY_DIR}/${LIBOMP_GENERATED_IMP_LIB_FILENAME})
-    endif()
-    set_source_files_properties(${LIBOMP_GENERATED_IMP_LIB} PROPERTIES GENERATED TRUE EXTERNAL_OBJECT TRUE)
-    # Create new import library that is just the previously created one + kmp_import.cpp
-    add_library(ompimp STATIC ${LIBOMP_GENERATED_IMP_LIB} kmp_import.cpp)
-    set_target_properties(ompimp PROPERTIES
-      PREFIX "" SUFFIX "" OUTPUT_NAME "${LIBOMP_IMP_LIB_FILE}"
-      LINKER_LANGUAGE C
-    )
-    add_dependencies(ompimp omp) # ensure generated import library is created first
-    set(LIBOMP_IMP_LIB_TARGET ompimp)
-  else()
-    set(LIBOMP_IMP_LIB_TARGET omp)
-  endif()
-
-  # Create def file to designate exported functions
+  # Create def files to designate exported functions
   libomp_get_gdflags(LIBOMP_GDFLAGS) # generate-def.pl flags (Windows only)
   libomp_string_to_list("${LIBOMP_GDFLAGS}" LIBOMP_GDFLAGS)
+  set(LIBOMP_GENERATED_DEF_FILE ${LIBOMP_LIB_NAME}.def)
+  set(LIBOMPIMP_GENERATED_DEF_FILE_IMP ${LIBOMP_LIB_NAME}.imp.def)
   add_custom_command(
     OUTPUT  ${LIBOMP_LIB_NAME}.def
-    COMMAND ${PERL_EXECUTABLE} ${LIBOMP_TOOLS_DIR}/generate-def.pl ${LIBOMP_GDFLAGS}
-      -o ${LIBOMP_LIB_NAME}.def ${CMAKE_CURRENT_SOURCE_DIR}/dllexports
+    # one with ordinals to use for building the runtime dll to maintain backwads compatible exports order
+    COMMAND ${PERL_EXECUTABLE} ${LIBOMP_TOOLS_DIR}/generate-def.pl ${LIBOMP_GDFLAGS} -D NAME=${LIBOMP_LIB_FILE}
+            -o ${LIBOMP_GENERATED_DEF_FILE} ${CMAKE_CURRENT_SOURCE_DIR}/dllexports
+    DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/dllexports ${LIBOMP_TOOLS_DIR}/generate-def.pl
+    # one without ordinals to use for building the import library to remove ordinal dependency going forward
+    COMMAND ${PERL_EXECUTABLE} ${LIBOMP_TOOLS_DIR}/generate-def.pl ${LIBOMP_GDFLAGS} -D NAME=${LIBOMP_LIB_FILE} -D NOORDINALS
+            -o ${LIBOMPIMP_GENERATED_DEF_FILE_IMP} ${CMAKE_CURRENT_SOURCE_DIR}/dllexports
     DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/dllexports ${LIBOMP_TOOLS_DIR}/generate-def.pl
   )
+
+  # Regenerate the import library to import by name, not ordinal
+  add_library(ompimp STATIC ${LIBOMP_SOURCE_FILES})
+    set_target_properties(ompimp PROPERTIES
+      PREFIX "" SUFFIX "" OUTPUT_NAME "${LIBOMP_IMP_LIB_FILE}"
+    LINKER_LANGUAGE C
+  )
+  set_target_properties(ompimp PROPERTIES STATIC_LIBRARY_OPTIONS 
+      "${CMAKE_LINK_DEF_FILE_FLAG}${CMAKE_CURRENT_BINARY_DIR}/${LIBOMPIMP_GENERATED_DEF_FILE_IMP}"
+  )
+  add_dependencies(ompimp libomp-needed-headers)
+  
+ 
 endif()
 
 # Building the Fortran module files
 # One compilation step creates both omp_lib.mod and omp_lib_kinds.mod
 if(${LIBOMP_FORTRAN_MODULES})
   configure_file(${LIBOMP_INC_DIR}/omp_lib.h.var omp_lib.h @ONLY)
   configure_file(${LIBOMP_INC_DIR}/omp_lib.f90.var omp_lib.f90 @ONLY)
   # Workaround for gfortran to build modules with the

 endif()
 add_dependencies(libomp-micro-tests libomp-test-deps)
 
 # Install rules
 # We want to install libomp in ${DESTDIR}/${CMAKE_INSTALL_FULL_LIBDIR}
 # We want to install headers in ${DESTDIR}/${CMAKE_INSTALL_FULL_INCLUDEDIR}
 if(WIN32)
   install(TARGETS omp RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}")
-  install(TARGETS ${LIBOMP_IMP_LIB_TARGET} ARCHIVE DESTINATION "${OPENMP_INSTALL_LIBDIR}")
+  install(TARGETS ompimp ARCHIVE DESTINATION "${OPENMP_INSTALL_LIBDIR}")
   # Create aliases (regular copies) of the library for backwards compatibility
   set(LIBOMP_ALIASES "libiomp5md")
   foreach(alias IN LISTS LIBOMP_ALIASES)
     install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E copy \"${LIBOMP_LIB_FILE}\"
       \"${alias}${LIBOMP_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \"${CMAKE_INSTALL_FULL_BINDIR}\")")
     extend_path(outdir "${CMAKE_INSTALL_PREFIX}" "${OPENMP_INSTALL_LIBDIR}")
     install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E copy \"${LIBOMP_IMP_LIB_FILE}\"
       \"${alias}${CMAKE_STATIC_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \"${outdir}\")")

openmp/runtime/tools/generate-def.pl

foreach my $entry ( keys( %$entries ) ) {
             }; # if
         }; # if
     }; # foreach
 
     return %$entries;
 
 }; # sub process
 
-sub generate_output(\%$) {
+sub generate_output(\%$\%) {
 
-    my ( $entries, $output ) = @_;
+    my ( $entries, $output, $defs ) = @_;
+    my $lib = %$defs {'NAME'};
     my $bulk;
 
-    $bulk = "EXPORTS\n";
+    if (defined($lib)) {
+        $bulk = sprintf("NAME %s\n", $lib);
+    }
+    $bulk .= sprintf("EXPORTS\n");
     foreach my $entry ( sort( keys( %$entries ) ) ) {
         if ( not $entries->{ $entry }->{ obsolete } ) {
             $bulk .= sprintf( "    %-40s ", $entry );
             my $ordinal = $entries->{ $entry }->{ ordinal };
             if ( defined( $ordinal ) ) {
                 if ( $ordinal eq "DATA" ) {
                     $bulk .= "DATA";
                 } else {
+                    if (not %$defs {'NOORDINALS'}) {
                         $bulk .= "\@" . $ordinal;
+                    }
                 }; # if
             }; # if
             $bulk .= "\n";
         }; # if
     }; # foreach
     if ( defined( $output ) ) {
         write_file( $output, \$bulk );
     } else {

 $input = shift( @ARGV );
 
 #
 # Work.
 #
 
 my %data = parse_input( $input, %defs );
 %data = process( %data );
-generate_output( %data, $output );
+generate_output( %data, $output, %defs );
 exit( 0 );
 
 __END__
 
 #
 # Embedded documentation.
 #