GeezerVison

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

GeezerVison is showing github.com — gh-107457: update dis documentation with changes in 3.12 by MatthieuDartiailh · Pull Request #108900 · python/cpython

Conversation

MatthieuDartiailh commented Sep 5, 2023 •

edited by github-actions Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

This PR addresses the following points of #107457 which should be backported to 3.12

document MIN_INSTRUMENTED_OPCODE which is needed to determine what are the "normal" opcodes
clarify the LOAD_SUPER_ATTR stack manipulation description
fix the description of CALL_INSTRINSIC_2 stack manipulation
document END_SEND
explain how the presence of CACHE instructions impact the computation of jump instruction argument

I did not address the following points:

the description of COMPARE_OP which was corrected in 3.13. However 3.13 does not detail the meaning of the cache: should it be detailed
POP_JUMP_IF_NOT_NONE and POP_JUMP_IF_NONE were not described as actual instructions but this appear to have been fixed at least in 3.13 if it is not in 3.12 it should be backported

I plan to address the What's new issues in a follow up PR

Issue: Bytecode documentation issues for Python 3.12 #107457

📚 Documentation preview 📚: https://cpython-previews--108900.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

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

P403n1x87 reacted with hooray emoji

All reactions

🎉 1 reaction

MatthieuDartiailh added 5 commits

September 4, 2023 16:16


          doc: describe how CACHE opcodes are taken into account in jumps

34250f3


          doc: document MIN_INSTRUMENTED_OPCODE in dis

e7d9021


          doc: document END_SEND and fix SEND documentation

0586fec


          Doc: clarify CALL_INSTRINSIC_2 stack effect


          doc: clarify the stack manipulation of LOAD_SUPER_ATTR

6ad38e3

bedevere-bot added awaiting review docs

Documentation in the Doc dir

skip news labels

Sep 5, 2023

bedevere-bot mentioned this pull request

Sep 5, 2023

Bytecode documentation issues for Python 3.12 #107457

Closed

20 tasks

AA-Turner added the needs backport to 3.12 only security fixes label

Sep 6, 2023

AA-Turner requested review from carljm, gvanrossum and iritkatriel

September 25, 2023 06:53

AA-Turner mentioned this pull request

Sep 25, 2023

GH-109190: Copyedit 3.12 What's New: Bytecode #109821

Merged

carljm previously requested changes

Sep 25, 2023

View reviewed changes

carljm 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.

The LOAD_SUPER_ATTR changes look OK to me, modulo suggested edits.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Doc/library/dis.rst Resolved

Uh oh!

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

Comment thread

Doc/library/dis.rst Resolved

Uh oh!

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

Comment thread

Doc/library/dis.rst Resolved

Uh oh!

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

Comment thread

Doc/library/dis.rst Resolved

Uh oh!

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

bedevere-app Bot added awaiting core review and removed awaiting review labels

Sep 25, 2023


          Apply suggestions from code review

Co-authored-by: Carl Meyer <[email protected]>

carljm reviewed

Sep 25, 2023

View reviewed changes

carljm 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.

The LOAD_SUPER_ATTR changes look good to me now. The other changes look reasonable, but I'd rather get @markshannon or @iritkatriel to confirm the accuracy of the new paragraph on 3.12 jump changes.

Sorry, something went wrong.

Uh oh!

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

All reactions

carljm dismissed their stale review

September 25, 2023 17:52

requested changes were made

bedevere-app Bot added awaiting review and removed awaiting core review labels

Sep 25, 2023

iritkatriel reviewed

Sep 25, 2023

View reviewed changes

Comment thread

Doc/library/dis.rst Resolved

Uh oh!

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

Comment thread

Doc/library/dis.rst Outdated

		adaptive bytecode can be shown by passing ``adaptive=True``.

		.. versionchanged:: 3.12
		For instruction leading to a jump, a jump by 0 instruction should always

iritkatriel Sep 25, 2023

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.

What is an "instruction leading to a jump"?

Sorry, something went wrong.

Uh oh!

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

All reactions

MatthieuDartiailh Sep 28, 2023

Copy link

Copy Markdown

Contributor Author

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 agree it is a bit convoluted. I am happy to use a different phrasing. Initially I went for this because of opcode like FOR_ITER that do not jump often.

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Sep 28, 2023

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 don't understand what you mean, so I can't offer how to reword it.

Sorry, something went wrong.

Uh oh!

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

All reactions

MatthieuDartiailh Oct 17, 2023

Copy link

Copy Markdown

Contributor Author

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.

Would the following re-wording read better ?

When the offset of a jump instruction is 0, the jump should always lead to the instruction directly following the jump instruction. Starting with 3.12, some jump instructions can be followed by a :opcode:CACHE instruction. When the argument of such an instruction is 0, it will jump to the first non-CACHE instruction following the jump instruction.

As a consequence, the presence of the :opcode:CACHE instructions is transparent for forward jumps but need to be taken into account when reasoning about backward jumps.

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Oct 17, 2023

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 think I see what you mean now.

Can we just say that "the arg of a jump is the offset from the instruction that appears immediately after the JUMP instruction's CACHE entries" ?

Sorry, something went wrong.

Uh oh!

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

All reactions

MatthieuDartiailh Oct 17, 2023

Copy link

Copy Markdown

Contributor Author

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.

Can we agree on the following ? I would prefer to insist on the forward backward asymmetry

The argument of a jump is the offset of the target relative to the instruction that appears immediately after the jump instruction's CACHE entries.

As a consequence, the presence of the CACHE instructions is transparent for forward jumps but need to be taken into account when reasoning about backward jumps.

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Oct 17, 2023

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.

Sure. I think "need --> needs" though.

Sorry, something went wrong.

Uh oh!

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

All reactions

MatthieuDartiailh Oct 17, 2023

Copy link

Copy Markdown

Contributor Author

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.

Sure

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Doc/library/dis.rst Resolved

Uh oh!

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

brandtbucher reviewed

Sep 25, 2023

View reviewed changes

Comment thread

Doc/library/dis.rst Resolved

Uh oh!

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


          Update dis.rst

0a1dee2

MatthieuDartiailh commented Oct 17, 2023

Copy link

Copy Markdown

Contributor Author

Can I do anything to help merge this work ?

All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel commented Oct 17, 2023

Copy link

Copy Markdown

Member

There was this question: https://github.com/python/cpython/pull/108900/files#r1336237016

All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel reviewed

Oct 17, 2023

View reviewed changes

Comment thread

Doc/library/dis.rst Outdated

		arguments and sets ``STACK[-1]`` to the result. Used to implement functionality that is
		necessary but not performance critical.
		Calls an intrinsic function with two arguments. Used to implement functionality
		that is necessary but not performance critical::

iritkatriel Oct 17, 2023

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

	that is necessary but not performance critical::
	that is not performance critical::

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Oct 17, 2023

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 can see you copied this from the 1-arg opcode. But I think it's redundant in both cases.

Sorry, something went wrong.

Uh oh!

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

All reactions

MatthieuDartiailh Oct 17, 2023

Copy link

Copy Markdown

Contributor Author

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 will update it

Sorry, something went wrong.

Uh oh!

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

All reactions


          docs: address review comments

5a93f49

iritkatriel approved these changes

Oct 17, 2023

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels

Oct 17, 2023


          Merge branch 'main' into dis-docs

4291f95

iritkatriel changed the title ~~gh-107457: Improve dis documentation~~ gh-107457: update dis documentation with some of the changes in 3.12

Oct 17, 2023

iritkatriel changed the title ~~gh-107457: update dis documentation with some of the changes in 3.12~~ gh-107457: update dis documentation with changes in 3.12

Oct 17, 2023

iritkatriel enabled auto-merge (squash)

October 17, 2023 12:50

iritkatriel merged commit 198aa67 into python:main

Oct 17, 2023

bedevere-app Bot removed the awaiting merge label

Oct 17, 2023

miss-islington-app Bot commented Oct 17, 2023

Copy link

Copy Markdown

Thanks @MatthieuDartiailh for the PR, and @iritkatriel for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Oct 17, 2023


          pythongh-107457: update dis documentation with changes in 3.12 (pytho…

02c489d

…nGH-108900)

(cherry picked from commit 198aa67)

Co-authored-by: Matthieu Dartiailh <[email protected]>

bedevere-app Bot commented Oct 17, 2023

Copy link

Copy Markdown

GH-110985 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the needs backport to 3.12 only security fixes label

Oct 17, 2023

MatthieuDartiailh deleted the dis-docs branch

October 17, 2023 15:04

vstinner pushed a commit that referenced this pull request

Oct 17, 2023


          [3.12] gh-107457: update dis documentation with changes in 3.12 (GH-1…

ae838af

…08900) (#110985)

gh-107457: update dis documentation with changes in 3.12 (GH-108900)
(cherry picked from commit 198aa67)

Co-authored-by: Matthieu Dartiailh <[email protected]>

aisk pushed a commit to aisk/cpython that referenced this pull request

Feb 11, 2024


          pythongh-107457: update dis documentation with changes in 3.12 (pytho…

b5a0c7d

…n#108900)

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-107457: update dis documentation with changes in 3.12 (pytho…

a132243

…n#108900)

Reviewers

carljm

carljm left review comments

brandtbucher

brandtbucher left review comments

iritkatriel

iritkatriel approved these changes

gvanrossum

Awaiting requested review from gvanrossum

Uh oh!

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

Assignees

No one assigned

Labels

docs

Documentation in the Doc dir

skip news

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.

6 participants

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.

Conversation

MatthieuDartiailh commented Sep 5, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MatthieuDartiailh commented Oct 17, 2023

Uh oh!

iritkatriel commented Oct 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington-app Bot commented Oct 17, 2023

Uh oh!

bedevere-app Bot commented Oct 17, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MatthieuDartiailh commented Sep 5, 2023 •

edited by github-actions Bot

Loading

Uh oh!

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