GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-98831: rewrite GET_LEN, GET_ITER, BEFORE_WITH and a few simple opcodes in the instruction definition DSL by iritkatriel · Pull Request #101443 · python/cpython

Conversation

iritkatriel commented Jan 31, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Issue: Generate the interpreter #98831

Sorry, something went wrong.

Uh oh!

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

All reactions


          pythongh-98831: rewrite GET_LEN, GET_ITER, BEFORE_WITH and a few simp…

26fc7a3

…le opcodes in the instruction definition DSL

iritkatriel requested review from brandtbucher, gvanrossum and markshannon

January 31, 2023 00:40

bedevere-bot added the awaiting core review label

Jan 31, 2023

bedevere-bot mentioned this pull request

Jan 31, 2023

Generate the interpreter #98831

Closed

iritkatriel added skip news labels

Jan 31, 2023

gvanrossum reviewed

Jan 31, 2023

View reviewed changes

gvanrossum 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.

There might be leaks. :-(

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/bytecodes.c Outdated

Comment on lines +2328 to +2317

		goto error;
		ERROR_IF(true, error);

gvanrossum Jan 31, 2023

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.

I believe this leaks a reference to mgr. You can either insert another DECREF_INPUTS() before this line, or just keep the goto error. I prefer the latter, we're not going to require using DECREF_INPUTS() everywhere (it mostly exists because it would be useful with the register conversion).

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Jan 31, 2023

Copy link

Copy Markdown

Member Author

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.

I'm was just looking at this. I added DECREF_INPUTS in these two places and it is still leaking something.

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Jan 31, 2023

Copy link

Copy Markdown

Member Author

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.

I pushed the change with goto, still looking for the other leak.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/bytecodes.c Outdated

Comment on lines +2340 to +2329

		goto error;
		ERROR_IF(true, error);

gvanrossum Jan 31, 2023

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.

Same here.

Sorry, something went wrong.

Uh oh!

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

All reactions


          goto instead of ERROR_IF to avoid leaks

6bfec26

iritkatriel commented

Jan 31, 2023

View reviewed changes

Comment thread

Python/generated_cases.c.h Outdated

		goto error;
		}
		PUSH(res);
		if (res == NULL) goto pop_1_error;

iritkatriel Jan 31, 2023

Copy link

Copy Markdown

Member Author

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.

It's leaking when enter raises.

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Jan 31, 2023

Copy link

Copy Markdown

Member Author

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.

It leaks exit. Before this PR it put exit in the stack before the 'goto error', but now it doesn't so it needs to decref exit.

Sorry, something went wrong.

Uh oh!

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

All reactions


          revert BEFORE_WITH

2ee6241

iritkatriel changed the title ~~gh-98831: rewrite GET_LEN, GET_ITER, BEFORE_WITH and a few simple opcodes in the instruction definition DSL~~ gh-98831: rewrite GET_LEN, GET_ITER and a few simple opcodes in the instruction definition DSL

Jan 31, 2023

iritkatriel added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels

Jan 31, 2023

bedevere-bot commented Jan 31, 2023

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 2ee6241 🤖

If you want to schedule another build, you need to add the :hammer: test-with-buildbots label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

2 similar comments

bedevere-bot commented Jan 31, 2023

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 2ee6241 🤖

If you want to schedule another build, you need to add the :hammer: test-with-buildbots label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot commented Jan 31, 2023

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 2ee6241 🤖

If you want to schedule another build, you need to add the :hammer: test-with-buildbots label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels

Jan 31, 2023

iritkatriel added 2 commits

January 31, 2023 01:47


          Revert "revert BEFORE_WITH"

0a5f78d

This reverts commit 2ee6241.


          fix refleak of exit when __enter__ raises

a925498

iritkatriel changed the title ~~gh-98831: rewrite GET_LEN, GET_ITER and a few simple opcodes in the instruction definition DSL~~ gh-98831: rewrite GET_LEN, GET_ITER, BEFORE_WITH and a few simple opcodes in the instruction definition DSL

Jan 31, 2023

iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label

Jan 31, 2023

bedevere-bot commented Jan 31, 2023

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit a925498 🤖

If you want to schedule another build, you need to add the :hammer: test-with-buildbots label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label

Jan 31, 2023

gvanrossum approved these changes

Jan 31, 2023

View reviewed changes

gvanrossum 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.

That LGTM, but there's still something that claims to leak -- importlib, no less. Let me re-run that build.

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot added awaiting merge and removed awaiting core review labels

Jan 31, 2023

gvanrossum commented Jan 31, 2023

Copy link

Copy Markdown

Member

The PPC64 buildbots are simply out of disk space. (I think it may be the same machine.)

All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel merged commit 29a858b into python:main

Jan 31, 2023

bedevere-bot removed the awaiting merge label

Jan 31, 2023

mdboom pushed a commit to mdboom/cpython that referenced this pull request

Jan 31, 2023


          pythongh-98831: rewrite GET_LEN, GET_ITER, BEFORE_WITH and a few simp…

cc9952d

…le opcodes in the instruction definition DSL (python#101443)

carljm added a commit to carljm/cpython that referenced this pull request

Jan 31, 2023


          Merge branch 'main' into inlinecomp2

ed3209b

* main:
  pythongh-101440: fix json snippet error in logging-cookbook.rst (python#101439)
  pythongh-99276 - Updated Doc/faq/general.rst (python#101396)
  Add JOBS parameter to docs Makefile (python#101395)
  pythongh-98831: rewrite GET_LEN, GET_ITER, BEFORE_WITH and a few simple opcodes in the instruction definition DSL (python#101443)
  pythongh-77607: Improve accuracy of os.path.join docs (python#101406)
  Fixes typo in asyncio.TaskGroup context manager code example (python#101449)
  pythongh-98831: Clean up and add cache size static_assert to macro (python#101442)
  pythongh-99955: use SUCCESS/ERROR return values in optimizer and assembler. Use RETURN_IF_ERROR where appropriate. Fix a couple of bugs. (python#101412)

iritkatriel deleted the rewrite_opcodes branch

April 3, 2023 17:47

Reviewers

gvanrossum

gvanrossum approved these changes

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

brandtbucher

Awaiting requested review from brandtbucher

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

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.

GeezerVison

Conversation

iritkatriel commented Jan 31, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

iritkatriel Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

iritkatriel Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

gvanrossum Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

iritkatriel Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

iritkatriel Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 31, 2023

Uh oh!

bedevere-bot commented Jan 31, 2023

Uh oh!

bedevere-bot commented Jan 31, 2023

Uh oh!

bedevere-bot commented Jan 31, 2023

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Jan 31, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iritkatriel commented Jan 31, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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