ICU-22787 Fix ClangCL compilation on Windows #3023

StefanStojanovic · 2024-06-03T14:12:23Z

I'm working on supporting Node.js on Windows. There is an ongoing effort to enable it to be compiled with ClangCL. While working on it I noticed an issue when creating .obj file in ICU because of the *pCPU = IMAGE_FILE_MACHINE_UNKNOWN; line, and the code I made fixes that by adding a new option. Although this is a broader problem that should be fixed for multiple compilers and platforms, my change is aimed toward ClangCL and Windows, but can be used as a good foundation to work on fixes for other platforms and compilers and make corrections in future iterations.

This issue is currently our main blocker for enabling ClangCL compilation for Node.js and it'd be greatly appreciated if it could be reviewed soon.

Refs: nodejs/node#53003

Checklist

Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22787
Required: The PR title must be prefixed with a JIRA Issue number.
Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
Required: Each commit message must be prefixed with a JIRA Issue number.
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

srl295 · 2024-06-03T15:24:15Z

#3013

roubert · 2024-06-06T19:16:13Z

icu4c/source/tools/toolutil/pkg_genc.cpp

+            } else if (strcmp(optCpuArch, "arm64") == 0) {
+                *pCPU = IMAGE_FILE_MACHINE_ARM64;
+            } else {
+                // This should never happen.


In that case, I'd say that it'd be easier to debug if the process terminated with an error message if that which should never happen ever happened anyway.

Makes sense. I've changed it.

As this code never will be executed, I recommend keeping it as simple as possible, it really doesn't need to print any custom error message that never will be printed anyway and a simple source code comment would suffice:

#include <exception> std::terminate(); // Unreachable.

I added it based on your previous comment:

In that case, I'd say that it'd be easier to debug if the process terminated with an error message if that which should never happen ever happened anyway.

Although I didn't think about it before you mentioned it, this log makes sense to me. For example, imagine someone makes a typo and passes -c X64 or -c amr64, this log would help them fix it much quicker.

For example, imagine someone makes a typo and passes -c X64 or -c amr64, this log would help them fix it much quicker.

Wait a second, your original comment here was "This should never happen." but the user making a typo on the commandline is the total opposite of that, that's something that certainly is going to happen, sooner or later.

But verifying that the commandline arguments are correct should be done already in main() in genccode.c, in the same way that the arguments to -a are being verified there by calling the checkAssemblyHeaderName() helper function.

I understand the confusion. My comment was misleading. Sorry for that. I checked the code now and saw the checkAssemblyHeaderName helper, I'll add that type of check to the -c option as well. Thanks for pointing that out. Once I add that the else branch should really never happen, and then the change you suggested with std::terminate() will be correct.

roubert · 2024-06-06T19:21:36Z

icu4c/source/tools/genccode/genccode.c

@@ -131,6 +133,7 @@ main(int argc, char* argv[]) {
            "\t-o or --object      write a .obj file instead of .c\n"
            "\t-m or --match-arch file.o  match the architecture (CPU, 32/64 bits) of the specified .o\n"
            "\t                    ELF format defaults to i386. Windows defaults to the native platform.\n"
+            "\t-c or --cpu-arch    Specify a CPU architecture for which to write a .obj file for ClangCL on Windows\n"


Wouldn't it be easier to use this tool correctly if this commandline flag was #ifdef'ed out altogether on platforms where it isn't implemented?

For now, I've only #ifdefed out the parts in genccode.c, as I didn't want to make writeObjectCode signature different depending on which compiler is used on Windows. I just pass it NULL if Clang isn't used. If you think I should make the signature #ifdefed too, let me know.

I think it would be better to leave it a generic argument, even if it is only currently valid for ClangCL. It may be needed in other situations.

roubert · 2024-06-06T19:23:30Z

icu4c/source/tools/toolutil/pkg_genc.cpp

@@ -847,7 +852,24 @@ getArchitecture(uint16_t *pCPU, uint16_t *pBits, UBool *pIsBigEndian, const char
 #   if defined(_M_IX86)
        *pCPU = IMAGE_FILE_MACHINE_I386;
 #   else
-        *pCPU = IMAGE_FILE_MACHINE_UNKNOWN;
+        // Linker for ClangCL doesn't handle IMAGE_FILE_MACHINE_UNKNOWN the same as


If I read the existing U_PLATFORM_HAS_WIN32_API block correctly it's all about Microsoft's link.exe, so it seems odd to now mix in another linker in that. Wouldn't this become easier to read if Microsoft's and LLVM's linkers got each their separate #if block?

In platform.h for U_PLATFORM_HAS_WIN32_API it says: Defines whether the Win32 API is available on the platform.. This is true for ClangCL, so I've added a new constant U_CLANG_CL to distinguish when using Clang on Windows. After that, I changed to using that constant inside the mentioned block.

Wouldn't this become easier to read if Microsoft's and LLVM's linkers got each their separate #if block

It would, but I do not know how to write linker-specific code (I'm not saying it is impossible). What can be done from my perspective is to detect the compiler (MSVC or Clang) and then infer a linker (link.exe or lld-link.exe), which is more or less what we have now after these changes.

roubert · 2024-06-06T19:23:32Z

icu4c/source/tools/toolutil/pkg_genc.cpp

+        // the target architecture.
+#       if defined(__clang__)
+            if (strcmp(optCpuArch, "x64") == 0) {
+                *pCPU = IMAGE_FILE_MACHINE_AMD64;


Are 32-bit builds not supported? It's been a long time since I last had any reason to do one, but I wasn't aware that they're no longer supported.

Makes sense. I've added a strcmp(optCpuArch, "x86") == 0 branch in case someone needs it since it is still supported.

Above this code there is:

# if defined(_M_IX86) *pCPU = IMAGE_FILE_MACHINE_I386;

but that would only apply if genccode was compiled targeting x86, which is not necessarily the case.

jira-pull-request-webhook · 2024-06-07T19:20:41Z

Notice: the branch changed across the force-push!

icu4c/source/common/unicode/platform.h is now changed in the branch
icu4c/source/tools/genccode/genccode.c is different
icu4c/source/tools/toolutil/pkg_genc.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

StefanStojanovic · 2024-06-07T19:24:55Z

Thanks for the review @roubert, I tried to address all your comments in a new commit.

I see 2 checks failed: jira-ticket and single-commit. I'll squash commits and keep only the original commit message once all is ready to land, but I kept 2 commits now for easier reviewing.

markusicu · 2024-06-07T19:39:34Z

I see 2 checks failed: jira-ticket and single-commit. I'll squash commits and keep only the original commit message once all is ready to land, but I kept 2 commits now for easier reviewing.

Please wait with squashing until after feedback is resolved.

StefanStojanovic · 2024-06-07T19:49:40Z

Please wait with squashing until after feedback is resolved.

Will do.

By the way, I see that in CI-ICU4C all MSVC builds failed. I'm not sure why that happened, but I built it locally before pushing (allinone.sln -> Build Solution) and it worked.

StefanStojanovic · 2024-06-11T17:31:59Z

Hey everyone, I wanted to check if there is anything else for me to do after my initial changes? If you think it is better to leave the new option I can every my latest changes in genccode.c. Other than that, are the other changes good? Regards.

roubert · 2024-06-12T15:00:48Z

I see that in CI-ICU4C all MSVC builds failed. I'm not sure why that happened,

The CI is currently broken, you'll just have to wait for it to get fixed (and then those tests should pass again).

roubert · 2024-06-13T12:58:52Z

The CI is currently broken,

It has now been fixed and the tests pass.

roubert · 2024-06-13T12:39:34Z

icu4c/source/common/unicode/platform.h

+ * @internal
+ */
+#if (defined(_MSC_VER) && defined(__clang__) && __clang__)
+#   define U_CLANG_CL


I'd recommend against adding a new macro to the public API for a check that's only used in one single internal tool. (That would also make the icu4c-docs-build failure disappear, without you having to actually update the documentation.)

I agree, I'll remove it.

roubert · 2024-06-13T12:51:13Z

icu4c/source/tools/toolutil/pkg_genc.cpp

+            } else if (strcmp(optCpuArch, "arm64") == 0) {
+                *pCPU = IMAGE_FILE_MACHINE_ARM64;
+            } else {
+                // This should never happen.


As this code never will be executed, I recommend keeping it as simple as possible, it really doesn't need to print any custom error message that never will be printed anyway and a simple source code comment would suffice:

#include <exception> std::terminate(); // Unreachable.

roubert · 2024-06-19T11:33:52Z

icu4c/source/tools/toolutil/pkg_genc.cpp

+            } else if (strcmp(optCpuArch, "arm64") == 0) {
+                *pCPU = IMAGE_FILE_MACHINE_ARM64;
+            } else {
+                // This should never happen.


For example, imagine someone makes a typo and passes -c X64 or -c amr64, this log would help them fix it much quicker.

Wait a second, your original comment here was "This should never happen." but the user making a typo on the commandline is the total opposite of that, that's something that certainly is going to happen, sooner or later.

But verifying that the commandline arguments are correct should be done already in main() in genccode.c, in the same way that the arguments to -a are being verified there by calling the checkAssemblyHeaderName() helper function.

jira-pull-request-webhook · 2024-06-20T10:30:09Z

Notice: the branch changed across the force-push!

icu4c/source/common/unicode/platform.h is different
icu4c/source/tools/genccode/genccode.c is different
icu4c/source/tools/toolutil/pkg_genc.cpp is different
icu4c/source/tools/toolutil/pkg_genc.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

StefanStojanovic · 2024-06-20T10:32:03Z

@roubert I pushed the changes in a new commit. I addressed your comments and made changes based on this comment. Please let me know if there is something else I should change. Regards.

roubert

OK, this all looks reasonable to me now, so I'll leave it to @rp9-next (@microsoft) to take this review from here (and verify that what this does really is what it should be doing, and that this is the preferred way of doing that, and so on).

rp9-next · 2024-08-08T08:09:46Z

@roubert , merging is blocked by a change requested check which I believe is outdated..
Could you dismiss it ?

@StefanStojanovic, please squash all the commits to a single commit for merging (you could use the squash branch tool as well)

jira-pull-request-webhook · 2024-08-08T12:48:15Z

Notice: the branch changed across the force-push!

icu4c/source/tools/pkgdata/pkgdata.cpp is different
icu4c/source/tools/toolutil/pkg_genc.cpp is different
icu4c/source/tools/toolutil/pkg_genc.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

StefanStojanovic · 2024-08-08T12:49:37Z

@StefanStojanovic, please squash all the commits to a single commit for merging (you could use the squash branch tool as well)

Thanks for the review @rp9-next. I rebased to the latest main branch and squashed commits afterward.

roubert · 2024-08-08T16:42:51Z

@roubert , merging is blocked by a change requested check which I believe is outdated.. Could you dismiss it ?

Done.

- Floating patch for ICU 75.x ICU Bug: https://unicode-org.atlassian.net/browse/ICU-22787 Backport of: unicode-org/icu#3023

- Floating patch for ICU 75.x ICU Bug: https://unicode-org.atlassian.net/browse/ICU-22787 Backport of: unicode-org/icu#3023 PR-URL: #54502 Reviewed-By: Steven R Loomis <srl295@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

- Floating patch for ICU 75.x ICU Bug: https://unicode-org.atlassian.net/browse/ICU-22787 Backport of: unicode-org/icu#3023 PR-URL: nodejs#54502 Reviewed-By: Steven R Loomis <srl295@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

StefanStojanovic changed the title ~~ICU-22787 Fix ClangCL cross-compilation on Windows~~ ICU-22787 Fix ClangCL compilation on Windows Jun 4, 2024

markusicu assigned roubert Jun 6, 2024

markusicu requested review from srl295 and markusicu June 6, 2024 16:49

roubert mentioned this pull request Jun 6, 2024

ICU-21633 Fix ClangCL cross-compilation on Windows #3013

Closed

7 tasks

roubert requested changes Jun 6, 2024

View reviewed changes

StefanStojanovic force-pushed the mefi-cross-compile-clangcl-fix branch from 1343c73 to a539dde Compare June 7, 2024 19:20

roubert requested changes Jun 13, 2024

View reviewed changes

richgillam requested a review from rp9-next June 13, 2024 15:52

richgillam assigned rp9-next Jun 13, 2024

roubert previously requested changes Jun 19, 2024

View reviewed changes

StefanStojanovic force-pushed the mefi-cross-compile-clangcl-fix branch from a539dde to d5fe469 Compare June 20, 2024 10:30

roubert reviewed Jun 20, 2024

View reviewed changes

roubert removed their assignment Jun 20, 2024

StefanStojanovic mentioned this pull request Jul 16, 2024

build node.js using clangcl when under Windows nodejs/node#52809

Open

13 tasks

rp9-next approved these changes Aug 8, 2024

View reviewed changes

rp9-next requested a review from roubert August 8, 2024 08:04

ICU-22787 Fix ClangCL compilation on Windows

d0712de

StefanStojanovic force-pushed the mefi-cross-compile-clangcl-fix branch from d5fe469 to d0712de Compare August 8, 2024 12:48

roubert removed their request for review August 8, 2024 17:03

rp9-next merged commit 66ba099 into unicode-org:main Aug 9, 2024
99 checks passed

StefanStojanovic mentioned this pull request Aug 19, 2024

deps: port ICU ClangCL fix nodejs/node#54448

Closed

StefanStojanovic added a commit to JaneaSystems/node that referenced this pull request Aug 22, 2024

deps: backport ICU-22787 to fix ClangCL on Windows

ec0852d

- Floating patch for ICU 75.x ICU Bug: https://unicode-org.atlassian.net/browse/ICU-22787 Backport of: unicode-org/icu#3023

StefanStojanovic mentioned this pull request Aug 22, 2024

deps: backport ICU-22787 to fix ClangCL on Windows nodejs/node#54502

Merged

StefanStojanovic added a commit to JaneaSystems/node that referenced this pull request Aug 23, 2024

deps: backport ICU-22787 to fix ClangCL on Windows

684cf72

- Floating patch for ICU 75.x ICU Bug: https://unicode-org.atlassian.net/browse/ICU-22787 Backport of: unicode-org/icu#3023

StefanStojanovic added a commit to JaneaSystems/node that referenced this pull request Aug 27, 2024

deps: backport ICU-22787 to fix ClangCL on Windows

36bbad5

- Floating patch for ICU 75.x ICU Bug: https://unicode-org.atlassian.net/browse/ICU-22787 Backport of: unicode-org/icu#3023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICU-22787 Fix ClangCL compilation on Windows #3023

ICU-22787 Fix ClangCL compilation on Windows #3023

StefanStojanovic commented Jun 3, 2024 •

edited by roubert

Loading

srl295 commented Jun 3, 2024

roubert Jun 6, 2024

StefanStojanovic Jun 7, 2024

roubert Jun 13, 2024

StefanStojanovic Jun 19, 2024

roubert Jun 19, 2024

StefanStojanovic Jun 19, 2024

roubert Jun 6, 2024

StefanStojanovic Jun 7, 2024

srl295 Jun 7, 2024

roubert Jun 6, 2024

StefanStojanovic Jun 7, 2024

roubert Jun 6, 2024

StefanStojanovic Jun 7, 2024

jira-pull-request-webhook bot commented Jun 7, 2024

StefanStojanovic commented Jun 7, 2024

markusicu commented Jun 7, 2024

StefanStojanovic commented Jun 7, 2024

StefanStojanovic commented Jun 11, 2024

roubert commented Jun 12, 2024

roubert commented Jun 13, 2024

roubert Jun 13, 2024

StefanStojanovic Jun 19, 2024

roubert Jun 13, 2024

roubert Jun 19, 2024

jira-pull-request-webhook bot commented Jun 20, 2024

StefanStojanovic commented Jun 20, 2024

roubert left a comment

rp9-next commented Aug 8, 2024

jira-pull-request-webhook bot commented Aug 8, 2024

StefanStojanovic commented Aug 8, 2024

roubert commented Aug 8, 2024 •

edited

Loading

ICU-22787 Fix ClangCL compilation on Windows #3023

ICU-22787 Fix ClangCL compilation on Windows #3023

Conversation

StefanStojanovic commented Jun 3, 2024 • edited by roubert Loading

Checklist

srl295 commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Jun 7, 2024

StefanStojanovic commented Jun 7, 2024

markusicu commented Jun 7, 2024

StefanStojanovic commented Jun 7, 2024

StefanStojanovic commented Jun 11, 2024

roubert commented Jun 12, 2024

roubert commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Jun 20, 2024

StefanStojanovic commented Jun 20, 2024

roubert left a comment

Choose a reason for hiding this comment

rp9-next commented Aug 8, 2024

jira-pull-request-webhook bot commented Aug 8, 2024

StefanStojanovic commented Aug 8, 2024

roubert commented Aug 8, 2024 • edited Loading

StefanStojanovic commented Jun 3, 2024 •

edited by roubert

Loading

roubert commented Aug 8, 2024 •

edited

Loading