gh-107457: update dis documentation with changes in 3.12 by MatthieuDartiailh · Pull Request #108900 · python/cpython · GitHub

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/

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

Co-authored-by: Carl Meyer <carl@oddbird.net>

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.

requested changes were made

What is an "instruction leading to a jump"?

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.

I don't understand what you mean, so I can't offer how to reword it.

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.

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" ?

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.

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

Sure

Can I do anything to help merge this work ?

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

Suggested change

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

I can see you copied this from the 1-arg opcode. But I think it's redundant in both cases.

I will update it

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

…nGH-108900) (cherry picked from commit 198aa67) Co-authored-by: Matthieu Dartiailh <m.dartiailh@gmail.com>

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

…08900) (#110985) gh-107457: update dis documentation with changes in 3.12 (GH-108900) (cherry picked from commit 198aa67) Co-authored-by: Matthieu Dartiailh <m.dartiailh@gmail.com>

…n#108900)

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

7617456

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

Comment thread Doc/library/dis.rst Outdated

Comment thread Doc/library/dis.rst Outdated

Comment thread Doc/library/dis.rst Outdated

Comment thread Doc/library/dis.rst Outdated

bedevere-app Bot added awaiting core review and removed awaiting review labels Sep 25, 2023

Apply suggestions from code review

3644200

Co-authored-by: Carl Meyer <carl@oddbird.net>

carljm reviewed Sep 25, 2023

View reviewed changes

bedevere-app Bot added awaiting review and removed awaiting core review labels Sep 25, 2023

iritkatriel reviewed Sep 25, 2023

View reviewed changes

brandtbucher reviewed Sep 25, 2023

View reviewed changes

Comment thread Doc/library/dis.rst Outdated

Update dis.rst

0a1dee2

iritkatriel reviewed Oct 17, 2023

View reviewed changes

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 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 <m.dartiailh@gmail.com>

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

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)

-                 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::

                     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

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

gh-107457: update dis documentation with changes in 3.12 by MatthieuDartiailh · Pull Request #108900 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-107457: update dis documentation with changes in 3.12 - #108900

gh-107457: update dis documentation with changes in 3.12#108900iritkatriel merged 9 commits intopython:mainpython/cpython:mainfrom MatthieuDartiailh:dis-docsMatthieuDartiailh/cpython:dis-docsCopy head branch name to clipboard

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gh-107457: update dis documentation with changes in 3.12#108900
iritkatriel merged 9 commits into
python:mainfrom
MatthieuDartiailh:dis-docs

MatthieuDartiailh commented Sep 5, 2023 •
edited by github-actions Bot
Loading