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.

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

Generate opcode metadata from bytecodes.c instead of opcode.py #105481

Closed
iritkatriel opened this issue Jun 7, 2023 · 9 comments · Fixed by #108367
Closed

Generate opcode metadata from bytecodes.c instead of opcode.py #105481

iritkatriel opened this issue Jun 7, 2023 · 9 comments · Fixed by #108367
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@iritkatriel
Copy link
Member

iritkatriel commented Jun 7, 2023

We would ideally have bytecodes.c as the single source of truth about opcodes. So opcode.py and the code generated from it should be replaced by alternatives from the cases_generator, if we can.

Linked PRs

@iritkatriel iritkatriel added the type-feature A feature request or enhancement label Jun 7, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Jun 7, 2023
@iritkatriel iritkatriel added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Jun 7, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Jun 8, 2023
iritkatriel added a commit that referenced this issue Jun 13, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Jun 14, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Jun 14, 2023
@iritkatriel
Copy link
Member Author

@gvanrossum The old macros HAS_ARG and HAS_CONST are exposed in Include/opcode.h (and have been for a long time). Does this mean that before we can remove them, we need to expose the opcode_metadata.h file there as well? We should also mark all of this as unstable API (how do we do that?)

carljm added a commit to carljm/cpython that referenced this issue Jun 15, 2023
* main: (57 commits)
  pythongh-105831: Fix NEWS blurb from pythongh-105828 (python#105833)
  pythongh-105820: Fix tok_mode expression buffer in file & readline tokenizer (python#105828)
  pythongh-105751, test_ctypes: Remove disabled tests (python#105826)
  pythongh-105821: Use a raw f-string in test_httpservers.py (python#105822)
  pythongh-105751: Remove platform usage in test_ctypes (python#105819)
  pythongh-105751: Reenable disable test_ctypes tests (python#105818)
  pythongh-105751: Remove dead code in test_ctypes (python#105817)
  More reorganisation of the typing docs (python#105787)
  Improve docs for `typing.dataclass_transform` (python#105792)
  pythonGH-89812: Churn `pathlib.Path` test methods (python#105807)
  pythongh-105800: Issue SyntaxWarning in f-strings for invalid escape sequences (python#105801)
  pythongh-105751: Cleanup test_ctypes imports (python#105803)
  pythongh-105481: add HAS_JUMP flag to opcode metadata (python#105791)
  pythongh-105751: test_ctypes avoids the operator module (pythonGH-105797)
  pythongh-105751: test_ctypes: Remove @need_symbol decorator (pythonGH-105798)
  pythongh-104909: Implement conditional stack effects for macros (python#105748)
  pythongh-75905: Remove test_xmlrpc_net: skipped since 2017 (python#105796)
  pythongh-105481: Fix types and a bug for pseudos (python#105788)
  Update DSL docs for cases generator (python#105753)
  pythonGH-77273: Better bytecodes for f-strings (pythonGH-6132)
  ...
@gvanrossum
Copy link
Member

gvanrossum commented Jun 16, 2023

Neither HAS_ARG nor HAS_CONST is mentioned in the docs at all. I am of the opinion that in this case this implies they are not public, and anyone using them should not be surprised if they disappear. (It would be more of an issue if they turned into lies.)

opcode.h is mentioned only once in the docs, as the source of truth for dis.py. (It is also mentioned twice in Misc/NEWS.d/, but that's really just a changelog.)

I do think we ought to provide similar functionality to debuggers.

Info about declaring unstable APIs comes from PEP 649, which links to https://devguide.python.org/developer-workflow/c-api/#c-api. It looks like you must use the PyUnstable_ prefix (e.g. PyUnstable_BytecodeHasArg), and it ought to be in a .h file under Include/cpython/.

@iritkatriel
Copy link
Member Author

I do think we ought to provide similar functionality to debuggers.

Do you mean to expose the stuff in opcode_metadata.h file through some unstable c api?

@gvanrossum
Copy link
Member

Yeah, to the extent that it’s useful, in function form (so the data structure is not public).

Or we can wait until someone asks.

@iritkatriel
Copy link
Member Author

There is a usage of HAS_ARG in Modules/_opcode.c. We could remove it (send oparg -1 if None was given and do this check inside Python/ code). Otherwise we need to move opcode_metadata.h from Python/ to Include/internal.

@gvanrossum
Copy link
Member

I would just set it to 0 when it's None or missing. Honestly it's always bothered me that you must know ahead of time whether an arg is needed or not.

iritkatriel added a commit that referenced this issue Jun 17, 2023
gvanrossum pushed a commit to gvanrossum/cpython that referenced this issue Jun 18, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Jun 20, 2023
gvanrossum pushed a commit to gvanrossum/cpython that referenced this issue Jun 22, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Aug 11, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Aug 14, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Aug 14, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Aug 16, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Aug 17, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Aug 17, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Aug 23, 2023
@vstinner
Copy link
Member

vstinner commented Nov 8, 2023

@encukou wrote "Unstable API needs documentation and tests."

I reopen this issue and close gh-107149.

@vstinner vstinner reopened this Nov 8, 2023
@iritkatriel
Copy link
Member Author

I'm not sure it was the right move to rename _PyUnstable_GetUnaryIntrinsicName to PyUnstable_GetUnaryIntrinsicName, etc.

I created them as private because I did not intend to document them as a public API (we need a reason to do that). I put Unstable in the name because it's one of those things that can change all the time. If we don't want the _PyUnstable_ prefix then let's remove the Unstable and leave the _ until we have a reason/decision to make these part of the public C API.

@iritkatriel
Copy link
Member Author

I'm moving this discussion back to #107149, and closing this, because this PR was about a large change, and #107149 is about the naming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants