skeleton_test fails to build with MSVC

[yzazik]

Hello,

First, I know MSVC hasn't been the first choice running fast, but starting from VS2022, the codegen is not nearly as bad as it was.
Second, I can use GCC or Clang (clang-cl or llvm) to build hwy but I still mostly use it as header only one and if I want to build non-header part I would probably need to patch and rebuild half of the world in our project as well.

Now, I'm trying to add new ops to HWY for [currently unsupported] NPU and for that building tests is essential.
I stumbled on this skeleton_test, which is the only test executable that tries to export something.
Currently it fails with:

  skeleton_test.cc
  skeleton.cc
  Generating Code...
C:\github\highway\hwy\examples\skeleton.cc(111,20): error C2491: 'skeleton::CallFloorLog2': definition of dllimport fun
ction not allowed [C:\github\highway\out\release\build\skeleton_test.vcxproj]
C:\github\highway\hwy\examples\skeleton.cc(120,20): error C2491: 'skeleton::SavedCallFloorLog2': definition of dllimpor
t function not allowed [C:\github\highway\out\release\build\skeleton_test.vcxproj]

From the looks of it, hwy has this HWY_SHARED_DEFINE as PUBLIC, means it would also be propagated to test executables which should export but there is no hwy_EXPORTS defined for it so it falls back to dllimport.

[jan-wassenberg]

Hi, is it an option to use static linking instead of shared libraries? That would be simplest, and is the much more heavily used path.
If not, I don't have experience with MSVC because we actually build/test using clang-cl and emulators, but we'd have to arrange for the build system to define hwy_EXPORTS.

[yzazik]

Ok, let me start over, I just need to understand the intent and explain this more.

Not the test code [per se], but [rather] an example code (skeleton_test is in examples),
gets built as an executable (not library) and trying to export a couple of functions.
AFAIK, exports in executable are only usable in Windows since only in Windows
you can call LoadLibrary() and GetProcAddress() for exports in executable.
Equivalent dlopen()/dlsym() generally won't work on executables in a GNU world.

Best guess, the code wasn't meant to export anything when built as executable
and it's using a wrong EXPORT symbol (ie hwy_EXPORTS) there since it's only meant for hwy target.
The export symbol should have been expanded into nothing when built as
embedded source (for executable) or static lib.
The hint would be this part in the top level CMakeLists.txt (key word is library):

# The skeleton test uses the skeleton library code.
target_sources(skeleton_test PRIVATE hwy/examples/skeleton.cc)

So the proper fix for that would be to build skeleton.cc as a separate library that would have
it's own skeleton_EXPORTS (in DLL case) and have it's own HW_SKELETON_DLLEXPORT for exports.
And then add dependency for skeleton_test executable on skeleton library.

Again, this is not a compiler issue, this is a platform issue.
If you want to build a DLL in most cases you would still need to go with
Microsoft C++ extension for __declspec(), but it does not matter which compiler you use.
But sure, let's take clang-cl, it would fail here as any other compiler.

And sure, if I would build hwy as static lib it won't be a problem because skeleton code
depends on hwy library type and for hwy static lib there would be no __declspec() expanded,
but the point is that skeleton should not depend on the hwy library type and
if this would be taken/accepted as a valid statement then for hwy DLL it fails.

[jan-wassenberg]

I see. Ideally apps would indeed not depend on the Highway static vs dynamic type, and even better, we should not have added dynamic library support in the first place. In retrospect, it has not been worth the trouble.
What's your proposed way forward, in light of that? Using static Hwy would be fine, right?

[yzazik]

Oh no, dynamic library support should not be dropped.
Half of the world won't be happy, myself including. Most of the things in our project born to be dynamic.
Often there is just no other way, especially isolating different versions of the same library in one executable.

Well, there is a quick fix and there is a proper fix.
A quick fix would be to create another HWY_EXAMPLE_DLLEXPORT.
And unlike the others (HWY_DLL_EXPORT, HWY_TEST_DLLEXPORT, HWY_CONTRIB_DLLEXPORT)
make it independent from HWY_SHARED_DEFINE, and use it in the skeleton.cc instead.

Proper fix would be to rework highway_export.h for each area or even split it to several files
and have two control defines for each of those areas:
${target}_EXPORTS (this one already exist and added by CMake for dynamic libs)
and another one ${target}_IMPORTS (to be set manually).
Let's just have it for two areas to shorten this post:

#if defined _WIN32 || defined __CYGWIN__ || defined __MSYS__
#	if defined hwy_EXPORTS		// build dynamic library (export things)
#		define HWY_API			__declspec(dllexport)
#	elif defined hwy_DLL		// use dynamic library (use exports)
#		define HWY_API			__declspec(dllimport)
#	else						// static lib or embedded source
#		define HWY_API
#	endif
#else	// __linux__, ANDROID, __APPLE__
#	if defined hwy_EXPORTS		// build shared object (ideally with -fvisibility=hidden)
#		define HWY_API			__attribute__ ((visibility ("default")))
#	elif defined hwy_DLL		// use shared object (keep same visibility)
#		define HWY_API			__attribute__ ((visibility ("default")))
#	else						// static lib or embedded source
#		define HWY_API
#	endif
#endif

#if defined _WIN32 || defined __CYGWIN__ || defined __MSYS__
#	if defined hwy_contrib_EXPORTS		// build dynamic library (export things)
#		define HWY_CONTRIB_API			__declspec(dllexport)
#	elif defined hwy_contrib_DLL		// use dynamic library (use exports)
#		define HWY_CONTRIB_API			__declspec(dllimport)
#	else								// static lib or embedded source
#		define HWY_CONTRIB_API
#	endif
#else	// __linux__, ANDROID, __APPLE__
#	if defined hwy_contrib_EXPORTS		// build shared object (ideally with -fvisibility=hidden)
#		define HWY_CONTRIB_API			__attribute__ ((visibility ("default")))
#	elif defined hwy_contrib_DLL		// use shared object (keep same visibility)
#		define HWY_CONTRIB_API			__attribute__ ((visibility ("default")))
#	else								// static lib or embedded source
#		define HWY_CONTRIB_API
#	endif
#endif

This may look like something that is already there, the difference here is that all those
${target}_APIs do not depend on the same HWY_SHARED_DEFINE but rather by its own ${target}_EXPORTS.

So again, ${target}_EXPORTS would be added by CMake automatically for dynamic libraries and [important!]
unlike HWY_SHARED_DEFINE it's added as PRIVATE (since it's only useful for building own ${target} library).
${target}_DLL needs to be added manually as INTERFACE one
(since should only be used by others using this library) and only add it when library type is dynamic.
Generic code for setting those usually looks like this:

get_target_property(${target}_TYPE ${target} TYPE)
if ("${${target}_TYPE}" STREQUAL "SHARED_LIBRARY")
	set_target_properties(${target} PROPERTIES
		CXX_VISIBILITY_PRESET			hidden			# hide everything but exports
		INTERFACE_COMPILE_DEFINITIONS	${target}_DLL	# declare imports for others
	)
endif()

This intentionally does not use if (BUILD_SHARED_LIBS) as the library may have been added with explicit
type (SHARED or STATIC) that could be different from one that's based on global BUILD_SHARED_LIBS.
When building static (or exe) none of the two (${target}_EXPORTS and ${target}_DLL)
would be set, so the ${target}_API would expand into nothing which is what we need for skeleton.
skeleton.cc is part of examples, so should still use [separate area] hwy_example_EXPORTS.