clang-cl on Windows still needs PreferredToolArchitecture

[chris-eibl]

I hoped I fixed it in 263870d, but it turned out, that this is not enough, see astral-sh/python-build-standalone#549 (comment).

Digging in deeper, I now know more, but do not have an ideal fix, yet.

First, for Hacl_Hash_Blake2b_Simd256.c

<AdditionalOptions>/arch:AVX2</AdditionalOptions>

is missing %(AdditionalOptions). Likewise, Hacl_Hash_Blake2s_Simd128.c. Then, both will "see" the -m32/ -m64 defined in

cpython/PCbuild/pyproject-clangcl.props

Lines 42 to 43 in 0a54bd6

	<AdditionalOptions Condition="'$(Platform)' == 'Win32'">-m32 %(AdditionalOptions)</AdditionalOptions>
	<AdditionalOptions Condition="'$(Platform)' == 'x64'">-m64 %(AdditionalOptions)</AdditionalOptions>

This will fix regular release or debug builds (#131475), since they do not have to link against anything from clang.

But unfortunately, for PGO builds, clang-cl needs to link against clang_rt.profile.lib:

• <VS install path>\Community\VC\Tools\Llvm\lib\clang\<clang major version>\lib\windows\clang_rt.profile-i386.lib
• <VS install path>\Community\VC\Tools\Llvm\x64\lib\clang\<clang major version>\lib\windows\clang_rt.profile-x86_64.lib

This is not found correctly without setting PreferredToolArchitecture (or LLVMInstallDir).
The reason stems from <VS install path>\Community\MSBuild\Microsoft\VC\v160\Microsoft.Cpp.ClangCl.Common.props:

    <_DefaultLLVMInstallDir Condition="'$(_DefaultLLVMInstallDir)' == '' AND '$(PreferredToolArchitecture)' == 'arm64'">$(VsInstallRoot)\VC\Tools\Llvm\ARM64</_DefaultLLVMInstallDir>
    <_DefaultLLVMInstallDir Condition="'$(_DefaultLLVMInstallDir)' == '' AND '$(PreferredToolArchitecture)' == 'x64'">$(VsInstallRoot)\VC\Tools\Llvm\x64</_DefaultLLVMInstallDir>
    <_DefaultLLVMInstallDir Condition="'$(_DefaultLLVMInstallDir)' == '' AND '$(PreferredToolArchitecture)' != 'x64'">$(VsInstallRoot)\VC\Tools\Llvm</_DefaultLLVMInstallDir>
    <LLVMInstallDir Condition="'$(LLVMInstallDir)' == ''">$(_DefaultLLVMInstallDir)</LLVMInstallDir>

This means, if PreferredToolArchitecture is not given on the command line, for a 64bit build the LLVMInstallDir is chosen to be the "32bit clang installation". Even though this one will happily "cross-compile" getting the -m64 switch, it will fail in the link step, because the 64bit libs are "in the 64bit clang installation directory".

See also https://learn.microsoft.com/en-us/cpp/build/reference/msbuild-visual-cpp-overview?view=msvc-170#preferredtoolarchitecture-property:

The PreferredToolArchitecture property determines whether the 32-bit or 64-bit compiler and tools are used in the build.

I am unsure what to do here:

• try to fix that somewhere in pyproject-clangcl.props: not so easy, because "too late": LLVMInstallDir will always be set here, either because
  • given on the command line, i.e. custom clang installation
  • or Microsoft.Cpp.ClangCl.Common.props will already have set it to the bundled clang installation based on PreferredToolArchitecture
• ask Microsoft to fix that? There are some hits about this behaviour in the net ...
• document (again, but this time I have more background knowledge) that the user is responsible to either
  • set PreferredToolArchitecture correctly when using the bundled version
  • set LLVMInstallDir to a 32bit installation for 32bit builds and similarily for 64bit builds.

See here, why I personally anyways always set PreferredToolArchitecture (spoiler: I do not like the _freeze_module to be compiled as 32bit, for exactly the same reason: if PreferredToolArchitecture is missing, it defaults to 32bit). Most probably an unwanted side effect of https://github.com/python/cpython/pull/28491/files or the lesser evil :)

ISTM, I returned to this habit too quickly, and so I missed that rabit hole - but now I've dug deeper.

FTR, this will also be needed when someone wants to do ASAN, UBSAN, FUZZER, etc, builds using clang-cl on Windows, because in all those cases the correct libs are needed.

Linked PRs

• GH-131473: add missing %(AdditionalOptions) for Hacl_Hash_Blake2b_Simd{128|256}.c #131475
• GH-131473: tweak PreferredToolArchitecture for bundled clang-cl #131689

[zooba]

See here, why I personally anyways always set PreferredToolArchitecture (spoiler: I do not like the _freeze_module to be compiled as 32bit, for exactly the same reason: if PreferredToolArchitecture is missing, it defaults to 32bit). Most probably an unwanted side effect of https://github.com/python/cpython/pull/28491/files or the lesser evil :)

Intentional design, not unintended ;) See the discussion on that PR. The PreferredToolArchitecture default value isn't handled by us, so it can be changed whenever Microsoft/MSVC decide that their AMD64 tools are better than the x86 ones (which I believe they are at this point, but they haven't updated the variable yet).

[zooba]

Also, is #131475 the only change needed here? Or are there more coming?

[chris-eibl]

Let's keep it open a little bit. Maybe somebody has an idea about a fix. Otherwise just go with

document (again, but this time I have more background knowledge) that the user is responsible to either

• set PreferredToolArchitecture correctly when using the bundled version
• set LLVMInstallDir to a 32bit installation for 32bit builds and similarily for 64bit builds.

and I'll try to write it up in PCBuild/readme.bat?

[chris-eibl]

Intentional design, not unintended ;) See the discussion on that PR. The PreferredToolArchitecture default value isn't handled by us, so it can be changed whenever Microsoft/MSVC decide that their AMD64 tools are better than the x86 ones (which I believe they are at this point, but they haven't updated the variable yet).

Yeah, I read that before - many times. The issue was about fixing _freeze_module to be built for ARM instead x64 - and it was quite tough to do that. PreferredToolArchitecture seemed to solve the ARM issue, but since then, for an AMD64 build, the _freeze_module is build in 32bit. Not a big issue, though :)

[zooba]

since then, for an AMD64 build, the _freeze_module is build in 32bit.

This is because you can do an AMD64 build on a 32-bit machine or an ARM64 machine, and so the most portable set of tools (binaries that will run during build) is 32-bit. The Windows build system is inherently cross-compiling, so choosing 64-bit tools has to be based on the current platform, not the target platform.

• set PreferredToolArchitecture correctly when using the bundled version

Just to confirm my understanding - this is because Clang doesn't do cross-compiling properly? And so we don't have the full set of tools that MSVC includes. Or perhaps all the Clang builds run on x64 and we just need to choose the right one for the target platform, but the MSBuild files are using the wrong variable to select?

[chris-eibl]

There are two clang folders bundled with MSVC: the 32bit tool chain and the 64bit tool chain. Both of them can cross compile using the -m32 and -m64 parameters. Their respective default is their own bitness.

The problem is, that in the 32bit folder only 32bit libraries are present and vice versa. Hence, we need to set PreferredToolArchitecture accordingly to get the "correct bundled one".

When I downloaded clang+llvm-20.1.0-rc2-x86_64-pc-windows-msvc from https://github.com/llvm/llvm-project/releases, there were no 32bit libs in there, so situation seems similar.

I am by far no clang expert - just observing.

but the MSBuild files are using the wrong variable to select?

For the bundled ones - I'd say "yes". Msbuild could chose based on the target version instead on the PreferredToolArchitecture and all would be fine.

For self-installed ones ISTM, one would have to chose the correct bitness toolchain - again, just due to the missing libs.

Sure I think one could just copy them in.

At least this is how I'd sum up.

Maybe someone with more experience in the LLVM tool chain can clarify?

[zooba]

The problem is, that in the 32bit folder only 32bit libraries are present and vice versa.

Yeah, so it's not really properly set up for cross-compiling. The libraries directory should follow the target platform, but it's following the tool platform.¹ Regardless of who understands this, it's up to the MSVC team to fix their bundled Clang to be set up properly, which means we can't really rely on it.

We discussed using the clang-cl.props file overriding PreferredToolArchitecture, didn't we? That might be the only reasonable option, and we'll just have to say that cross-compiling with clang-cl isn't supported (which would also rule it out from becoming the default compiler, but that's not planned anyway).

Footnotes

1. I dislike the "real" definitions for host, build and target platforms so I'm not using them. "Tool platform" is PreferredToolArchitecture and "target platform" is Platform. ↩

[chris-eibl]

Yeah, you were suggesting that in #129907 (comment)

and I tried to add

<PreferredToolArchitecture Condition="$(PROCESSOR_ARCHITECTURE) == 'AMD64'">x64</PreferredToolArchitecture>

to https://github.com/python/cpython/blob/0a54bd6dd7cda3b9611bf33652184c477a332c7e/PCbuild/pyproject-clangcl.props in #129907 (comment), but that was "too late": Microsoft.Cpp.ClangCl.Common.props resolves LLVMInstalDir "before" this, and so it has no effect.

I can try again, though?

Or is there a way we could kick in earlier?

[zooba]

Probably just needs to go in python.props with a PlatformToolset check and a comment explaining why it's there (along with other checks to make sure the variable hasn't been overridden already).