ABI issues + link error on MinGW target

[nu774]

Hi, found several issues on MinGW target. Patch is attached.

1. Build with OpenMP results in link time error, since gcc requires -fopenmp on link time but it is not set. This can be fixed by setting target LINK_FLAGS property to $OpenMP_C_FLAGS, but I don't know if it's appropriate for other compilers/linkers than GNU C. It seems at least OK for MSVC (Although MSVC linker doesn't need -openmp switch, it's just gracefully ignored with a warning).
   
2. Found several ABI weakness on MinGW DLL. At first, GNUC assumes 16byte stack frame alignment by dafault, but it's not assured by x86 ABI, and MSVC doesn't align stack frames as such. As a result, calling SIMD enabled function in MinGW DLL from MSVC program results in segfaults, due to stack frame misalignment. This can be avoided by setting -mincoming-stack-boundary=2.
   
3. Secondly, calling convention for function returning structure of Mingw-GCC changed at version 4.7, and it breaks compatibility. Read https://sourceforge.net/projects/mingw/files/MinGW/Base/gcc/Version4/gcc-4.7.2-1/ for details. Since fixing this breaks API, I don't think fixing this is a MUST.
   

The following is the patch I tried.

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 2a21156..cc83a27 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -28,6 +27,9 @@ endif ()

 if (HAVE_SIMD)
   set (SIMD_SOURCES rate32s ${RDFT32S} simd)
+  if (CMAKE_COMPILER_IS_GNUCC)
+    set (SIMD_C_FLAGS "${SIMD_C_FLAGS} -mincoming-stack-boundary=2")
+  endif ()
   foreach (source ${SIMD_SOURCES})
     set_property (SOURCE ${source} PROPERTY COMPILE_FLAGS ${SIMD_C_FLAGS})
   endforeach ()
@@ -45,6 +47,18 @@ set_target_properties (${PROJECT_NAME} PROPERTIES
   INSTALL_NAME_DIR ${LIB_INSTALL_DIR}
   LINK_INTERFACE_LIBRARIES ""
   PUBLIC_HEADER "${PROJECT_NAME}.h")
+
+if (WITH_OPENMP)
+    get_target_property(_existing ${PROJECT_NAME} LINK_FLAGS)
+    if (_existing)
+        set (TARGET_LD_FLAGS "${_existing} ${OpenMP_C_FLAGS}")
+    else ()
+        set (TARGET_LD_FLAGS "${OpenMP_C_FLAGS}")
+    endif ()
+    set_target_properties(${PROJECT_NAME} PROPERTIES
+        LINK_FLAGS "${TARGET_LD_FLAGS}")
+endif()
+
 if (BUILD_FRAMEWORK)
   set_target_properties (${PROJECT_NAME} PROPERTIES FRAMEWORK TRUE)
 elseif (NOT WIN32)

[nu774]

Hmm, setting -mincomming-stack-boundary=2 on x86_64 target seems to result in compile error.
More finer condition is required by investigating target arch (we only need it for 32bit x86 target).

[nu774]

Updated patch.

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 2a21156..ff40e1b 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -28,6 +27,13 @@ endif ()

 if (HAVE_SIMD)
   set (SIMD_SOURCES rate32s ${RDFT32S} simd)
+  if (CMAKE_COMPILER_IS_GNUCC)
+    include (CheckCCompilerFlag)
+    CHECK_C_COMPILER_FLAG("-mincoming-stack-boundary=2" HAVE_STACK_BOUNDARY_2)
+    if (HAVE_STACK_BOUNDARY_2)
+      set (SIMD_C_FLAGS "${SIMD_C_FLAGS} -mincoming-stack-boundary=2")
+    endif ()
+  endif ()
   foreach (source ${SIMD_SOURCES})
     set_property (SOURCE ${source} PROPERTY COMPILE_FLAGS ${SIMD_C_FLAGS})
   endforeach ()
@@ -45,6 +51,18 @@ set_target_properties (${PROJECT_NAME} PROPERTIES
   INSTALL_NAME_DIR ${LIB_INSTALL_DIR}
   LINK_INTERFACE_LIBRARIES ""
   PUBLIC_HEADER "${PROJECT_NAME}.h")
+
+if (WITH_OPENMP)
+    get_target_property(_existing ${PROJECT_NAME} LINK_FLAGS)
+    if (_existing)
+        set (TARGET_LD_FLAGS "${_existing} ${OpenMP_C_FLAGS}")
+    else ()
+        set (TARGET_LD_FLAGS "${OpenMP_C_FLAGS}")
+    endif ()
+    set_target_properties(${PROJECT_NAME} PROPERTIES
+        LINK_FLAGS "${TARGET_LD_FLAGS}")
+endif()
+
 if (BUILD_FRAMEWORK)
   set_target_properties (${PROJECT_NAME} PROPERTIES FRAMEWORK TRUE)
 elseif (NOT WIN32)

[robs]

Hello and thanks for these suggestions. For the first issue, please can you let me know if the following patch works for you?

diff --git a/cmake/Modules/FindSIMD.cmake b/cmake/Modules/FindSIMD.cmake
index 6484bbd..6ac51cb 100644
--- a/cmake/Modules/FindSIMD.cmake
+++ b/cmake/Modules/FindSIMD.cmake
@@ -40,12 +40,18 @@
include (CheckCSourceCompiles)
include (FindPackageHandleStandardArgs)

+if (WIN32) # Safety for when mixed lib/app compilers (but performance hit)
+ set (GCC_WIN32_SIMD_OPTS "-mincoming-stack-boundary=2")
+endif ()
+
set (SIMD_C_FLAG_CANDIDATES
- # Microsoft Visual Studio x64
+ # x64
" "
# Microsoft Visual Studio x86
"/arch:SSE /fp:fast -DSSE"
- # Gnu
+ # Gcc x86
+ "-msse -mfpmath=sse ${GCC_WIN32_SIMD_OPTS}"
+ # Gcc x86 (old versions)
"-msse -mfpmath=sse"
)

[robs]

and maybe this for the second issue:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 8a24952..7bfcd97 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -79,6 +79,9 @@ endif ()
if (OPENMP_FOUND)
set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${OpenMP_C_FLAGS}")
set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${OpenMP_EXE_LINKER_FLAGS}")
+ if (MINGW)
+ set (CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${OpenMP_C_FLAGS}")
+ endif ()
endif ()

if (WITH_SIMD)

[nu774]

Thanks, it works fine here.
And sorry for patch formatting. Since all whitespaces at line beginning are stripped off, copy & paste doesn't seem to work.

One more thing. On i686-w64-mingw target, __divdi3() is imported from libgcc.
Requiring extra libgcc DLL for merely one function looks somewhat wasteful.
Is it possible to set -static-libgcc for MinGW target?