Skip to content
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.

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

GH-103488: Use return-offset, not yield-offset. #103502

Merged
merged 4 commits into from
Apr 13, 2023

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Apr 13, 2023

This ensures that the callers instruction pointer is correct when sending to a generator or coroutine.

This is necessary for exception handling, and nice for debuggers and profilers.

Instead of returning to prev_instr +1 and yielding to prev_instr - yield_offset + 1 we now return to prev_instr + return_offset + 1 and yield to prev_instr +1.
These are equivalent unless there is an exception, or when introspecting the stack.

Since call/returns are more common than send/yields this will be a tiny bit slower than main, but reverting to the 3.11 approach would a lot slower.
Compared to the 3.11, this makes call/returns a few percent slower and send/yields an order of magnitude faster.

@@ -1724,6 +1727,7 @@ dummy_func(
STACK_SHRINK(shrink_stack);
new_frame->localsplus[0] = owner;
JUMPBY(INLINE_CACHE_ENTRIES_LOAD_ATTR);
frame->return_offset = 0;
Copy link
Member

Choose a reason for hiding this comment

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

What are the rules about when this needs to be 0ed and when it doesn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

return_offset is only meaningful to the callee, so it needs to be set in any CALL (to a Python function) or SEND (to a coroutine or generator).
If there is no callee, then it is meaningless.

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've added a comment in pycore_frame.h

@Eclips4
Copy link
Member

Eclips4 commented Apr 13, 2023

Also, this PR fixes #101914, output from pylint using example:

(venv) eclips4@HOME-PC:~/ide/cpython$ pylint test.py
************* Module test
test.py:1:0: C0114: Missing module docstring (missing-module-docstring)
test.py:1:4: E0602: Undefined variable 'b' (undefined-variable)
test.py:2:0: E0602: Undefined variable 'func' (undefined-variable)

------------------------------------------------------------------
Your code has been rated at 0.00/10 (previous run: 0.00/10, +0.00)

@mdboom can you confirm that? =)

@markshannon markshannon merged commit efb8a25 into python:main Apr 13, 2023
carljm added a commit to carljm/cpython that referenced this pull request Apr 13, 2023
* main:
  pythongh-103479: [Enum] require __new__ to be considered a data type (pythonGH-103495)
  pythongh-103365: [Enum] STRICT boundary corrections (pythonGH-103494)
  pythonGH-103488: Use return-offset, not yield-offset. (pythonGH-103502)
  pythongh-103088: Fix test_venv error message to avoid bytes/str warning (pythonGH-103500)
  pythonGH-103082: Turn on branch events for FOR_ITER instructions. (python#103507)
  pythongh-102978: Fix mock.patch function signatures for class and staticmethod decorators (python#103228)
  pythongh-103462: Ensure SelectorSocketTransport.writelines registers a writer when data is still pending (python#103463)
  pythongh-95299: Rework test_cppext.py to not invoke setup.py directly (python#103316)
@mdboom
Copy link
Contributor

mdboom commented Apr 20, 2023

@mdboom can you confirm that? =)

Yes, confirmed. I closed #101914 as a result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants