node icon indicating copy to clipboard operation
node copied to clipboard

ICU build issue with clang-cl

Open targos opened this issue 5 years ago • 7 comments

I'm experimenting with Visual Studio's Clang support and have an issue with ICU:

There's only one line with an error while building tools\icu\icudata.vcxproj

  ..\..\out\Release\\obj\global_intermediate\icudt67l_dat.obj: unknown machine: 0

/cc @srl295

Diff to get there
diff --git a/deps/zlib/adler32_simd.c b/deps/zlib/adler32_simd.c
index f8b07297b93..d5b63727a48 100644
--- a/deps/zlib/adler32_simd.c
+++ b/deps/zlib/adler32_simd.c
@@ -50,7 +50,7 @@
 #define NMAX 5552
 
 #if defined(ADLER32_SIMD_SSSE3)
-#ifndef __GNUC__
+#if !defined(__GNUC__) && !defined(__clang__)
 #define __attribute__()
 #endif
 
diff --git a/deps/zlib/crc32_simd.c b/deps/zlib/crc32_simd.c
index 27481847e97..4c5ab42701a 100644
--- a/deps/zlib/crc32_simd.c
+++ b/deps/zlib/crc32_simd.c
@@ -8,7 +8,7 @@
 #include "crc32_simd.h"
 
 #if defined(CRC32_SIMD_SSE42_PCLMUL)
-#ifndef __GNUC__
+#if !defined(__GNUC__) && !defined(__clang__)
 #define __attribute__()
 #endif
 
diff --git a/deps/zlib/crc_folding.c b/deps/zlib/crc_folding.c
index 54f4b5c9401..1e4ec5de64c 100644
--- a/deps/zlib/crc_folding.c
+++ b/deps/zlib/crc_folding.c
@@ -21,9 +21,11 @@
 #include <inttypes.h>
 #include <emmintrin.h>
 #include <immintrin.h>
+#include <smmintrin.h>
+#include <tmmintrin.h>
 #include <wmmintrin.h>
 
-#ifndef __GNUC__
+#if !defined(__GNUC__) && !defined(__clang__)
 #define __attribute__()
 #endif
 
diff --git a/tools/gyp/pylib/gyp/generator/msvs.py b/tools/gyp/pylib/gyp/generator/msvs.py
index 09abadb1bcb..1c960aefca3 100644
--- a/tools/gyp/pylib/gyp/generator/msvs.py
+++ b/tools/gyp/pylib/gyp/generator/msvs.py
@@ -2799,7 +2799,7 @@ def _GetMSBuildLocalProperties(msbuild_toolset):
   if msbuild_toolset:
     properties = [
         ['PropertyGroup', {'Label': 'Locals'},
-          ['PlatformToolset', msbuild_toolset],
+          ['PlatformToolset', 'ClangCL'],
         ]
       ]
   return properties

targos avatar Jul 04 '20 15:07 targos

cc @nodejs/i18n

gengjiawen avatar Jul 10 '20 08:07 gengjiawen

@targos have not heard of this, may be good to file an ICU bug upstream at https://unicode-org.atlassian.net // @jefgen

srl295 avatar Jul 10 '20 15:07 srl295

may have to fall back to .c file builds for data if the assembly generayion is getting confused

srl295 avatar Jul 10 '20 15:07 srl295

I am not familiar with icu, which part involved into this ? Is unknown machine: 0 comes from icu code or clang ?

gengjiawen avatar Jul 20 '20 03:07 gengjiawen

I think the problem comes from this file: https://github.com/nodejs/node/blob/a51436cbea0d216f260453321a15fdce72ee28d3/deps/icu-small/source/tools/toolutil/pkg_genc.cpp

During compilation, the following printf: https://github.com/nodejs/node/blob/a51436cbea0d216f260453321a15fdce72ee28d3/deps/icu-small/source/tools/toolutil/pkg_genc.cpp#L1156

  genccode: using architecture cpu=0 bits=64 big-endian=0

targos avatar Jul 20 '20 06:07 targos

The value of 0 for cpu comes from here: https://github.com/nodejs/node/blob/a51436cbea0d216f260453321a15fdce72ee28d3/deps/icu-small/source/tools/toolutil/pkg_genc.cpp#L787

From the comment in the file:

// link.exe will link an IMAGE_FILE_MACHINE_UNKNOWN data-only .obj file
// no matter what architecture it is targeting (though other values are
// required to match). Unfortunately, the variable name decoration/mangling
// is slightly different on x86, which means we can't use the UNKNOWN type
// for all architectures though.

I wonder if maybe the linker for clang doesn't handle IMAGE_FILE_MACHINE_UNKNOWN the same as the linker for MSVC...?

IIRC, the reason IMAGE_FILE_MACHINE_UNKNOWN was set, was to produce a generic .obj file that could be used for both x64 or ARM64, in order to support cross-compiling for ARM64 on x64.

If you don't need to cross-compile ARM64 (on a x64 host) then perhaps this could be changed to set an explicit CPU arch when building with clang.

Perhaps something like:

#elif U_PLATFORM_HAS_WIN32_API
        // Windows always runs in little-endian mode.
        *pIsBigEndian = FALSE;

        // Note: The various _M_<arch> macros are predefined by the MSVC compiler based
        // on the target compilation architecture.
        // https://docs.microsoft.com/cpp/preprocessor/predefined-macros

        // link.exe will link an IMAGE_FILE_MACHINE_UNKNOWN data-only .obj file
        // no matter what architecture it is targeting (though other values are
        // required to match). Unfortunately, the variable name decoration/mangling
        // is slightly different on x86, which means we can't use the UNKNOWN type
        // for all architectures though.
#   if defined(_M_IX86)
        *pCPU = IMAGE_FILE_MACHINE_I386;
#   else
#ifndef (__clang__)
        // the linker for clang-cl doesn't work with unknown.
        *pCPU = IMAGE_FILE_MACHINE_UNKNOWN;
#elif defined(_M_AMD64)
        *pCPU = IMAGE_FILE_MACHINE_AMD64;
#endif /* ifndef (__clang__) */
#   endif

jefgen avatar Jul 20 '20 07:07 jefgen

If you don't need to cross-compile ARM64 (on a x64 host)

Unfortunately, we do cross-compile ARM64 on x64 hosts.

targos avatar May 07 '24 15:05 targos