GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-132508: Use tagged integers on the evaluation stack for the last instruction offset by markshannon · Pull Request #132545 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Apr 15, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

When reraising in a finally block, the exception needs to look as if it were raised from an earlier point in the code.
To do this we save the earlier instruction offset as an integer on the evaluation stack.
Currently, this requires boxing the integer, which can (extremely rarely) fail.
By using a tagged integer we can avoid that failure mode.

This is might seem like an elaborate fix for a very minor issue, and it is, but we will want tagged integers/pointers for many other things and this is a nice small step to that larger change.

See #132509

  • Issue: Exception unwinding might be broken when handling a memory error. #132508

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
markshannon added 4 commits April 14, 2025 14:26
@markshannon
Use tagged integers on the evaluation stack for lasti
ba30e9a
@markshannon
Add support for Py_STACKREF_DEBUG
1d223c3
@markshannon
Add news
246199b
@markshannon
Fix dump_item
b1b1252
@markshannon
markshannon requested review from Fidget-Spinner and ericsnowcurrently as code owners April 15, 2025 09:25
@bedevere-app bedevere-app Bot added the awaiting core review label Apr 15, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Apr 15, 2025
Exception unwinding might be broken when handling a memory error. #132508
Closed
Fidget-Spinner
Fidget-Spinner reviewed Apr 15, 2025
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

2 comments

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/pystate.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Apr 15, 2025

Copy link
Copy Markdown
Member Author

As expected, performance is neutral.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Clarify division between pre-defined stackref indexes and others
8d3c3ea
Fidget-Spinner
Fidget-Spinner reviewed Apr 15, 2025
View reviewed changes
Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Cast before shifting
aa4ddf0
brandtbucher
brandtbucher reviewed Apr 15, 2025
View reviewed changes
Comment thread Python/executor_cases.c.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/generated_cases.c.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher
brandtbucher previously requested changes Apr 15, 2025
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I believe the magic number needs to be updated (and everything regenerated too) since the exception_unwind label now pushes tagged ints.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting core review labels Apr 15, 2025
@bedevere-app

bedevere-app Bot commented Apr 15, 2025

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Apr 15, 2025

Copy link
Copy Markdown
Member Author

I believe the magic number needs to be updated (and everything regenerated too) since the exception_unwind label now pushes tagged ints.

Nothing has changed in terms of stack effects. The only change is that we push a tagged int instead of a boxed one.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher
brandtbucher dismissed their stale review April 15, 2025 16:43

Ah, nevermind.

@bedevere-app bedevere-app Bot added awaiting review and removed awaiting changes labels Apr 15, 2025
markshannon added 4 commits April 15, 2025 17:47
@markshannon
Integer tagging doesn't escape
e223e36
@markshannon
Push assert up to caller
9e11cc7
@markshannon
Avoid side effects in macro
79ed469
@markshannon
Remove debug code
b491544
@markshannon

markshannon commented Apr 16, 2025

Copy link
Copy Markdown
Member Author

The failure on JIT/ARM64 look like a JIT bug. The assertion errors seem impossible and do not occur on any other platform.
Neither the ARM64 (JIT) interpreter or the other JIT builds show any of the failures.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Apr 16, 2025

Copy link
Copy Markdown
Member

I think I've seen that JIT failure before on my old PR when I used a different tagging scheme.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner reviewed Apr 16, 2025
View reviewed changes
Comment thread Python/stackrefs.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner reviewed Apr 16, 2025
View reviewed changes
Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Handle negative ints properly in Py_STACKREF_DEBUG build
f6126b1
Fidget-Spinner
Fidget-Spinner approved these changes Apr 16, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Apr 16, 2025
@markshannon

markshannon commented Apr 16, 2025

Copy link
Copy Markdown
Member Author

The JIT ARM 64 windows failure is just a timeout for the int repr test that sometimes happens.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher

brandtbucher commented Apr 17, 2025

Copy link
Copy Markdown
Member

This isn't a JIT problem, it's a tier two problem (I can reproduce on an M2 Mac with --with-pydebug --enable-experimental-jit=interpreter). Digging in deeper...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher

brandtbucher commented Apr 17, 2025

Copy link
Copy Markdown
Member

When entering tier two, the current frame's f_executable is 0x0000000000000002. Which is, uh, not a valid stackref. It looks suspiciously like BITS_TO_PTR_MASKED(PyStackRef_TagInt(0)).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher

brandtbucher commented Apr 18, 2025

Copy link
Copy Markdown
Member

Never mind, it looks like the bug is in the JIT code. I believe this is because the Clang is not actually inlining the some _PyFrame_GetCode calls used for stack_pointer sanity checks on this build. Instead, it’s emitting the code for the function alongside the normal bytecode, and making calls to that. My guess is it’s using some clever calling convention or something and we’re messing up how it’s being called.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher

brandtbucher commented Apr 18, 2025

Copy link
Copy Markdown
Member

@markshannon, this fixes it:

diff --git a/Tools/jit/_stencils.py b/Tools/jit/_stencils.py
index 8faa9e8cac2..639e4bcc793 100644
--- a/Tools/jit/_stencils.py
+++ b/Tools/jit/_stencils.py
@@ -291,6 +291,7 @@ def process_relocations(
                 hole.kind
                 in {"R_AARCH64_CALL26", "R_AARCH64_JUMP26", "ARM64_RELOC_BRANCH26"}
                 and hole.value is HoleValue.ZERO
+                and hole.symbol not in self.symbols
             ):
                 hole.func = "patch_aarch64_trampoline"
                 hole.need_state = True
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher

brandtbucher commented Apr 18, 2025

Copy link
Copy Markdown
Member

For anyone curious: on this particular build, Clang doesn't inline one of the calls to _PyFrame_GetCode, and instead emits it alongside our _JIT_ENTRY function. This is fine, but our handling of jumps on AArch64 macOS (which inserts trampolines for out-of-range jumps) meant that we tried to "link against" the _PyFrame_GetCode function in the main executable, rather than the (slightly different!) version the compiler emitted as part of the stencil.

Though they both should be identical, my guess is that Clang realized that the function was static and only called in one place by the bytecode, and didn't use an ABI-conforming calling convention. If we change the logic to check for duplicate local symbols in the template before trying to link to the main executable (which is what my change does), then we end up calling the intended version of the function.

(And, in case anyone was wondering: lldb is useless in our JIT code, and printf-debugging didn't help at all in this situation. It was basically just looking at the template we generated alongside the disassembly that led to the "aha" moment.)

Zheaoli reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Brandt's fix to the stencils
1b4aba6
@markshannon
markshannon requested a review from savannahostrowski as a code owner April 28, 2025 17:09
@brandtbucher

brandtbucher commented Apr 28, 2025

Copy link
Copy Markdown
Member

@markshannon, you applied my patch to the wrong line.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Put Brandt's patch in the right place :)
d84cd2d
@markshannon markshannon mentioned this pull request Apr 29, 2025
Use tagged integers on the evaluation stack for lasti #132405
Closed
iritkatriel
iritkatriel reviewed Apr 29, 2025
View reviewed changes
Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Apr 29, 2025
View reviewed changes
Comment thread Include/internal/pycore_stackref.h Outdated
PyObject *obj = _Py_stackref_get_object(ref);
Py_INCREF(obj);
return _Py_stackref_create(obj, filename, linenumber);
if (ref.index & 1) {

@iritkatriel iritkatriel Apr 29, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
if (ref.index & 1) {
if (is_tagged_int(ref)) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@markshannon
Replace is_tagged_int with PyStackRef_IsTaggedInt
fb89cd9
iritkatriel
iritkatriel approved these changes Apr 29, 2025
View reviewed changes
@markshannon
markshannon merged commit ccf1b0b into python:main Apr 29, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 29, 2025
@markshannon
markshannon deleted the fix-132508 branch April 29, 2025 17:00
@chris-eibl chris-eibl mentioned this pull request Sep 6, 2025
[3.13] gh-134163: fix: exec('_testcapi.set_nomemory(0)') hang in new repl 3.13 #138491
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes
@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes
@brandtbucher brandtbucher brandtbucher left review comments
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@savannahostrowski savannahostrowski Awaiting requested review from savannahostrowski

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@markshannon @Fidget-Spinner @brandtbucher @iritkatriel
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.