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-104584: Fix error handling from backedge optimization #106484

Merged
merged 3 commits into from Jul 6, 2023

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jul 6, 2023

When _PyOptimizer_BackEdge returns NULL, we should restore next_instr (and stack_pointer). To accomplish this we should jump to resume_with_error instead of just error.

The problem this causes is subtle -- the only repro I have is in PR gh-106393, at commit d7df54b. But the fix is real (as shown there).

While we're at it, also improve the debug output: the offsets at which traces are identified are now measured in bytes, and always show the start offset. This makes it easier to correlate executor calls with optimizer calls, and either with dis output.

This fixes a nasty bug in exception handling.
Apparently some part of the computation that determines
where the exception handler is uses next_instr,
which was pointing at the JUMP_BACKWARD instruction instead of
where the error was occurring.

Better fix for _PyOptimizer_BackEdge error handling
Offsets in debug output identifying the start of the trace
are now measured in bytes (and the text shows it).
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.

One minor issue, otherwise LGTM

Python/optimizer.c Show resolved Hide resolved
@gvanrossum gvanrossum enabled auto-merge (squash) July 6, 2023 17:48
@gvanrossum gvanrossum merged commit 003ba71 into python:main Jul 6, 2023
21 checks passed
@gvanrossum gvanrossum deleted the fix-backedge branch July 6, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants