GH-93252: Fix error handling for failed Python calls by brandtbucher · Pull Request #94693 · python/cpython · GitHub

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

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

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 :)

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.

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

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 🤣

LGTM

Great work 👌

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

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? 😉

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

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

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?

Maybe related to #93649?

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

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

…94693) (cherry picked from commit 8a285df) Co-authored-by: Brandt Bucher <brandtbucher@microsoft.com>

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

Post commit LGTM.

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.

…thonGH-94693) (cherry picked from commit 8a285df) Co-authored-by: Brandt Bucher <brandtbucher@microsoft.com>

…94693)

GH-94708) Automerge-Triggered-By: GH:tiran

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

pablogsal approved these changes Jul 8, 2022

View reviewed changes

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 Outdated

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 removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 8, 2022

brandtbucher merged commit 8a285df into python:main Jul 9, 2022

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 <brandtbucher@microsoft.com>

bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 9, 2022

kumaraditya303 reviewed Jul 9, 2022

View reviewed changes

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 <brandtbucher@microsoft.com>

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

+                  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);

                       self.assertIsInstance(res, dict)
                       self.assertEqual(list(res.items()), expected)
+                  def test_frames_are_popped_after_failed_calls(self):

GH-93252: Fix error handling for failed Python calls by brandtbucher · Pull Request #94693 · python/cpython · GitHub

Uh oh!

Uh oh!

GH-93252: Fix error handling for failed Python calls - #94693

GH-93252: Fix error handling for failed Python calls#94693brandtbucher merged 4 commits intopython:mainpython/cpython:mainfrom brandtbucher:failed-call-cleanupbrandtbucher/cpython:failed-call-cleanupCopy head branch name to clipboard

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

GH-93252: Fix error handling for failed Python calls#94693
brandtbucher merged 4 commits into
python:mainfrom
brandtbucher:failed-call-cleanup

brandtbucher commented Jul 8, 2022 •
edited by bedevere-bot
Loading

brandtbucher commented Jul 8, 2022 •
edited
Loading