Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICU build issue with clang-cl #34201

Closed
targos opened this issue Jul 4, 2020 · 7 comments · Fixed by #54655
Closed

ICU build issue with clang-cl #34201

targos opened this issue Jul 4, 2020 · 7 comments · Fixed by #54655
Labels
build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation. windows Issues and PRs related to the Windows platform.

Comments

@targos
Copy link
Member

targos commented Jul 4, 2020

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 targos added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Jul 4, 2020
@gengjiawen
Copy link
Member

cc @nodejs/i18n

@srl295
Copy link
Member

srl295 commented Jul 10, 2020

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

@srl295
Copy link
Member

srl295 commented Jul 10, 2020

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

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Jul 10, 2020
@gengjiawen
Copy link
Member

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

@targos
Copy link
Member Author

targos commented Jul 20, 2020

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:

printf("genccode: using architecture cpu=%hu bits=%hu big-endian=%d\n", cpu, bits, makeBigEndian);

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

@jefgen
Copy link

jefgen commented Jul 20, 2020

The value of 0 for cpu comes from here:

// link.exe will link an IMAGE_FILE_MACHINE_UNKNOWN data-only .obj file

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

@targos
Copy link
Member Author

targos commented May 7, 2024

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

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

StefanStojanovic added a commit to JaneaSystems/node that referenced this issue Aug 30, 2024
This uses the previously backported ICU fix needed for compiling with ClangCL.

Refs: nodejs#54502
Fixes: nodejs#34201
StefanStojanovic added a commit to JaneaSystems/node that referenced this issue Aug 30, 2024
This uses the backported ICU fix needed for compiling with ClangCL.

Refs: nodejs#54502
Fixes: nodejs#34201
StefanStojanovic added a commit to JaneaSystems/node that referenced this issue Aug 30, 2024
This uses the backported ICU fix needed for compiling with ClangCL.

Refs: nodejs#54502
Fixes: nodejs#34201
StefanStojanovic added a commit to JaneaSystems/node that referenced this issue Aug 30, 2024
This uses the backported ICU fix needed for compiling with ClangCL.

Refs: nodejs#54502
Fixes: nodejs#34201
nodejs-github-bot pushed a commit that referenced this issue Sep 9, 2024
This uses the backported ICU fix needed for compiling with ClangCL.

Refs: #54502
Fixes: #34201
PR-URL: #54655
Refs: #52809
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Steven R Loomis <[email protected]>
RafaelGSS pushed a commit that referenced this issue Sep 16, 2024
This uses the backported ICU fix needed for compiling with ClangCL.

Refs: #54502
Fixes: #34201
PR-URL: #54655
Refs: #52809
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Steven R Loomis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants