GeezerVison

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

GeezerVison is showing github.com — GH-93252: Fix error handling for failed Python calls by brandtbucher · Pull Request #94693 · python/cpython

Conversation

brandtbucher commented Jul 8, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Also, add an assert so that things like this fail much earlier (and more clearly) on debug builds.

Issue: Python/pystate.c:2218: _PyThreadState_PopFrame: Assertion `tstate->datastack_top >= base' failed. #93252

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher added 3 commits

July 8, 2022 11:40


          Add failing regression test

d0b41d1


          Always pop the new frame after a failed call

e21751f


          blurb add

eb6f8c4

brandtbucher added interpreter-core

(Objects, Python, Grammar, and Parser dirs)

type-crash

A hard crash of the interpreter, possibly with a core dump

needs backport to 3.11 only security fixes labels

Jul 8, 2022

brandtbucher requested review from kumaraditya303 and pablogsal

July 8, 2022 18:53

brandtbucher self-assigned this

Jul 8, 2022

brandtbucher requested a review from markshannon as a code owner

July 8, 2022 18:53

bedevere-bot added the awaiting core review label

Jul 8, 2022

brandtbucher commented

Jul 8, 2022

View reviewed changes

Comment thread

Python/ceval.c Outdated

Comment on lines +6435 to +6438

		PyObject base = (PyObject )frame;
		// Make sure that this is, indeed, the top frame. We can't check this in
		// _PyThreadState_PopFrame, since f_code is already cleared at that point:
		assert(base + frame->f_code->co_framesize == tstate->datastack_top);

brandtbucher Jul 8, 2022

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.

@pablogsal, just curious: does adding this assert (and not the fix) make tons of tests crash on your build?

If so, my theory is that your compiler is optimizing out the old assert, since the failing case is always undefined behavior according to the C standard.

Sorry, something went wrong.

Uh oh!

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

All reactions

pablogsal Jul 8, 2022

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 will test this night or tomorrow, but I checked and I was indeed compiling in debug mode with asserts, so the old assert should have triggered. I'm curious to see what's going on so I will also investigate that, but that won't affect this issue or the PR, is just that I'm very curious to see what's going on there :)

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher Jul 8, 2022

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.

Well, even if asserts were turned on, the inequality comparison between the pointers would be undefined if they're part of different allocations. Which is exactly the situation it was checking for!

So if the compiler could somehow prove that the comparison it was always true when the result was defined, it could have optimized it into assert(1) or something.

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher Jul 8, 2022

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.

But yeah, it doesn't affect this PR. The thing that's better about this new assert is that it's never undefined, and we don't need to overflow our stack chunk to trigger it. One failed call should do it (which is why so many tests crash if you add this without the fix).

Sorry, something went wrong.

Uh oh!

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

All reactions

pablogsal Jul 8, 2022

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.

So if the compiler could somehow prove that the comparison it was always true when the result was defined, it could have optimized it into assert(1) or something.

I don't think so because I was compiling with -O0. If the compiler was being smart there I want my money back 🤣

Sorry, something went wrong.

Uh oh!

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

All reactions

pablogsal approved these changes

Jul 8, 2022

View reviewed changes

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

LGTM

Great work 👌

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_call.py

		self.assertIsInstance(res, dict)
		self.assertEqual(list(res.items()), expected)

		def test_frames_are_popped_after_failed_calls(self):

pablogsal Jul 8, 2022

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.

Not that it matters much, but maybe you want to add the CPython specific decorator

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher Jul 8, 2022

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 thought about that, but I figure that this should still pass on any CPython implementation. If you somehow crash here, it's a bug, right? 😉

Sorry, something went wrong.

Uh oh!

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

All reactions

pablogsal Jul 8, 2022

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.

Yeah, we have been approaching these kind of tests in different ways so it doesn't matter :)

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

Jul 8, 2022

pablogsal reviewed

Jul 8, 2022

View reviewed changes

Comment thread

Python/ceval.c Resolved

Uh oh!

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


          Fix compiler warning

5b46418

The-Compiler mentioned this pull request

Jul 8, 2022

Python/pystate.c:2218: _PyThreadState_PopFrame: Assertion `tstate->datastack_top >= base' failed. #93252

Closed

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

Jul 8, 2022

bedevere-bot commented Jul 8, 2022

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit 5b46418 🤖

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

Jul 8, 2022

brandtbucher commented Jul 8, 2022 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member Author

Not sure why some of the wasm32 buildbots are failing, but it looks totally unrelated to these changes:

Warnings:

../../Python/initconfig.c:2284:27: warning: format specifies type 'wint_t' (aka 'int') but the argument has type 'wint_t' (aka 'unsigned int') [-Wformat]
../../Python/initconfig.c:2284:42: warning: format specifies type 'wint_t' (aka 'int') but the argument has type 'wint_t' (aka 'unsigned int') [-Wformat]
../../Python/pytime.c:297:10: warning: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion]
../../Python/pytime.c:352:14: warning: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion]
../../Python/pytime.c:518:10: warning: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion]
../../Modules/expat/xmlparse.c:3107:9: warning: code will never be executed [-Wunreachable-code]
../../Modules/expat/xmlparse.c:4050:9: warning: code will never be executed [-Wunreachable-code]
../../Modules/expat/xmlparse.c:7681:11: warning: format specifies type 'int' but the argument has type 'ptrdiff_t' (aka 'long') [-Wformat]
../../Modules/socketmodule.c:4001:33: warning: comparison of integers of different signs: 'unsigned long' and 'long' [-Wsign-compare]
../../Modules/socketmodule.c:4054:33: warning: comparison of integers of different signs: 'unsigned long' and 'long' [-Wsign-compare]
../../Modules/socketmodule.c:4678:54: warning: comparison of integers of different signs: 'unsigned long' and 'long' [-Wsign-compare]

Error:

error: unable to open output file 'Modules/_testcapi/vectorcall.o': 'No such file or directory'

Maybe related to #93649?

All reactions

Sorry, something went wrong.

Uh oh!

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

brandtbucher commented Jul 9, 2022

Copy link

Copy Markdown

Member Author

Maybe related to #93649?

Ah, yes, it is: #94549 (comment)

All reactions

Sorry, something went wrong.

Uh oh!

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

brandtbucher merged commit 8a285df into python:main

Jul 9, 2022

miss-islington commented Jul 9, 2022

Copy link

Copy Markdown

Contributor

Thanks @brandtbucher for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the awaiting merge label

Jul 9, 2022

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jul 9, 2022


          pythonGH-93252: Fix error handling for failed Python calls (pythonGH-…

23d96a6

…94693)

(cherry picked from commit 8a285df)

Co-authored-by: Brandt Bucher <[email protected]>

bedevere-bot commented Jul 9, 2022

Copy link

Copy Markdown

GH-94699 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the needs backport to 3.11 only security fixes label

Jul 9, 2022

kumaraditya303 reviewed

Jul 9, 2022

View reviewed changes

kumaraditya303 left a comment

Copy link

Copy Markdown

Contributor

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.

Post commit LGTM.

Sorry, something went wrong.

Uh oh!

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

All reactions

tiran commented Jul 9, 2022

Copy link

Copy Markdown

Member

Not sure why some of the wasm32 buildbots are failing, but it looks totally unrelated to these changes:

FYI, I have fixed the WASM buildbot issues in #94695. Once dependency was wrong and a directory was missing for OOT builds.

All reactions

Sorry, something went wrong.

Uh oh!

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

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

Jul 9, 2022


          [3.11] pythonGH-93252: Fix error handling for failed Python calls (py…

305cfe8

…thonGH-94693)

(cherry picked from commit 8a285df)

Co-authored-by: Brandt Bucher <[email protected]>

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

Jul 9, 2022


          pythonGH-93252: Fix error handling for failed Python calls (pythonGH-…

be41a14

…94693)

miss-islington pushed a commit that referenced this pull request

Jul 9, 2022


          [3.11] GH-93252: Fix error handling for failed Python calls (GH-94693) (

7a34172

GH-94708)

Automerge-Triggered-By: GH:tiran

brandtbucher deleted the failed-call-cleanup branch

July 21, 2022 19:52

Reviewers

pablogsal

pablogsal approved these changes

kumaraditya303

kumaraditya303 left review comments

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

Uh oh!

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

Assignees

brandtbucher

Labels

interpreter-core

(Objects, Python, Grammar, and Parser dirs)

type-crash

A hard crash of the interpreter, possibly with a core dump

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.

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

Conversation

brandtbucher commented Jul 8, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

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!

pablogsal left a comment

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!

bedevere-bot commented Jul 8, 2022

Uh oh!

brandtbucher commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandtbucher commented Jul 9, 2022

Uh oh!

miss-islington commented Jul 9, 2022

Uh oh!

bedevere-bot commented Jul 9, 2022

Uh oh!

kumaraditya303 left a comment

Choose a reason for hiding this comment

Uh oh!

tiran commented Jul 9, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

brandtbucher commented Jul 8, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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

brandtbucher commented Jul 8, 2022 •

edited

Loading

Uh oh!

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