google / highway

Performance-portable, length-agnostic SIMD with runtime dispatch
Apache License 2.0
4.13k stars 315 forks source link

Unable to build shared library on Windows (MinGW-w64) #2225

Closed brechtsanders closed 4 months ago

brechtsanders commented 4 months ago

When building highway 1.2.0 on Windows using MinGW-w64 the shared build fails with the following error:

hwy/contrib/thread_pool/topology.cc:67:20: error: function 'bool hwy::HaveThreadingSupport()' definition is marked dllimport

When looking at this section in hwy/highway_export.h:

#if !defined(HWY_SHARED_DEFINE)
#define HWY_DLLEXPORT
#define HWY_CONTRIB_DLLEXPORT
#define HWY_TEST_DLLEXPORT
#else  // !HWY_SHARED_DEFINE

#ifndef HWY_DLLEXPORT
#if defined(hwy_EXPORTS)
/* We are building this library */
#ifdef _WIN32
#define HWY_DLLEXPORT __declspec(dllexport)
#else
#define HWY_DLLEXPORT __attribute__((visibility("default")))
#endif
...

and this section in CMakeLists.txt:

if("${HWY_LIBRARY_TYPE}" STREQUAL "SHARED")
  set(DLLEXPORT_TO_DEFINE "HWY_SHARED_DEFINE")
else()
  set(DLLEXPORT_TO_DEFINE "HWY_STATIC_DEFINE")
endif()

it's clear that set(DLLEXPORT_TO_DEFINE "HWY_SHARED_DEFINE") causes HWY_DLLEXPORT to be defined as nothing instead of __declspec(dllexport).

I was able to build successfully after changing CMakeLists.txt so HWY_SHARED_DEFINE is not defined.

vtorri commented 4 months ago

same here

vtorri commented 4 months ago

when I compile with verbose mode, what is passed to compiler is -Dhwy_contrib_EXPORTS not -Dhwy_EXPORTS. So, according to highway_export.h, HWY_DLLEXPORT is defined to __declspec(dllimport)

brechtsanders commented 4 months ago

That is only true if HWY_SHARED_DEFINE is not defined, because if it is HWY_DLLEXPORT is empty, see the code from hwy/highway_export.h I mentioned above.

kmilos commented 4 months ago

https://github.com/google/highway/pull/413 would still be nice to have 😉

jan-wassenberg commented 4 months ago

Apologies, I used the wrong macro: it should have been HWY_CONTRIB_DLLEXPORT for anything in hwy/contrib. Fixing shortly.

That aside, I'm not sure it actually makes sense to use shared libraries for Highway. There's not a lot of code that can actually go into the .so/.dll, and maybe it's more trouble than it is worth?

jan-wassenberg commented 4 months ago

@kmilos we can certainly add the LANGUAGES CXX to CMakelists, I'm not sure the other prefix change is still relevant because we now instead set RUNTIME_OUTPUT_DIRECTORY?

kmilos commented 4 months ago

I was suggesting at MSYS2 in the CI in general...

jan-wassenberg commented 4 months ago

hm, when we last tried enabling it, there were numerous issues. Do you have any statistics on how many projects are using Highway with MSYS/MinGW? Would anyone be willing to help with these and future issues?

kmilos commented 4 months ago

Do you have any statistics on how many projects are using Highway with MSYS/MinGW?

So far libjxl and libvips depend on it directly in the MSYS2 repo https://packages.msys2.org/package/mingw-w64-ucrt-x86_64-highway?repo=ucrt64 and then all of their clients. It's hard to tell who else uses it indirectly, outside the repo...