Skip to content

GH-131521: fix clangcl build on Windows for zlib-ng #131526

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

Merged
merged 7 commits into from
Mar 24, 2025
Merged

GH-131521: fix clangcl build on Windows for zlib-ng #131526

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1852ff4
fix clangcl build on Windows for zlib-ng
chris-eibl Mar 20, 2025
335009b
use -msse4.2 for adler32_sse42.c
chris-eibl Mar 22, 2025
4bdf82f
Do not use AdvancedVectorExtensions2 for all *.c files,
chris-eibl Mar 22, 2025
3ad6eb2
Merge branch 'main' into fix_clangcl_zlibng, because
chris-eibl Mar 22, 2025
b651937
// DO NOT MERGE - just triggering Windows tail-call CI)
chris-eibl Mar 22, 2025
5d423d5
fix clang-cl PGO build
chris-eibl Mar 22, 2025
123e047
remove trigger comment from ceval_macros.h
chris-eibl Mar 24, 2025
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
1 change: 1 addition & 0 deletions PCbuild/zlib-ng.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
<ItemDefinitionGroup>
<ClCompile>
<AdditionalOptions>%(AdditionalOptions) /utf-8 /w34242</AdditionalOptions>
<AdditionalOptions Condition="$(SupportPGO) and $(Configuration) == 'PGUpdate' and $(PlatformToolset) == 'ClangCL'">%(AdditionalOptions) -fno-profile-instr-use</AdditionalOptions>
Copy link
Member Author

Choose a reason for hiding this comment

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

Since zlib-ng is compiled into a static lib, it won't produce any profile data. It is the first target that is built (after the _freeze_module)

cpython/PCbuild/pcbuild.proj

Lines 49 to 51 in 9962469

<ItemGroup>
<!-- Static libraries for use later in the build -->
<Projects Include="zlib-ng.vcxproj" Condition="$(zlibNgDir) != '' and Exists('$(zlibNgDir)\zlib-ng.h.in')" />

But since there is no \$(TargetName)_*.profraw for it

cpython/PCbuild/pyproject-clangcl.props

Lines 16 to 18 in 9962469

<ItemGroup>
<_profrawFiles Include="$(OutDir)instrumented\$(TargetName)_*.profraw" />
</ItemGroup>

MergeClangProfileData won't create profdata.profdata:

cpython/PCbuild/pyproject-clangcl.props

Lines 26 to 28 in 9962469

<Target Name="MergeClangProfileData" BeforeTargets="PrepareForBuild"
Condition="'$(SupportPGO)' and $(Configuration) == 'PGUpdate'"
Inputs="@(_profrawFiles)"

IMHO, this is the best fix (instead of just setting SupportPGO=false for it):

  • it still will get compiled with -fprofile-instr-generate
  • it will be linked with python314.dll and (hopefully?) happily contribute its part to python314.profraw
  • MSVC doesn't create zlib-ng!1.pgc and zlib-ng.pgd either. No idea whether it gets PGOed there, but the same idea applies? The difference is just, that MSVC does not produce a hard error if the profile data isn't there, whereas clang does (TRACKEDEXEC : error : Error in reading profile profdata.profdata: no such file or directory), and I still have found no way to make that a warning :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Since zlib-ng is compiled into a static lib, it won't produce any profile data.

Thinking about it again, instead of doing the -fno-profile-instr-use trick in the PGUpdate phase, it would be better, to just create the lib once instead of twice (like I've recently done for the _freeze_module).
There is no need to build the lib twice (same holds true for the MSVC case)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I gave it a quick try:

  • introducing a StaticLibProjects like FreezeProjects in pcbuild.proj is simple and straight forward.
  • Then I'd have to make the ProjectReference Include="zlib-ng.vcxproj conditional (otherwise it would still be built in the PGUpdate phase) - ok, doable.
  • but then zlib.h is no longer found, because it is generated into $(IntDir)zlib.h - likewise $(IntDir)zlib-ng.h. So stopping here :)

So turns out to be not so easy and I'd like to keep that for a follow-up PR (if at all).

Copy link
Member

Choose a reason for hiding this comment

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

I assumed the call counts for PGO are going straight into each project that references it, since a static lib is essentially just a bundle of .obj files and so the link-time optimisation happens in the referencing project, not the static lib project.

Which means yes, it's participating in PGO, and it doesn't require rebuilding. Though it's not going to hurt that badly, so I'd tend towards decoupled build configuration (it would be easy to fix by moving the header file regeneration into pythoncore.vcxproj, but that's bad coupling).

Possibly we could make SupportsPGO choose the definition of IntDir and then it would usually be a quick rebuild?

Copy link
Member Author

@chris-eibl chris-eibl Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, but let's do that in a follow-up PR, since it is not really related to clang-cl.

Technically, the /arch changes so that the resulting binary can run on legacy CPUs aren't either, but IMHO they are a nice fit here.

It would just put some rebase burden onto this PR, if we'd do it in a separate one.

<DisableSpecificWarnings>4206;4054;4324</DisableSpecificWarnings>
<LanguageStandard_C>stdc11</LanguageStandard_C>
<PrecompiledHeader>NotUsing</PrecompiledHeader>
Expand Down
Loading