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.

Sign up for GitHub

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

Jump to bottom

An opcode optimize error in Python 3.12.0 #112356

Closed
tiwb opened this issue Nov 24, 2023 · 2 comments
Closed

An opcode optimize error in Python 3.12.0 #112356

tiwb opened this issue Nov 24, 2023 · 2 comments
Labels
3.11 only security fixes 3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@tiwb
Copy link

tiwb commented Nov 24, 2023 •
edited by bedevere-app bot

Bug report

Bug description:

import dis

def _f(a):
	print(a)

dis.dis(_f)

Here is a test in Python 3.12 test_dis.py BytecodeTests.test_disassembled, It outputs:

  3           0 RESUME                   0

  4           2 LOAD_GLOBAL              1 (NULL + print)
             12 LOAD_FAST                0 (a)
             14 CALL                     1
             22 POP_TOP
             24 RETURN_CONST             0 (None)

source code for this optimization is at: https://github.com/python/cpython/blob/3.12/Python/flowgraph.c#L1556-L1560

            case PUSH_NULL:
                if (nextop == LOAD_GLOBAL && (inst[1].i_opcode & 1) == 0) {
                    INSTR_SET_OP0(inst, NOP);
                    inst[1].i_oparg |= 1;
                }
                break;

I feel the correct check should be:

if (nextop == LOAD_GLOBAL && (inst[1].i_oparg & 1) == 0)

I'm unable to write a unit test for this code to make some real error, but if opcode for LOAD_GLOBAL is changed to some odd number, test_dis will fail.

CPython versions tested on:

3.12

Operating systems tested on:

Windows

Linked PRs
@tiwb tiwb added the type-bug An unexpected behavior, bug, or error label Nov 24, 2023
@Eclips4 Eclips4 added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 24, 2023
@neonene
Copy link
Contributor

neonene commented Nov 27, 2023 •
edited

It seems to me that inst[1].i_oparg here is supposed to be an even number via compile.c. If you can hit some quantity of an odd number, the tuning for 3.11 and 3.12 might be attractive for CPython team.

@bedevere-app bedevere-app bot mentioned this issue Nov 30, 2023
Merged
@sweeneyde
Copy link
Member

This line dates back to 3011a09. It was fixed in main by a9caf9c, but the inst[1].i_opcode & 1 still exists in 3.11 and 3.12.

I feel the correct check should be: if (nextop == LOAD_GLOBAL && (inst[1].i_oparg & 1) == 0)

I agree--this is likely the mistake.

I'm unable to write a unit test for this code to make some real error, but if opcode for LOAD_GLOBAL is changed to some odd number, test_dis will fail.

#112566 uses test_peepholer.DirectCfgOptimizerTests

@AlexWaygood AlexWaygood added 3.11 only security fixes 3.12 bugs and security fixes labels Nov 30, 2023
sweeneyde added a commit that referenced this issue Dec 1, 2023
@sweeneyde
[3.12] gh-112356: LOAD_GLOBAL can only include one PUSH_NULL (#112566)
a99305e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.11 only security fixes 3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@iritkatriel @tiwb @sweeneyde @neonene @AlexWaygood @Eclips4