Skip to content

Conversation

@stefanor
Copy link
Contributor

@stefanor stefanor commented Oct 13, 2025

Adapted from a patch for Python 3.14 submitted to the Debian BTS by John David Anglin https://bugs.debian.org/1105111#20

The backport to the 3.14 branch is non-trivial as some of the affected code was refactored in 7094f09 but the patch in the Debian BTS was targetted at 3.14 and can be referenced to backport.

@picnixz
Copy link
Member

picnixz commented Oct 13, 2025

No news, as this is a simple regression fix

Please do so. A regression fix needs to be announced so that users know that their bug has been indeed fixed.

@picnixz picnixz changed the title GH-139914: On HPPA, the stack grows up GH-139914: Handle stack growth direction on HPPA Oct 13, 2025
picnixz
picnixz previously approved these changes Oct 13, 2025
Python/ceval.c Outdated
uintptr_t here_addr = _Py_get_machine_stack_pointer();
_PyThreadStateImpl *_tstate = (_PyThreadStateImpl *)tstate;
#ifdef __hppa__
if (here_addr <= _tstate->c_stack_soft_limit - margin_count * _PyOS_STACK_MARGIN_BYTES) {
Copy link
Member

Choose a reason for hiding this comment

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

Can't we pre-compute the bound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The immediate answer is: No because margin_count is a function parameter.

However... This is always set to 1, so it could be removed...

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant to avoid computing twice _tstate->c_stack_soft_limit - margin_count * _PyOS_STACK_MARGIN_BYTES to avoid having a double ifdef in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_Py_InitializeRecursionLimits() can change the limits if c_stack_hard_limit == 0 (I assume that's the uninitialized state)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can pre-compute margin_count * _PyOS_STACK_MARGIN_BYTES (used 4 times in the source) potentially saving a multiply.

Copy link
Member

Choose a reason for hiding this comment

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

I assume that's the uninitialized state

That's also something I actually find weird. If we're in an uninitialized state, how come can we use the soft limit? anyway, let's leave the code as is as it could actually be less readable in the end and introduce a subtle issue if _Py_InitializeRecursionLimits could change the soft limit value at that point.

Copy link
Contributor Author

@stefanor stefanor Oct 13, 2025

Choose a reason for hiding this comment

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

Come to think of it, we only hit that second if block if we pass the first one. Given that we're reversing the direction of the comparison, on HPPA we won't get here if c_stack_soft_limit is 0.

@markshannon: Why do we call _Py_InitializeRecursionLimits here?

Copy link
Member

Choose a reason for hiding this comment

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

Because this in an API function, and the stack limits might not have been initialized.
Maybe we are being overly conservative, but it is harmless to check.

Copy link
Contributor Author

@stefanor stefanor Oct 14, 2025

Choose a reason for hiding this comment

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

Don't we risk never reaching never reaching the initialisation check on most platforms, if c_stack_soft_limit is 0? (And possibly on HPPA too, due to underflow of an unsigned int)

It seems like this whole first if block should just be removed.

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

The general implementation looks sound.

Rather than mysterious #ifdef __hppa__ could you #define STACK_GROWS_DOWN in the config and use #if STACK_GROWS_DOWN for clarity and to keep things tidy in case any other platform has a stack the grows up.

OOI what is hppa? Is this PA-RISC, or some new variant?

Python/ceval.c Outdated
uintptr_t here_addr = _Py_get_machine_stack_pointer();
_PyThreadStateImpl *_tstate = (_PyThreadStateImpl *)tstate;
#ifdef __hppa__
if (here_addr <= _tstate->c_stack_soft_limit - margin_count * _PyOS_STACK_MARGIN_BYTES) {
Copy link
Member

Choose a reason for hiding this comment

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

Because this in an API function, and the stack limits might not have been initialized.
Maybe we are being overly conservative, but it is harmless to check.

@bedevere-app
Copy link

bedevere-app bot commented Oct 14, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@stefanor
Copy link
Contributor Author

OOI what is hppa? Is this PA-RISC, or some new variant?

Yes, it is. Still going as Debian port. The newest hardware is 20 years old at this point, but the port is still alive. It'll probably keep going as long as GCC and Linux support it.

@stefanor stefanor requested a review from markshannon October 14, 2025 16:07
@encukou
Copy link
Member

encukou commented Oct 17, 2025

Thank you for upstreaming the patch!

Do the tests pass on HPPA? I would think that TestRecursion in Lib/test/test_call.py needs an update too.

I've sent a PR to your branch to expose _Py_STACK_GROWS_DOWN to Python tests (as _testcapi._Py_STACK_GROWS_DOWN), and to the rest of CPython (through Autuconf).


This PR will conflict with #139668, which will take some time to approve as it adds new API. If this PR is not urgent, I think it's best to wait for #139668, then rebase this one and ask you to check the result. Does that sound OK?

@python-cla-bot
Copy link

python-cla-bot bot commented Oct 19, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

stefanor added a commit to stefanor/cpython that referenced this pull request Oct 22, 2025
While looking at python#140028 I found some test failures that are caused by
new tests (from python#138122) running too slowly.

This adds an arbitrary heuristic to double the sampling run time.
We could do 10x instead? And/or move the heuristic into test_support.
Thoughts?
stefanor added a commit to stefanor/cpython that referenced this pull request Oct 22, 2025
While looking at python#140028 I found some test failures that are caused by
new tests (from python#138122) running too slowly.

This adds an arbitrary heuristic to double the sampling run time.
We could do 10x instead? And/or move the heuristic into test_support.
Thoughts?
stefanor added a commit to stefanor/cpython that referenced this pull request Oct 22, 2025
While looking at python#140028 I found some test failures that are caused by
new tests (from python#138122) running too slowly.

This adds an arbitrary heuristic to 10x the sampling run time (to the
default value of 10 seconds). Doubling the 1-second duration was
sufficient for my HP PA tests, but Fedora reported one of the 2-second
durations being too slow for a freethreaded build.

This heuristic could move into test_support. Thoughts?
vstinner pushed a commit that referenced this pull request Oct 22, 2025
While looking at #140028, I found some unrelated test regressions in the
3.14 cycle. These seem to all come from #130317. From what I can tell,
that made Python more correct than it was before. According to [0], HP PA
RISC uses 1 for SNaN and thus a 0 for QNaN.

[0]: https://grouper.ieee.org/groups/1788/email/msg03272.html
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 22, 2025
While looking at pythonGH-140028, I found some unrelated test regressions in the
3.14 cycle. These seem to all come from pythonGH-130317. From what I can tell,
that made Python more correct than it was before. According to [0], HP PA
RISC uses 1 for SNaN and thus a 0 for QNaN.

[0]: https://grouper.ieee.org/groups/1788/email/msg03272.html
(cherry picked from commit 76fea55)

Co-authored-by: Stefano Rivera <stefano@rivera.za.net>
vstinner pushed a commit that referenced this pull request Oct 22, 2025
…40467)

gh-130317: Fix SNaN broken tests on HP PA RISC (GH-140452)

While looking at GH-140028, I found some unrelated test regressions in the
3.14 cycle. These seem to all come from GH-130317. From what I can tell,
that made Python more correct than it was before. According to [0], HP PA
RISC uses 1 for SNaN and thus a 0 for QNaN.

[0]: https://grouper.ieee.org/groups/1788/email/msg03272.html
(cherry picked from commit 76fea55)

Co-authored-by: Stefano Rivera <stefano@rivera.za.net>
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I just have a minor comment on a test.

@markshannon: Your concern has been addressed, _Py_STACK_GROWS_DOWN macro was added. Would you mind to review the updated PR?

@gpshead
Copy link
Member

gpshead commented Nov 11, 2025

might also be good to go ahead and get a 3.14 backport ready at the same time? i would click merge - but i'm not going to take on the backport.

@bedevere-app
Copy link

bedevere-app bot commented Nov 11, 2025

GH-141404 is a backport of this pull request to the 3.14 branch.

@stefanor
Copy link
Contributor Author

Yes, I have a backport already prepared. #141404

stefanor added a commit to stefanor/cpython that referenced this pull request Nov 11, 2025
…GH-140028)

Adapted from a patch for Python 3.14 submitted to the Debian BTS by John David Anglin https://bugs.debian.org/1105111#20
stefanor added a commit to stefanor/cpython that referenced this pull request Nov 11, 2025
…GH-140028)

Adapted from a patch for Python 3.14 submitted to the Debian BTS by John David Anglin https://bugs.debian.org/1105111#20
@stefanor
Copy link
Contributor Author

Merged main and reran tests, since #139668 landed.

@encukou encukou merged commit f6dd9c1 into python:main Nov 17, 2025
52 checks passed
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian root 3.x (tier-1) has failed when building commit f6dd9c1.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/345/builds/12701) and take a look at the build logs.
  4. Check if the failure is related to this commit (f6dd9c1) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/345/builds/12701

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/support/__init__.py", line 847, in gc_collect
    gc.collect()
    ~~~~~~~~~~^^
ResourceWarning: unclosed file <_io.FileIO name=11 mode='wb' closefd=True>

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Windows Server 2022 NoGIL 3.x (tier-1) has failed when building commit f6dd9c1.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1241/builds/7195) and take a look at the build logs.
  4. Check if the failure is related to this commit (f6dd9c1) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1241/builds/7195

Failed tests:

  • test_profiling

Failed subtests:

  • test_sample_target_module - test.test_profiling.test_sampling_profiler.TestSampleProfilerIntegration.test_sample_target_module

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "C:\Users\Administrator\buildarea\3.x.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\test\test_profiling\test_sampling_profiler.py", line 2059, in test_sample_target_module
    self.assertIn("slow_fibonacci", output)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'slow_fibonacci' not found in 'Captured 1763 samples in 1.00 seconds\nSample rate: 1762.85 samples/sec\nError rate: 2.95%\nWarning: missed 8237 samples from the expected total of 10000 (82.37%)\n\x1b[1;34mProfile Stats:\x1b[0m\n\x1b[1;34m       nsamples\x1b[0m  \x1b[1;34m sample%\x1b[0m  \x1b[1;34mtottime (ms)\x1b[0m  \x1b[1;34m  cumul%\x1b[0m  \x1b[1;34mcumtime (ms)\x1b[0m  \x1b[1;34mfilename:lineno(function)\x1b[0m\n         0/3088       0.0         0.000     180.5       308.800  \x1b[32mrunpy.py\x1b[0m:\x1b[33m88\x1b[0m(\x1b[36m_run_code\x1b[0m)\n         0/1711       0.0         0.000     100.0       171.100  \x1b[32m_sync_coordinator.py\x1b[0m:\x1b[33m144\x1b[0m(\x1b[36m_execute_module\x1b[0m)\n         0/1711       0.0         0.000     100.0       171.100  \x1b[32m_sync_coordinator.py\x1b[0m:\x1b[33m229\x1b[0m(\x1b[36mmain\x1b[0m)\n         0/1711       0.0         0.000     100.0       171.100  \x1b[32m_sync_coordinator.py\x1b[0m:\x1b[33m251\x1b[0m(\x1b[36m<module>\x1b[0m)\n         0/1711       0.0         0.000     100.0       171.100  \x1b[32mrunpy.py\x1b[0m:\x1b[33m198\x1b[0m(\x1b[36m_run_module_as_main\x1b[0m)\n         0/1377       0.0         0.000      80.5       137.700  \x1b[32mtest_module.py\x1b[0m:\x1b[33m63\x1b[0m(\x1b[36m<module>\x1b[0m)\n         0/1377       0.0         0.000      80.5       137.700  \x1b[32mrunpy.py\x1b[0m:\x1b[33m98\x1b[0m(\x1b[36m_run_module_code\x1b[0m)\n         0/1377       0.0         0.000      80.5       137.700  \x1b[32mrunpy.py\x1b[0m:\x1b[33m226\x1b[0m(\x1b[36mrun_module\x1b[0m)\n         2/1338       0.1         0.200      78.2       133.800  \x1b[32mtest_module.py\x1b[0m:\x1b[33m52\x1b[0m(\x1b[36mmain_loop\x1b[0m)\n        746/746      43.6        74.600      43.6        74.600  \x1b[32mtest_module.py\x1b[0m:\x1b[33m15\x1b[0m(\x1b[36mcpu_intensive_work\x1b[0m)\n        404/404      23.6        40.400      23.6        40.400  \x1b[32mtest_module.py\x1b[0m:\x1b[33m16\x1b[0m(\x1b[36mcpu_intensive_work\x1b[0m)\n          0/334       0.0         0.000      19.5        33.400  \x1b[32m<frozen importlib._bootstrap_external>\x1b[0m:\x1b[33m1003\x1b[0m(\x1b[36mSourceFileLoader.set_data\x1b[0m)\n          0/334       0.0         0.000      19.5        33.400  \x1b[32m<frozen importlib._bootstrap_external>\x1b[0m:\x1b[33m978\x1b[0m(\x1b[36mSourceFileLoader._cache_bytecode\x1b[0m)\n          0/334       0.0         0.000      19.5        33.400  \x1b[32m<frozen importlib._bootstrap_external>\x1b[0m:\x1b[33m910\x1b[0m(\x1b[36mSourceLoader.get_code\x1b[0m)\n          0/334       0.0         0.000      19.5        33.400  \x1b[32mrunpy.py\x1b[0m:\x1b[33m159\x1b[0m(\x1b[36m_get_module_details\x1b[0m)\n\n\x1b[1;34mLegend:\x1b[0m\n  \x1b[33mnsamples\x1b[0m: Direct/Cumulative samples (direct executing / on call stack)\n  \x1b[33msample%\x1b[0m: Percentage of total samples this function was directly executing\n  \x1b[33mtottime\x1b[0m: Estimated total time spent directly in this function\n  \x1b[33mcumul%\x1b[0m: Percentage of total samples when this function was on the call stack\n  \x1b[33mcumtime\x1b[0m: Estimated cumulative time (including time in called functions)\n  \x1b[33mfilename:lineno(function)\x1b[0m: Function locati
\x1b[0m)\n\n\x1b[1;34mFunctions with Highest Call Magnification (Cumulative/Direct):\x1b[0m\n  669.0x call magnification, 1336 indirect calls from 2 direct: \x1b[32mtest_module.py\x1b[0m:\x1b[33m\x1b[0m(\x1b[36mmain_loop\x1b[0m)\n'


Traceback (most recent call last):
  File "C:\Users\Administrator\buildarea\3.x.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\test\test_profiling\test_sampling_profiler.py", line 2059, in test_sample_target_module
    self.assertIn("slow_fibonacci", output)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'slow_fibonacci' not found in 'Captured 2010 samples in 1.00 seconds\nSample rate: 2009.31 samples/sec\nError rate: 2.74%\nWarning: missed 7990 samples from the expected total of 10000 (79.90%)\n\x1b[1;34mProfile Stats:\x1b[0m\n\x1b[1;34m       nsamples\x1b[0m  \x1b[1;34m sample%\x1b[0m  \x1b[1;34mtottime (ms)\x1b[0m  \x1b[1;34m  cumul%\x1b[0m  \x1b[1;34mcumtime (ms)\x1b[0m  \x1b[1;34mfilename:lineno(function)\x1b[0m\n         0/3457       0.0         0.000     176.8       345.700  \x1b[32mrunpy.py\x1b[0m:\x1b[33m88\x1b[0m(\x1b[36m_run_code\x1b[0m)\n         0/1955       0.0         0.000     100.0       195.500  \x1b[32m_sync_coordinator.py\x1b[0m:\x1b[33m144\x1b[0m(\x1b[36m_execute_module\x1b[0m)\n         0/1955       0.0         0.000     100.0       195.500  \x1b[32m_sync_coordinator.py\x1b[0m:\x1b[33m229\x1b[0m(\x1b[36mmain\x1b[0m)\n         0/1955       0.0         0.000     100.0       195.500  \x1b[32m_sync_coordinator.py\x1b[0m:\x1b[33m251\x1b[0m(\x1b[36m<module>\x1b[0m)\n         0/1955       0.0         0.000     100.0       195.500  \x1b[32mrunpy.py\x1b[0m:\x1b[33m198\x1b[0m(\x1b[36m_run_module_as_main\x1b[0m)\n         0/1502       0.0         0.000      76.8       150.200  \x1b[32mtest_module.py\x1b[0m:\x1b[33m63\x1b[0m(\x1b[36m<module>\x1b[0m)\n         0/1502       0.0         0.000      76.8       150.200  \x1b[32mrunpy.py\x1b[0m:\x1b[33m98\x1b[0m(\x1b[36m_run_module_code\x1b[0m)\n         0/1502       0.0         0.000      76.8       150.200  \x1b[32mrunpy.py\x1b[0m:\x1b[33m226\x1b[0m(\x1b[36mrun_module\x1b[0m)\n         0/1469       0.0         0.000      75.1       146.900  \x1b[32mtest_module.py\x1b[0m:\x1b[33m52\x1b[0m(\x1b[36mmain_loop\x1b[0m)\n        809/809      41.4        80.900      41.4        80.900  \x1b[32mtest_module.py\x1b[0m:\x1b[33m15\x1b[0m(\x1b[36mcpu_intensive_work\x1b[0m)\n        457/457      23.4        45.700      23.4        45.700  \x1b[32mtest_module.py\x1b[0m:\x1b[33m16\x1b[0m(\x1b[36mcpu_intensive_work\x1b[0m)\n          0/453       0.0         0.000      23.2        45.300  \x1b[32mrunpy.py\x1b[0m:\x1b[33m159\x1b[0m(\x1b[36m_get_module_details\x1b[0m)\n          0/453       0.0         0.000      23.2        45.300  \x1b[32mrunpy.py\x1b[0m:\x1b[33m222\x1b[0m(\x1b[36mrun_module\x1b[0m)\n          0/452       0.0         0.000      23.1        45.200  \x1b[32m<frozen importlib._bootstrap_external>\x1b[0m:\x1b[33m978\x1b[0m(\x1b[36mSourceFileLoader._cache_bytecode\x1b[0m)\n          0/452       0.0         0.000      23.1        45.200  \x1b[32m<frozen importlib._bootstrap_external>\x1b[0m:\x1b[33m910\x1b[0m(\x1b[36mSourceLoader.get_code\x1b[0m)\n\n\x1b[1;34mLegend:\x1b[0m\n  \x1b[33mnsamples\x1b[0m: Direct/Cumulative samples (direct executing / on call stack)\n  \x1b[33msample%\x1b[0m: Percentage of total samples this function was directly executing\n  \x1b[33mtottime\x1b[0m: Estimated total time spent directly in this function\n  \x1b[33mcumul%\x1b[0m: Percentage of total samples when this function was on the call stack\n  \x1b[33mcumtime\x1b[0m: Estimated cumulative time (including time in called functions)\n  \x1b[33mfilename:lineno(function)\x1b[0m: Function location and name\n\n\x1b[1;34mSummary of Interesting Functions:\x1b[0m\n\n\x1b[1;34mFunctions with Highest Direct/Cumulative Ratio (Hot Spots):\x1b[0m\n  1.000 direct/cumulative ratio, 64.8% direct samples: \x1b[32mtest_module.py\x1b[0m:\x1b[33m\x1b[0m(\x1b[36mcpu_intensive_work\x1b[0m)\n\n\x1b[1;34mFunctions with Highest Call Frequency (Indirect Calls):\x1b[0m\n  3457 indirect calls, 176.8% total stack presence: \x1b[32mrunpy.py\x1b[0m:\x1b[33m\x1b[0m(\x1b[36m_run_code\x1b[0m)\n  1955 indirect calls, 100.0% total stack presence: \x1b[32m_sync_coordinator.py\x1b[0m:\x1b[33m\x1b[0m(\x1b[36m_execute_module\x1b[0m)\n  1955 indirect calls, 100.0% total stack presence: \x1b[32m_sync_coordinator.py\x1b[0m:\x1b[33m\x1b[0m(\x1b[36mmain\x1b[0m)\n\n\x1b[1;34mFunctions with Highest Call Magnification (Cumulative/Direct):\x1b[0m\n'

@stefanor stefanor deleted the hppa-stack branch November 17, 2025 14:24
@stefanor
Copy link
Contributor Author

I don't think those regressions are related.

@encukou
Copy link
Member

encukou commented Nov 17, 2025

Yeah, for the profiling #139485 or #141108 look like more likely causes, and asyncio looks flaky again on the Debian :(

gpshead pushed a commit that referenced this pull request Nov 23, 2025
…141404)

* [3.14] GH-139914: Handle stack growth direction on HPPA (GH-140028)

Adapted from a patch for Python 3.14 submitted to the Debian BTS by John David Anglin https://bugs.debian.org/1105111#20

* Forgot to update test_call

* WTF typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

9 participants