Skip to content

gh-122712: Test CALL_ALLOC_AND_ENTER_INIT handles reassignment of __code__ #122713

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

Merged
merged 18 commits into from
Aug 22, 2024

Conversation

mpage
Copy link
Contributor

@mpage mpage 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
Contributor Author

mpage commented Aug 6, 2024

Failing hypothesis tests are a known issue: #122686

@mpage
Copy link
Contributor Author

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.

@mpage mpage marked this pull request as ready for review August 6, 2024 02:22
@mpage mpage requested review from brandtbucher and colesbury August 7, 2024 04:46
@mpage
Copy link
Contributor Author

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

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

bedevere-app bot commented Aug 15, 2024

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

@brandtbucher brandtbucher 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
Contributor Author

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

bedevere-app bot commented Aug 20, 2024

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

@mpage
Copy link
Contributor Author

mpage commented Aug 20, 2024

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_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

======================================================================
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
Contributor Author

mpage commented Aug 21, 2024

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

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

mpage commented Aug 21, 2024

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

@markshannon markshannon merged commit 79ddf75 into python:main Aug 22, 2024
32 checks passed
@markshannon
Copy link
Member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants