Skip to content

Conversation

@mpage
Copy link
Contributor

@mpagempage commented Aug 6, 2024

Add a test to verify CALL_ALLOC_AND_ENTER_INIT handles the case where the __init__ function's code object is reassigned.

@mpage
Copy link
ContributorAuthor

mpage commented Aug 6, 2024

Failing hypothesis tests are a known issue: #122686

@mpage
Copy link
ContributorAuthor

mpage commented Aug 6, 2024

JIT test failures look like unrelated issues with qemu:

ERROR:../../plugins/core.c:221:qemu_plugin_vcpu_init_hook: assertion failed: (success) Bail out! ERROR:../../plugins/core.c:221:qemu_plugin_vcpu_init_hook: assertion failed: (success) aarch64-binfmt-P: ./include/qemu/rcu.h:101: rcu_read_unlock: Assertion `p_rcu_reader->depth != 0' failed. 

@mpagempage marked this pull request as ready for review August 6, 2024 02:22
@mpage
Copy link
ContributorAuthor

mpage commented Aug 7, 2024

@brandtbucher@markshannon - I've updated this to perform the necessary checks inline in CALL_ALLOC_AND_ENTER_INIT rather than use function versions. Would you please have a look?

Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the correct fix. Thanks.

I'd like the DEOPT test streamlined a bit, but it looks good otherwise.

@bedevere-app
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Copy link
Member

@brandtbucherbrandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @markshannon's suggestion. Otherwise, looks good!

@mpage
Copy link
ContributorAuthor

mpage commented Aug 20, 2024

Looks like this was fixed on main by gh-123140. The underlying issue is still present in 3.13, however. @markshannon how would you like to proceed with fixing on 3.13?

@bedevere-app
Copy link

GH-123184 is a backport of this pull request to the 3.13 branch.

@mpage
Copy link
ContributorAuthor

Closing this since gh-123140 fixes it on main.

@markshannon
Copy link
Member

Sorry. I meant to merge this first, so we got the test before changing CALL_ALLOC_AND_ENTER_INIT. It appears I forgot.

Do you want to re-open this PR with just the test, or make a new one?

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot iOS ARM64 Simulator 3.13 has failed when building commit 50a595b.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1386/builds/496) and take a look at the build logs.
  4. Check if the failure is related to this commit (50a595b) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1386/builds/496

Failed tests:

  • test_import

Failed subtests:

  • test_basic_multiple_interpreters_deleted_no_reset - test.test_import.SinglephaseInitTests.test_basic_multiple_interpreters_deleted_no_reset

Summary of the results of the build (if available):

==

Click to see traceback logs
TracebackTests.test_exec_failure_nested) ... ok TracebackTests.test_nonexistent_module_nested) ... ok TracebackTests.test_syntax_error) ... ok TracebackTests.test_nonexistent_module) ... ok TracebackTests.test_unencodable_filename) ... skipped 'need TESTFN_UNENCODABLE' TracebackTests.test_broken_parent_from) ... ok TracebackTests.test_broken_from) ... ok TracebackTests.test_exec_failure) ... ok TracebackTests.test_broken_parent) ... ok TracebackTests.test_import_bug) ... ok TracebackTests.test_broken_submodule) ... ok Traceback (most recent call last): File "/Users/buildbot/Library/Developer/XCTestDevices/163B47BC-A1C1-4382-9772-4D79687530BB/data/Containers/Bundle/Application/FDBABAF6-E02D-45C5-A42C-5FCCA078C9B2/iOSTestbed.app/python/lib/python3.13/test/test_import/__init__.py", line 136, in wrapper func(self) ~~~~^^^^^^ File "/Users/buildbot/Library/Developer/XCTestDevices/163B47BC-A1C1-4382-9772-4D79687530BB/data/Containers/Bundle/Application/FDBABAF6-E02D-45C5-A42C-5FCCA078C9B2/iOSTestbed.app/python/lib/python3.13/test/test_import/__init__.py", line 3031, in test_basic_multiple_interpreters_deleted_no_resetself.check_copied(loaded_interp1, base) ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^ File "/Users/buildbot/Library/Developer/XCTestDevices/163B47BC-A1C1-4382-9772-4D79687530BB/data/Containers/Bundle/Application/FDBABAF6-E02D-45C5-A42C-5FCCA078C9B2/iOSTestbed.app/python/lib/python3.13/test/test_import/__init__.py", line 2694, in check_copiedself.assertNotEqual(snap.id, base.snapshot.id) ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^AssertionError: 4607884272 == 4607884272

@ericsnowcurrently
Copy link
Member

====================================================================== FAIL: test_basic_multiple_interpreters_deleted_no_reset (test.test_import.SinglephaseInitTests.test_basic_multiple_interpreters_deleted_no_reset) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/buildbot/Library/Developer/XCTestDevices/163B47BC-A1C1-4382-9772-4D79687530BB/data/Containers/Bundle/Application/FDBABAF6-E02D-45C5-A42C-5FCCA078C9B2/iOSTestbed.app/python/lib/python3.13/test/test_import/__init__.py", line 136, in wrapper func(self) ~~~~^^^^^^ File "/Users/buildbot/Library/Developer/XCTestDevices/163B47BC-A1C1-4382-9772-4D79687530BB/data/Containers/Bundle/Application/FDBABAF6-E02D-45C5-A42C-5FCCA078C9B2/iOSTestbed.app/python/lib/python3.13/test/test_import/__init__.py", line 3031, in test_basic_multiple_interpreters_deleted_no_reset self.check_copied(loaded_interp1, base) ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^ File "/Users/buildbot/Library/Developer/XCTestDevices/163B47BC-A1C1-4382-9772-4D79687530BB/data/Containers/Bundle/Application/FDBABAF6-E02D-45C5-A42C-5FCCA078C9B2/iOSTestbed.app/python/lib/python3.13/test/test_import/__init__.py", line 2694, in check_copied self.assertNotEqual(snap.id, base.snapshot.id) ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: 4607884272 == 4607884272 ---------------------------------------------------------------------- 

@ericsnowcurrently
Copy link
Member

Also, the failure is relative to the 3.13 backport: gh-123184.

@mpage
Copy link
ContributorAuthor

That failure seems like it should be unrelated to this change. I also haven't been able to reproduce it locally.

@mpagempage reopened this Aug 21, 2024
@mpagempage changed the title gh-122712: Guard against __code__ reassignment in CALL_ALLOC_AND_ENTER_INITgh-122712: Test CALL_ALLOC_AND_ENTER_INIT handles reassignment of __code__Aug 21, 2024
@mpage
Copy link
ContributorAuthor

@markshannon - No worries. Reopened this with just the test.

@markshannonmarkshannon merged commit 79ddf75 into python:mainAug 22, 2024
@markshannon
Copy link
Member

Thanks

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@mpage@markshannon@bedevere-bot@ericsnowcurrently@brandtbucher