Proposed fixes for building with CMake and Clang-cl on Windows with HAVE_BIN=OFF

[MantisClone]

Proposed fixes:

1. Disable intrinsic mapping when compiling with clang-cl.

openfst/src/include/fst/compat.h

#ifdef _MSC_VER
const char* basename(const char* path);
#ifndef __clang__
#include <intrin.h>
#define __builtin_popcount __popcnt

#ifdef _M_X64
// Using 64-bit MSVC intrinsics.
#define __builtin_popcountll __popcnt64
inline unsigned int __builtin_ctzll(std::uint64_t w) {
  unsigned long v;
  return _BitScanForward64(&v, std::uint32_t(w)) ? v : 0;
}
#else
// Using 32-bit MSVC intrinsics.
inline unsigned int __builtin_popcountll(std::uint64_t w) {
  return __popcnt(std::uint32_t(w)) + __popcnt(std::uint32_t(w >> 32));
}
inline unsigned int __builtin_ctzll(std::uint64_t w) {
  unsigned long v;
  return (_BitScanForward(&v, std::uint32_t(w)) ? v :
          _BitScanForward(&v, std::uint32_t(w >> 32)) ? v + 32 : 0);
}
#endif  // _M_X64
#endif  // __clang__
#endif  // _MSC_VER

2. Fix fstspecial-bin install logic

openfst/src/extensions/special/CMakeLists.txt

set(FST_SPECIAL_INSTALL_TARGETS fstspecial)
if(HAVE_BIN)
  list(APPEND FST_SPECIAL_INSTALL_TARGETS fstspecial-bin)
endif()

install(TARGETS ${FST_SPECIAL_INSTALL_TARGETS}
  LIBRARY DESTINATION lib
  RUNTIME DESTINATION bin
  ARCHIVE DESTINATION lib
)

3. Check for MSVC compiler instead of WIN32 OS

openfst/CMakeLists.txt

if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
  add_definitions(/bigobj)
  set(WHOLEFST "/WHOLEARCHIVE:fst")
  set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${WHOLEFST}")
  #set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS 1)
  #this must be disabled unless the previous option (CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS) is enabled
  option(BUILD_SHARED_LIBS "Build shared libraries" OFF)
else()
  option(BUILD_SHARED_LIBS "Build shared libraries" ON)
endif()

[jtrmal]

lgtm

On Thu, Nov 12, 2020 at 10:21 PM David Hunt-Mateo notifications@github.com wrote:

Proposed fixes:

1. Disable intrinsic mapping when compiling with clang-cl.

openfst/src/include/fst/compat.h

ifdef _MSC_VERconst char basename(const char path);

ifndef clang

include

define __builtin_popcount __popcnt

ifdef _M_X64// Using 64-bit MSVC intrinsics.

define __builtin_popcountll popcnt64inline unsigned int builtin_ctzll(std::uint64_t w) {

unsigned long v; return _BitScanForward64(&v, std::uint32_t(w)) ? v : 0; }

else// Using 32-bit MSVC intrinsics.inline unsigned int __builtin_popcountll(std::uint64_t w) {

return popcnt(std::uint32_t(w)) + __popcnt(std::uint32_t(w >> 32)); }inline unsigned int builtin_ctzll(std::uint64_t w) { unsigned long v; return (_BitScanForward(&v, std::uint32_t(w)) ? v : _BitScanForward(&v, std::uint32_t(w >> 32)) ? v + 32 : 0); }

endif // _M_X64

endif // clang

endif // _MSC_VER

1. Fix fstspecial-bin install logic

openfst/src/extensions/special/CMakeLists.txt

set(FST_SPECIAL_INSTALL_TARGETS fstspecial)if(HAVE_BIN) list(APPEND FST_SPECIAL_INSTALL_TARGETS fstspecial-bin)endif() install(TARGETS ${FST_SPECIAL_INSTALL_TARGETS} LIBRARY DESTINATION lib RUNTIME DESTINATION bin ARCHIVE DESTINATION lib )

1. Check for MSVC compiler instead of WIN32 OS

openfst/CMakeLists.txt

if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") add_definitions(/bigobj) set(WHOLEFST "/WHOLEARCHIVE:fst") set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${WHOLEFST}")

set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS 1)

this must be disabled unless the previous option (CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS) is enabled

option(BUILD_SHARED_LIBS "Build shared libraries" OFF)else() option(BUILD_SHARED_LIBS "Build shared libraries" ON)endif()

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/35, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUKYX4L2GZZQCQU65AGKALSPSQ4LANCNFSM4TUCLW6A .

[kkm000]

Could you please make a PR? A couple questions.

1. Does clang-cl have __builtin_popcount, __builtin_popcountll and __builtin_ctzll always defined, no header required? Or are they defined just because some other header included them? I'm kinda subscribe to IWYU philosophy.

2. Regarding (3), the dependencies looks a bit entangled. First, the BUILD_SHARED_LIBS default is already kinda quirky. I'd prefer it be the same regardless of the OS. Second, something like /WHOLEARCHIVE:fst should still be required for shared libraries. I do not know if CMake knows and generalizes that setting, but for gcc something like -Wl,--whole-archive -lfst -Wl,--no-whole-archive should also be needed (I think clang is ok to pass this flag to the linker, but I have no idea what liker is used in the MSVC toolchain). Without this, FST types (like StdVectorFst) would not be readable, as their reading code is not referenced directly. And this whole thing should better be conditioned on BUILD_SHARED_LIBS, neither on the OS API nor the compiler.

3. /bigobj is required for the PE format for the shared build. This is needed, IIRC, when building a DLL with C7 debug information, but the reason is not important. One way or another, clang or not, if a PE native Windows executable is being built, something, somehow has to tell the linker to expect more than 0xFFF0 sections.

[jtrmal]

I might not remember well, but the /bigobj was always linked without any issues (and without any additional flags). y.

On Sun, Nov 15, 2020 at 5:28 PM kkm000 notifications@github.com wrote:

Could you please make a PR? A couple questions.

1.

Does clang-cl have builtin_popcount, __builtin_popcountll and builtin_ctzll always defined, no header required? Or are they defined just because some other header included them? I'm kinda subscribe to IWYU philosophy. 2.

The dependency looks a bit messy. First, the BUILD_SHARED_LIBS default is already kinda quirky. I'd prefer it be the same regardless of the OS. Second, something like /WHOLEARCHIVE:fst should still be required for shared libraries. I do not know if CMake knows and generalizes that setting, but for gcc something like -Wl,--whole-archive -lfst -Wl,--no-whole-archive should also be needed (I think clang is ok to pass this flag to the linker, but I have no idea what liker is used in the MSVC toolchain. Without this, FST types (like StdVectorFst) would not be readable, as their reading code is not referenced directly. And this whole thing should better be conditioned on BUILD_SHARED_LIBS, neither on the OS API nor the compiler. 3.

/bigobj is required for the PE format for the shared build. This is needed, IIRC, when building a DLL with C7 debug information, but the reason is not important. One way or another, clang or not, if a PE native Windows executable is being built, something, somehow has to tell the linker to expect more than 0xFFF0 sections.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/35#issuecomment-727648082, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUKYX324BYEKQI2GE337S3SQBIXVANCNFSM4TUCLW6A .

[MantisClone]

@kkm000 Thanks for the feedback. Yes, I can create a PR, but I'll wait until I've worked through all the questions you've posed.

[kkm000]

@DMats, thanks a whole lot! Implementing this stuff correctly is hard, and takes some research.

@jtrmal Do you mean adding /bigobj as it's currently done (under if (WIN32)) works with clang-cl?

[jtrmal]

oh, I don't know anything about clang-cl (except it exists), sorry if I mislead you. y.

On Mon, Nov 16, 2020 at 3:20 PM kkm000 notifications@github.com wrote:

@DMats https://github.com/DMats, thanks a whole lot! Implementing this stuff correctly is hard, and takes some research.

@jtrmal--Do you mean adding /bigobj as it's currently done (under if (WIN32)) works with clang-cl?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/35#issuecomment-728301824, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACUKYXYS6FH4MLCXRLFZ4XTSQGCQ7ANCNFSM4TUCLW6A .

[MantisClone]

PR #36 created. I chose not to change anything related to /bigobj or BUILD_SHARED_LIBS because I'm only building static libs.