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-128045: Mark unknown opcodes as deopting to themselves by DinoV · Pull Request #128044 · python/cpython

Conversation

@DinoV

@DinoV DinoV commented Dec 17, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

When accessing co_code on a code object we'll first run through to do the de-opt https://github.com/python/cpython/blob/main/Objects/codeobject.c#L1706

This removes any unknown opcodes. Instead the deopt table should just recognize unknown opcodes as deopting to themselves, allowing the extensible interpreter loop to consume unknown opcodes.

  • Issue: code objects remove unknown opcodes from the instruction stream when accessing co_code #128045

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DinoV DinoV added the skip news label Dec 17, 2024
@DinoV DinoV changed the title Mark unknown opcodes as deopting to themselves gh-128045: Mark unknown opcodes as deopting to themselves Dec 17, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Dec 17, 2024
code objects remove unknown opcodes from the instruction stream when accessing co_code #128045
Closed
@DinoV
DinoV marked this pull request as ready for review December 17, 2024 20:12
@DinoV
DinoV requested a review from markshannon as a code owner December 17, 2024 20:12
@bedevere-app bedevere-app Bot added the awaiting core review label Dec 17, 2024
iritkatriel
iritkatriel reviewed Dec 17, 2024
View reviewed changes
Comment thread Tools/cases_generator/opcode_metadata_generator.py Outdated
Comment on lines +251 to +256
for name, deopt in sorted(deopts):
out.emit(f"[{name}] = {deopt},\n")
defined = set(analysis.opmap.values())
for i in range(256):
if i not in defined:
out.emit(f"[{i}] = {i},\n")

@iritkatriel iritkatriel Dec 17, 2024

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.

Since we're not testing this at all in cpython, I'd suggest we at least add a couple of assertions to make sure this is correctly covering the range of opcodes:

Suggested change
for name, deopt in sorted(deopts):
out.emit(f"[{name}] = {deopt},\n")
defined = set(analysis.opmap.values())
for i in range(256):
if i not in defined:
out.emit(f"[{i}] = {i},\n")
defined = set(analysis.opmap.values())
for i in range(256):
if i not in defined:
deopts.append((f'{i}', f'{i}'))
assert len(deopts) == 256
assert len(set(x[0] for x in deopts)) == 256
for name, deopt in sorted(deopts):
out.emit(f"[{name}] = {deopt},\n")

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DinoV reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
facebook-github-bot pushed a commit to facebookincubator/MetaPython that referenced this pull request Dec 18, 2024
@DinoV @facebook-github-bot
Mark undefined instructions as deopting to themselves
3d1b81e 
Summary:
When CPython hands out the bytecode it will first do a de-opt on it:

https://www.internalfb.com/code/fbsource/[241e0d0760d4c107a3c0ac2b4914524120b0c909]/third-party/python/3.12/Objects/codeobject.c?lines=1540&reveal=1540

This uses the de-opt table which only defines the known opcodes, meaning unknown opcodes get turned into 0's. We need CPython to at least define unknown opcodes to at least de-opt to themselves.

Upstream PR: python/cpython#128044

Reviewed By: jbower-fb

Differential Revision: D67350914

fbshipit-source-id: 0073efab52da1be775272e7dd9ae5a46468ccb10
@markshannon

markshannon commented Jan 7, 2025

Copy link
Copy Markdown
Member

I think we regard undefined instructions an error.
If you want to pass custom bytecodes to your custom interpreter, a more robust approach might be needed.
We can do the deopt thing for now, but it seems fragile.

For example, one thing I have considered is storing bytecodes in a compact format on disk: Instructions without an oparg would take 1 byte, those with an oparg would take 2. We would then combine the unmarshalling and quickening steps to create the full in-memory form in a single pass. Custom instructions would not survive this process.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DinoV
Mark unknown opcodes as deopting to themselves
e37182c
@DinoV
DinoV force-pushed the deopt_unknown_ops branch from 7048634 to a675bfa Compare April 2, 2025 17:28
@DinoV

DinoV commented Apr 2, 2025

Copy link
Copy Markdown
Contributor Author

Took me a while to get back to this but I'm finally back :) I applied the changes suggested by @iritkatriel.

I think we regard undefined instructions an error. If you want to pass custom bytecodes to your custom interpreter, a more robust approach might be needed. We can do the deopt thing for now, but it seems fragile.

For example, one thing I have considered is storing bytecodes in a compact format on disk: Instructions without an oparg would take 1 byte, those with an oparg would take 2. We would then combine the unmarshalling and quickening steps to create the full in-memory form in a single pass. Custom instructions would not survive this process.

I think as long as we could still have some way to construct a code object ourselves that would be fine, we'd just may need to implement our own custom unmarshaling logic that could support our opcodes. Obviously that doesn't cover all the ways that things could change in the future but we can try and figure out how to adapt to other potential changes :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DinoV
Add extra assertions on number of opcodes emitted
efe6990
@DinoV
DinoV force-pushed the deopt_unknown_ops branch from a675bfa to efe6990 Compare April 2, 2025 17:36
markshannon
markshannon approved these changes May 19, 2025
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.

Looks good

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 core review labels May 19, 2025
@DinoV DinoV added awaiting core review needs backport to 3.14 bugs and security fixes and removed awaiting merge labels May 19, 2025
@DinoV
DinoV merged commit cc9add6 into python:main May 19, 2025
@miss-islington-app

miss-islington-app Bot commented May 19, 2025

Copy link
Copy Markdown

Thanks @DinoV for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting core review label May 19, 2025
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2025
@DinoV @miss-islington
pythongh-128045: Mark unknown opcodes as deopting to themselves (pyth…
215207e 
…onGH-128044)

* Mark unknown opcodes as deopting to themselves
(cherry picked from commit cc9add6)

Co-authored-by: Dino Viehland <[email protected]>
@bedevere-app

bedevere-app Bot commented May 19, 2025

Copy link
Copy Markdown

GH-134228 is a backport of this pull request to the 3.14 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label May 19, 2025
DinoV pushed a commit that referenced this pull request May 19, 2025
@miss-islington
[3.14] gh-128045: Mark unknown opcodes as deopting to themselves (GH-…
c869898 
…128044) (#134228)

* gh-128045: Mark unknown opcodes as deopting to themselves (GH-128044)
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@DinoV @Pranjal095
pythongh-128045: Mark unknown opcodes as deopting to themselves (pyth…
76d7013 
…on#128044)

* Mark unknown opcodes as deopting to themselves
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@DinoV @taegyunkim
pythongh-128045: Mark unknown opcodes as deopting to themselves (pyth…
499aebc 
…on#128044)

* Mark unknown opcodes as deopting to themselves
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel left review comments
@markshannon markshannon markshannon approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

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.

3 participants

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