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

MatthieuDartiailh · 2023-09-05T08:09:42Z

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/

carljm

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

Doc/library/dis.rst

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

carljm

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

Doc/library/dis.rst

iritkatriel · 2023-09-25T18:14:23Z

Doc/library/dis.rst

 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


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.

Doc/library/dis.rst

MatthieuDartiailh · 2023-10-17T10:39:23Z

Can I do anything to help merge this work ?

iritkatriel · 2023-10-17T10:55:31Z

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

iritkatriel · 2023-10-17T10:56:38Z

Doc/library/dis.rst

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


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

miss-islington-app · 2023-10-17T12:59:39Z

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>

bedevere-app · 2023-10-17T12:59:54Z

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

Doc/library/dis.rst Outdated Show resolvedHide resolved
Doc/library/dis.rst Outdated Show resolvedHide resolved
Doc/library/dis.rst Outdated Show resolvedHide resolved
Doc/library/dis.rst Outdated Show resolvedHide resolved

bedevere-appbot 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-appbot 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

Doc/library/dis.rst Outdated Show resolvedHide resolved

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-appbot 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.12Oct 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.12Oct 17, 2023

iritkatriel enabled auto-merge (squash) October 17, 2023 12:50

iritkatriel merged commit 198aa67 into python:mainOct 17, 2023

bedevere-appbot 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-appbot 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)

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

Uh oh!

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

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

Uh oh!

Conversation

MatthieuDartiailh commented Sep 5, 2023• edited by github-actions botLoading 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-appbot commented Oct 17, 2023

Uh oh!

bedevere-appbot commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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