gh-130039: Tailcall for windows builds #130040

Fidget-Spinner · 2025-02-12T13:58:13Z

Issue: Tail calling interpreter for Windows #130039

Fidget-Spinner · 2025-02-12T14:12:28Z

@mdboom I'm baffled by the error in the CI. By any chance do you know how to fix it? https://github.com/python/cpython/actions/runs/13287244377/job/37098802007?pr=130040

mdboom · 2025-02-12T14:52:44Z

@mdboom I'm baffled by the error in the CI. By any chance do you know how to fix it? https://github.com/python/cpython/actions/runs/13287244377/job/37098802007?pr=130040

I can try poking at this locally a bit later today, but my first guess is that it's because the equals sign is getting swallowed here:

/p:PlatformToolset clangcl

...so clangcl is just seen as another project name.

I don't really understand the quoting rules for PowerShell -- maybe "/p:PlatformToolset\=clangcl" is needed?

I think I ran into this same problem in bench-runner, and I ultimately gave up and used cmd rather than PowerShell, which you can do by putting shell: cmd above the run: ... in the yml file. See here for how I solved a similar problem in bench_runner.

Fidget-Spinner · 2025-02-12T15:06:14Z

That makes sense. Thanks. Applied the change to use cmd instead!

Fidget-Spinner · 2025-02-12T15:17:37Z

@chris-eibl I guess we're blocked on getting patches to make clang-cl build CPython. Currently there's a few build errors which I assume you patched out on your branch? Do you mind upstreaming those patches? I can review them.

Fidget-Spinner · 2025-02-12T16:14:23Z

Nevermind, I will try to patch them into this PR as well, so that we can do an integration test.

chris-eibl · 2025-02-12T16:25:45Z

I don't really understand the quoting rules for PowerShell -- maybe "/p:PlatformToolset\=clangcl" is needed?

Yeah, see this part of build.bat -h:

If the argument contains an '=', the
entire argument must be quoted (e.g. `build.bat "/p:PlatformToolset=v141"`).

Had a quick look at the PR and everything looks fine to me.

The build log of your latest commit 7f91920 shows

  Performing Custom Build Tools
   Assembling: D:\a\cpython\cpython\externals\mpdecimal-4.0.0\libmpdec\vcdiv64.asm
  error: symbol 'AsyncioDebug' is already defined

I have never seen that error before.

For sure not during assembling, must come out of _asynciomodule.c:

GENERATE_DEBUG_SECTION(AsyncioDebug, Py_AsyncioModuleDebugOffsets AsyncioDebug)

That's the only occurence I've found.

Ah - you've found it yourself while I was typing :)

Interestingly: I cannot reproduce locally for the failing commit 7f91920 using:

build.bat --tail-call-interp -p x64 -c Release "/p:PlatformToolset=clangcl" "/p:PreferredToolArchitecture=x64"

with Visual Studio 2022 17.13.0 Preview 5.0, which ships with cl.exe 19.43.34808 and clang 19.1.1.

Let's see whether your latest commit fixes CI - looks promising :)

mdboom · 2025-02-12T16:31:24Z

Yeah, see this part of build.bat -h:

If the argument contains an '=', the
entire argument must be quoted (e.g. `build.bat "/p:PlatformToolset=v141"`).

I'm pretty sure this advice assumes using cmd, not PowerShell (which is the default on Github Actions). Overriding the shell used on Github Actions seems to be the easy solution. The harder solution is figuring out PowerShell's escaping rules...

Fidget-Spinner · 2025-02-12T16:32:52Z

with Visual Studio 2022 17.13.0 Preview 5.0, which ships with cl.exe 19.43.34808 and clang 19.1.1.

Let's see whether your latest commit fixes CI - looks promising :)

That didn't fix it :(.

chris-eibl · 2025-02-12T16:34:25Z

Ups - still the same error - very weird. No idea atm - sorry ...

chris-eibl · 2025-02-12T16:37:07Z

Unfortunately I cannot reproduce locally. Have a different clang version, though. Did also use clang 19.1.6 a lot.

Will fire up a build for this commit ...

Update:
Commit 7f91920 builds for me with clang 19.1.6, too:

build.bat --tail-call-interp -p x64 -c Release "/p:PlatformToolset=clangcl" "/p:LLVMToolsVersion=19.1.6" "/p:LLVMInstallDir=clang+llvm-19.1.6-x86_64-pc-windows-msvc"

Fidget-Spinner · 2025-02-12T16:39:50Z

Ah.. I think I forgot to add parentheses :) my bad

Fidget-Spinner · 2025-02-12T16:47:15Z

Nevermind, that didn't fix it. Now that I look at the macros closer, it shouldn't fix it either.

chris-eibl · 2025-02-12T17:02:38Z

Really strange why that happens on CI. Cannot reproduce with clang 19.1.1 and 19.1.6.
The CI seems to use 19.1.5 - but hard to believe this is the culprit.

Those macros have been touched recently #129223.
Yet, I never had an issue with clang-cl so far (and still have not).

Fidget-Spinner · 2025-02-12T17:07:21Z

Fixed it on CI!

Fidget-Spinner · 2025-03-05T08:41:36Z

@zooba gentle ping for a review please. Thanks again for reviewing that other PR on getting PGO working on clang-cl. This should be relatively tame in comparison :).

.github/workflows/tail-call.yml

Co-authored-by: Hugo van Kemenade <[email protected]>

.github/workflows/tail-call.yml

zooba · 2025-03-05T16:49:20Z

Include/internal/pycore_debug_offsets.h

@@ -23,7 +23,7 @@ extern "C" {
   declaration                                      \
   _GENERATE_DEBUG_SECTION_LINUX(name)

-#if defined(MS_WINDOWS)
+#if defined(MS_WINDOWS) && !defined(__clang__)


Does this need an alternate definition for Windows + clang?

No, below there's a default that it uses is we're not one of the supported platforms. And that default works for Clang Windows.

PCbuild/build.bat

zooba · 2025-03-05T16:51:09Z

.github/workflows/tail-call.yml

@@ -2,12 +2,14 @@ name: Tail calling interpreter
 on:
  pull_request:
    paths:
+      - '.github/workflows/tail-call.yml'      


This feels wrong to me, but if it's needed, then sure. Hopefully someone else can sign off on this part of the change.

This is fine: when changing this file, it's good to run it as well to make sure we didn't break it -- which is rather too easy with YAML.

(And let's strip the trailing spaces.)

Suggested change

- '.github/workflows/tail-call.yml'

- '.github/workflows/tail-call.yml'

(We should add yaml to this list, feel free to include it in the PR if you like:)

cpython/.pre-commit-config.yaml

Line 49 in 2904ec2

types_or: [c, inc, python, rst]

We can't specify the minor version as the apt script doesn't recognize it.

python#130040 (comment)

zooba · 2025-03-10T16:52:39Z

Looks fine to me.

I hope we can justify (one day) taking options out of build.bat without deprecation periods and warnings. I think we probably can. In any case, this PR isn't the cause of sprawling options, so no harm in merging it.

chris-eibl · 2025-03-10T17:52:44Z

@zooba: I understand. But doing that options in build.bat is the only way to get it configurable without modifying/hardcoding it in pythoncore.vcxproj. Which I manually had to do when trying computed gotos builds in #130090, and thus always had a "dirty repo".

Would it be okay for you if I create a similar PR for computed gotos?

Or is there a better way?

zooba · 2025-03-11T13:52:13Z

Environment variables pass through into the build just fine, and are exposed as MSBuild properties (provided we don't immediately overwrite them). I have no problem (well, fewer problems) with enabling a feature by environment variable, it's just the batch file option that worries me.

The main difference is that if we remove the setting and it was only used by environment variable, the build will still succeed. If we remove a build.bat option, builds will break. So arguably a good dividing line is whether builds ought to break if/when we remove the option - most optimisations and experimental features aren't in that category, IMHO.

chris-eibl · 2025-03-11T15:05:21Z

That would be fine with me, too. IIUC, e.g. just add

<PreprocessorDefinitions Condition="'$(HAVE_COMPUTED_GOTOS)' == 'true'">HAVE_COMPUTED_GOTOS;%(PreprocessorDefinitions)</PreprocessorDefinitions>

and leave it to the user to

either add "/p:HAVE_COMPUTED_GOTOS=true" when invoking build.bat
or just set the environment variable HAVE_COMPUTED_GOTOS=true before invoking build.bat

up to their liking.

Either way, build.bat does not have to "learn" all of that things. Might have been better for some of the existing switches, too - but it's too late for that. Maybe not for tailcall, since this is new?

And if it is really important to warn or fail if a variable does not make sense any longer, we could do something like

<Message Text="HAVE_COMPUTED_GOTOS are no longer supported" Condition="$(HAVE_COMPUTED_GOTOS) == "true" Importance="high" />

or

<Error Text="HAVE_COMPUTED_GOTOS are no longer supported" Condition="$(HAVE_COMPUTED_GOTOS) == "true" Code="1" />

Regarding documentation: shall this still be available via build.bat -h (still less clutter) or only via PCbuild/readme.txt?

PS: we even could do some meaningful defaults like enabling tail calling if not given and LLVMToolsVersion indicates clang-cl >= 19.x.x.

Likewise, we could enable cg if HAVE_COMPUTED_GOTOS is not given (or true) but disable it if HAVE_COMPUTED_GOTOS=false, etc ...

chris-eibl · 2025-03-11T15:26:57Z

Ah, and the "switch" shall maybe be named WITH_COMPUTED_GOTOS in analogy to --with-computed-gotos in the configure script and the other --with things, where build.bat tries to mimic configure.

Naming is always hard :(

zooba · 2025-03-11T17:39:07Z

IIUC, ...

Yeah, though I'd test for != '' rather than a specific value. And it only really needs mentioning in PCbuild\README.txt, probably with a note that they are subject to change at any time.

I'm okay with meaningful defaults, but do be very careful about the checks. It's very easy to break unrelated builds, because basically everything gets evaluated all the time and there's not much in the way of error handling.

Co-authored-by: Hugo van Kemenade <[email protected]>

Fidget-Spinner added 3 commits February 12, 2025 20:54

Tail-calling flag for Window

ab0a611

Update pythoncore.vcxproj

d57c175

Add Windows to the tailcall CI

3eb568f

Fidget-Spinner requested review from a team, ezio-melotti, hugovk and AA-Turner as code owners February 12, 2025 13:58

bedevere-app bot added the awaiting core review label Feb 12, 2025

bedevere-app bot mentioned this pull request Feb 12, 2025

Tail calling interpreter for Windows #130039

Closed

Fidget-Spinner requested a review from mdboom February 12, 2025 13:58

Update tail-call.yml

4dd19cf

Fidget-Spinner added the skip news label Feb 12, 2025

Update tail-call.yml

7f91920

Update pycore_debug_offsets.h

c3d656b

Update pycore_debug_offsets.h

3638cda

Update pycore_debug_offsets.h

b8b79ed

Address review by using env vars

c811b8a

Fidget-Spinner requested a review from zooba February 13, 2025 15:37

Fidget-Spinner added 3 commits February 13, 2025 23:39

Merge remote-tracking branch 'upstream/main' into tailcall_for_windows

904b7ee

Merge remote-tracking branch 'upstream/main' into tailcall_for_windows

991b50f

Update pythoncore.vcxproj

166187c

chris-eibl mentioned this pull request Feb 24, 2025

GH-130213: clang-cl: blake2module.c needs to "see" the AVX intrinsics during compiling #130447

Closed

hugovk reviewed Mar 5, 2025

View reviewed changes

.github/workflows/tail-call.yml Outdated Show resolved Hide resolved

Fidget-Spinner and others added 2 commits March 5, 2025 17:14

Update .github/workflows/tail-call.yml

5f7ad24

Co-authored-by: Hugo van Kemenade <[email protected]>

Update tail-call.yml

6d7b689

zooba reviewed Mar 5, 2025

View reviewed changes

Fidget-Spinner added 2 commits March 6, 2025 01:21

Address review

aadfec4

Partially revert "Address review"

1d7c051

We can't specify the minor version as the apt script doesn't recognize it.

Fidget-Spinner mentioned this pull request Mar 5, 2025

Update Windows builds to use ClangCL astral-sh/python-build-standalone#549

Draft

Fidget-Spinner requested a review from zooba March 7, 2025 17:00

chris-eibl added a commit to chris-eibl/cpython that referenced this pull request Mar 9, 2025

Interestingly, for debug builds I now need this, too.

0bd337d

python#130040 (comment)

chris-eibl mentioned this pull request Mar 9, 2025

GH-130090: build time analysis #131005

Closed

Fidget-Spinner merged commit a8ee1e1 into python:main Mar 11, 2025
47 checks passed

bedevere-app bot removed the awaiting core review label Mar 11, 2025

Fidget-Spinner deleted the tailcall_for_windows branch March 11, 2025 02:53

This was referenced Mar 15, 2025

Support building using "computed gotos" for clang-cl on Windows #131278

Closed

GH-131278: Support building using "computed gotos" for clang-cl on Windows #131279

Merged

chris-eibl mentioned this pull request Apr 4, 2025

gh-132132: Upgrade LLVM on tail calling CI #132098

Merged

seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025

pythongh-130039: Tailcall for windows builds (pythonGH-130040)

211db49

Co-authored-by: Hugo van Kemenade <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-130039: Tailcall for windows builds #130040

gh-130039: Tailcall for windows builds #130040

Fidget-Spinner commented Feb 12, 2025 •

edited by bedevere-app bot

Loading

Fidget-Spinner commented Feb 12, 2025

mdboom commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

chris-eibl commented Feb 12, 2025 •

edited

Loading

mdboom commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

chris-eibl commented Feb 12, 2025

chris-eibl commented Feb 12, 2025 •

edited

Loading

Fidget-Spinner commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

chris-eibl commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

Fidget-Spinner commented Mar 5, 2025

zooba Mar 5, 2025

Fidget-Spinner Mar 5, 2025

zooba Mar 5, 2025

hugovk Mar 5, 2025

zooba commented Mar 10, 2025

chris-eibl commented Mar 10, 2025

zooba commented Mar 11, 2025

chris-eibl commented Mar 11, 2025

chris-eibl commented Mar 11, 2025

zooba commented Mar 11, 2025

	- '.github/workflows/tail-call.yml'
	- '.github/workflows/tail-call.yml'

gh-130039: Tailcall for windows builds #130040

gh-130039: Tailcall for windows builds #130040

Conversation

Fidget-Spinner commented Feb 12, 2025 • edited by bedevere-app bot Loading

Fidget-Spinner commented Feb 12, 2025

mdboom commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

chris-eibl commented Feb 12, 2025 • edited Loading

mdboom commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

chris-eibl commented Feb 12, 2025

chris-eibl commented Feb 12, 2025 • edited Loading

Fidget-Spinner commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

chris-eibl commented Feb 12, 2025

Fidget-Spinner commented Feb 12, 2025

Fidget-Spinner commented Mar 5, 2025

zooba Mar 5, 2025

Choose a reason for hiding this comment

Fidget-Spinner Mar 5, 2025

Choose a reason for hiding this comment

zooba Mar 5, 2025

Choose a reason for hiding this comment

hugovk Mar 5, 2025

Choose a reason for hiding this comment

zooba commented Mar 10, 2025

chris-eibl commented Mar 10, 2025

zooba commented Mar 11, 2025

chris-eibl commented Mar 11, 2025

chris-eibl commented Mar 11, 2025

zooba commented Mar 11, 2025

Fidget-Spinner commented Feb 12, 2025 •

edited by bedevere-app bot

Loading

chris-eibl commented Feb 12, 2025 •

edited

Loading

chris-eibl commented Feb 12, 2025 •

edited

Loading