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-109094: replace frame->prev_instr by frame->instr_ptr by iritkatriel · Pull Request #109095 · python/cpython

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Sep 7, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: Replace frame->prev_instr by frame->instr_ptr #109094

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot mentioned this pull request Sep 7, 2023
Replace frame->prev_instr by frame->instr_ptr #109094
Closed
iritkatriel added 14 commits September 7, 2023 15:43
@iritkatriel
add instr_ptr and assert it's 1 more than prev_instr in SET_LOCALS_FR…
637b5da 
…OM_FRAME
@iritkatriel
fix YIELD_VALUE
ffb980c
@iritkatriel
fix test_sys
82a5b92
@iritkatriel
update instr_ptr in _PyOptimizer_BackEdge
1fc0df1
@iritkatriel
All tests pass except test_jump_from_yield (which is disabled)
6bbb1c2
@iritkatriel
assert always
be6d428
@iritkatriel
remove unnecessary stuff
bbe8e2c
@iritkatriel
fix test_sys expected sizes
50a1f3a
@iritkatriel
add assert
4beb50b
@iritkatriel
use instr_ptr instead of prev_instr in genobject.c
8e4a310
@iritkatriel
update executor.c
58d3bc5
@iritkatriel
remove unecessary setting of prev_instr in _Py_call_instrumentation_jump
909de93
@iritkatriel
replace assignment by assertion in _Py_call_instrumentation_line
0a06b75
@iritkatriel
revert change to signature of _PyFrame_PushTrampolineUnchecked
da6c97b
@iritkatriel
iritkatriel force-pushed the instr_ptr branch from 7bb3b93 to da6c97b Compare September 7, 2023 14:43
iritkatriel added 6 commits September 7, 2023 18:50
@iritkatriel
prev_instr --> previous_instr to avoid confusion
6488d69
@iritkatriel
revert change to JUMPBY
6c684b0
@iritkatriel
use instr_ptr in _PyFrame_OpAlreadyRan
b6a8b0c
@iritkatriel
use instr_ptr in _PyFrame_IsIncomplete
1ceeba1
@iritkatriel
use instr_ptr in LOAD_IP
371fa66
@iritkatriel
use intr_ptr in INSTRUMENTED_RESUME
557f36f
@iritkatriel
iritkatriel force-pushed the instr_ptr branch from 2cfbdbf to 557f36f Compare September 11, 2023 10:31
iritkatriel added 7 commits September 11, 2023 13:26
@iritkatriel
Python/optimizer.c
cb22365
@iritkatriel
Merge remote-tracking branch 'upstream/main' into instr_ptr
9875704
@iritkatriel
tweaks
aea21a6
@iritkatriel
Merge remote-tracking branch 'upstream/main' into instr_ptr
b2221d1
@iritkatriel
skip test_long_loop for now
7078fe5
@iritkatriel
added harness for LASTI migration, but no functional change yet
3ac7cc6
@iritkatriel
WIP
5a672a4
gvanrossum
gvanrossum reviewed Oct 25, 2023
View reviewed changes
Comment thread Python/optimizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel
address (most of) Guido's comments
0b68e54
gvanrossum
gvanrossum reviewed Oct 25, 2023
View reviewed changes
Comment thread Python/ceval.c Resolved

Uh oh!

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

gvanrossum commented Oct 25, 2023

Copy link
Copy Markdown
Member

For me, the only thing that prevent this from being merged is the failing gdb test. Maybe you can get help on Discord? Mark should still review it one more time.
All reactions

Sorry, something went wrong.

Uh oh!

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

gvanrossum commented Oct 25, 2023

Copy link
Copy Markdown
Member

(When you turn on uops all the time there are some additional failures in test_embed, but those are also on main; it's the thing Brandt reported. I'm looking into a shorter repro.)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iritkatriel added 3 commits October 25, 2023 23:44
@iritkatriel
LOAD_IP takes the offset
96a996c
@iritkatriel
no need to zero return_offset
8574469
@iritkatriel
remove OPARG_SET_IP
855513a
@iritkatriel

iritkatriel commented Oct 25, 2023

Copy link
Copy Markdown
Member Author

For me, the only thing that prevent this from being merged is the failing gdb test. Maybe you can get help on Discord? Mark should still review it one more time.

I'll look into the gdb tests tomorrow.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel
fix test_gdb?
0cf9ec9
@iritkatriel

iritkatriel commented Oct 26, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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.
@AlexWaygood AlexWaygood mentioned this pull request Oct 26, 2023
Bedevere doesn't say "thank you" for making the requested changes anymore python/bedevere#599
Closed
markshannon
markshannon requested changes Oct 26, 2023
View reviewed changes

@markshannon markshannon 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.

A few minor comments and questions, but looks good overall.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/genobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_frame.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_frame.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_frame.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/ceval_macros.h Resolved

Uh oh!

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

markshannon commented Oct 26, 2023

Copy link
Copy Markdown
Member

All test except test_embed pass with export PYTHONUOPS=1
The test_embed failure looks like the same as on main.
iritkatriel 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.
iritkatriel added 2 commits October 26, 2023 12:24
@iritkatriel
move explanations to frame_layout.md
82f9d9d
@iritkatriel
Mark's comments
95b63df
@iritkatriel

iritkatriel commented Oct 26, 2023

Copy link
Copy Markdown
Member Author

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.
@iritkatriel
same tweak to INSTRUMENTED_YIELD_VALUE as well
f2d149b
@markshannon
markshannon self-requested a review October 26, 2023 13:10
markshannon
markshannon approved these changes Oct 26, 2023
View reviewed changes

@markshannon markshannon left a comment •
edited
Loading

Uh oh!

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

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.

Looks good. A definite improvement over the increasingly mis-named prev_instr

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 merge and removed awaiting changes labels Oct 26, 2023
@iritkatriel
Merge branch 'main' into instr_ptr
578e266
@iritkatriel
iritkatriel enabled auto-merge (squash) October 26, 2023 13:36
@iritkatriel
iritkatriel merged commit 67a91f7 into python:main Oct 26, 2023
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 26, 2023
@sobolevn sobolevn mentioned this pull request Oct 27, 2023
New warning: '=': conversion from 'unsigned __int64' to 'uint16_t' in generated_cases.c.h #111386
Closed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@iritkatriel @aisk
pythongh-109094: replace frame->prev_instr by frame->instr_ptr (pytho…
bc7df6b 
…n#109095)
@iritkatriel iritkatriel mentioned this pull request May 24, 2024
f_lasti points to CACHE opcode during inlined CALL #96970
Closed
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@iritkatriel @Glyphack
pythongh-109094: replace frame->prev_instr by frame->instr_ptr (pytho…
feb4455 
…n#109095)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments
@markshannon markshannon markshannon approved these changes
@brandtbucher brandtbucher Awaiting requested review from brandtbucher

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.

3 participants

@iritkatriel @gvanrossum @markshannon
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.