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.

Sign up for GitHub

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-22787 Fix ClangCL compilation on Windows #3023

Merged
merged 1 commit into from
Aug 9, 2024
Merged

ICU-22787 Fix ClangCL compilation on Windows #3023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions icu4c/source/common/unicode/platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@
/**
* \def U_PLATFORM_USES_ONLY_WIN32_API
* Defines whether the platform uses only the Win32 API.
* Set to 1 for Windows/MSVC and MinGW but not Cygwin.
* Set to 1 for Windows/MSVC, ClangCL and MinGW but not Cygwin.
* @internal
*/
#ifdef U_PLATFORM_USES_ONLY_WIN32_API
Expand All @@ -250,7 +250,7 @@
/**
* \def U_PLATFORM_HAS_WIN32_API
* Defines whether the Win32 API is available on the platform.
* Set to 1 for Windows/MSVC, MinGW and Cygwin.
* Set to 1 for Windows/MSVC, ClangCL, MinGW and Cygwin.
* @internal
*/
#ifdef U_PLATFORM_HAS_WIN32_API
Expand Down
12 changes: 12 additions & 0 deletions icu4c/source/tools/genccode/genccode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ enum {
#ifdef CAN_GENERATE_OBJECTS
kOptObject,
kOptMatchArch,
kOptCpuArch,
kOptSkipDllExport,
#endif
kOptFilename,
Expand All @@ -86,6 +87,7 @@ static UOption options[]={
#ifdef CAN_GENERATE_OBJECTS
/*6*/UOPTION_DEF("object", 'o', UOPT_NO_ARG),
UOPTION_DEF("match-arch", 'm', UOPT_REQUIRES_ARG),
UOPTION_DEF("cpu-arch", 'c', UOPT_REQUIRES_ARG),
UOPTION_DEF("skip-dll-export", '\0', UOPT_NO_ARG),
#endif
UOPTION_DEF("filename", 'f', UOPT_REQUIRES_ARG),
Expand Down Expand Up @@ -131,6 +133,8 @@ 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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"\t Valid values for this opton are x64, x86 and arm64.\n"
"\t--skip-dll-export Don't export the ICU data entry point symbol (for use when statically linking)\n");
#endif
fprintf(stderr,
Expand Down Expand Up @@ -193,9 +197,17 @@ main(int argc, char* argv[]) {
break;
#ifdef CAN_GENERATE_OBJECTS
case CALL_WRITEOBJECT:
if(options[kOptCpuArch].doesOccur) {
if (!checkCpuArchitecture(options[kOptCpuArch].value)) {
fprintf(stderr,
"CPU architecture \"%s\" is unknown.\n", options[kOptCpuArch].value);
return -1;
}
}
writeObjectCode(filename, options[kOptDestDir].value,
options[kOptEntryPoint].doesOccur ? options[kOptEntryPoint].value : NULL,
options[kOptMatchArch].doesOccur ? options[kOptMatchArch].value : NULL,
options[kOptCpuArch].doesOccur ? options[kOptCpuArch].value : NULL,
options[kOptFilename].doesOccur ? options[kOptFilename].value : NULL,
NULL,
0,
Expand Down
1 change: 1 addition & 0 deletions icu4c/source/tools/pkgdata/pkgdata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,7 @@ static int32_t pkg_executeOptions(UPKGOptions *o) {
o->entryName,
(optMatchArch[0] == 0 ? nullptr : optMatchArch),
nullptr,
nullptr,
gencFilePath,
sizeof(gencFilePath),
true);
Expand Down
38 changes: 35 additions & 3 deletions icu4c/source/tools/toolutil/pkg_genc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
# define NOMCX
#include <windows.h>
#include <time.h>
# if defined(__clang__)
# include <exception>
# endif
# ifdef __GNUC__
# define WINDOWS_WITH_GNUC
# endif
Expand Down Expand Up @@ -294,6 +297,11 @@ checkAssemblyHeaderName(const char* optAssembly) {
return false;
}

U_CAPI UBool U_EXPORT2
checkCpuArchitecture(const char* optCpuArch) {
return strcmp(optCpuArch, "x64") == 0 || strcmp(optCpuArch, "x86") == 0 || strcmp(optCpuArch, "arm64") == 0;
}


U_CAPI void U_EXPORT2
printAssemblyHeadersToStdErr() {
Expand Down Expand Up @@ -799,7 +807,12 @@ getOutFilename(

#ifdef CAN_GENERATE_OBJECTS
static void
getArchitecture(uint16_t *pCPU, uint16_t *pBits, UBool *pIsBigEndian, const char *optMatchArch) {
getArchitecture(
uint16_t *pCPU,
uint16_t *pBits,
UBool *pIsBigEndian,
const char *optMatchArch,
[[maybe_unused]] const char *optCpuArch) {
union {
char bytes[2048];
#ifdef U_ELF
Expand Down Expand Up @@ -847,7 +860,25 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// linker for MSVC. Because of this optCpuArch is used to define the CPU
// architecture in that case. While _M_AMD64 and _M_ARM64 could be used,
// this would potentially be problematic when cross-compiling as this code
// would most likely be ran on host machine to generate the .obj file for
// the target architecture.
# if defined(__clang__)
if (strcmp(optCpuArch, "x64") == 0) {
*pCPU = IMAGE_FILE_MACHINE_AMD64;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else if (strcmp(optCpuArch, "x86") == 0) {
*pCPU = IMAGE_FILE_MACHINE_I386;
} else if (strcmp(optCpuArch, "arm64") == 0) {
*pCPU = IMAGE_FILE_MACHINE_ARM64;
} else {
std::terminate(); // Unreachable.
}
# else
*pCPU = IMAGE_FILE_MACHINE_UNKNOWN;
# endif
# endif
# if defined(_M_IA64) || defined(_M_AMD64) || defined (_M_ARM64)
*pBits = 64; // Doesn't seem to be used for anything interesting though?
Expand Down Expand Up @@ -934,6 +965,7 @@ writeObjectCode(
const char *destdir,
const char *optEntryPoint,
const char *optMatchArch,
const char *optCpuArch,
const char *optFilename,
char *outFilePath,
size_t outFilePathCapacity,
Expand Down Expand Up @@ -1201,7 +1233,7 @@ writeObjectCode(
#endif

/* deal with options, files and the entry point name */
getArchitecture(&cpu, &bits, &makeBigEndian, optMatchArch);
getArchitecture(&cpu, &bits, &makeBigEndian, optMatchArch, optCpuArch);
if (optMatchArch)
{
printf("genccode: --match-arch cpu=%hu bits=%hu big-endian=%d\n", cpu, bits, makeBigEndian);
Expand Down
4 changes: 4 additions & 0 deletions icu4c/source/tools/toolutil/pkg_genc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ printAssemblyHeadersToStdErr(void);
U_CAPI UBool U_EXPORT2
checkAssemblyHeaderName(const char* optAssembly);

U_CAPI UBool U_EXPORT2
checkCpuArchitecture(const char* optCpuArch);

U_CAPI void U_EXPORT2
writeCCode(
const char *filename,
Expand All @@ -98,6 +101,7 @@ writeObjectCode(
const char *destdir,
const char *optEntryPoint,
const char *optMatchArch,
const char *optCpuArch,
const char *optFilename,
char *outFilePath,
size_t outFilePathCapacity,
Expand Down