Skip to content
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

Fix instr_ptr branch for uops (?) #111144

Closed
wants to merge 80 commits into from
Closed

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Oct 20, 2023

Once ready this should be merged into gh-109095.

I'm unhappy with how this turned out; it's too complicated and unprincipled. I feel I don't know which invariants apply when to frame->instr_ptr and frame->next_instr_offset, and the clever exceptions in the code generator and the Tier 1-2 translator.

It feels too much like programming by random modifications.

If this passes all tests it should probably be merged into Irit's instr_ptr branch.

I'm unhappy with how this turned out; it's too complicated and
unprincipled. I feel I don't know which invariants apply when to
frame->instr_ptr and frame->next_instr_offset, and the clever
exceptions in the code generator and the Tier 1-2 translator.

It feels too much like programming by random modifications.
@gvanrossum
Copy link
Member Author

  • It's suspicious that the macOS tests pass in 9 minutes?
  • The "Check if generated files are up to date" failure is due to a comment I missed; will fix next.
  • The Ubuntu and Address Sanitizer failures are all in test_gdb subtests; those also fail on Irit's branch.
  • I want to run this again with the "uops-forever" patch added.

@gvanrossum
Copy link
Member Author

gvanrossum commented Oct 20, 2023

Hm, with "uops-forever" I get a bunch of failed tests. So I have to do some more debugging.


Running locally, these tests fail:

    test_argparse test_buffer test_datetime test_dbm_dumb test_descr
    test_http_cookiejar test_pickle test_plistlib test_shutil
    test_slice test_strtod test_symtable test_tarfile test_unittest
    test_zipfile

Interestingly, it seems the --fast-ci is partially responsible -- without this, at least some of those tests pass.


Small repro without --fast-ci:

~/cpython$ ./python.exe -m test -v test_descr -m test_pickle_slots
== CPython 3.13.0a0 (heads/instr_ptr:06a470d8500, Oct 20 2023, 17:00:01) [Clang 14.0.3 (clang-1403.0.22.14.1)]
== macOS-13.5.2-x86_64-i386-64bit little-endian
== Python build: debug
== cwd: /Users/guido/cpython/build/test_python_worker_34652æ
== CPU count: 12
== encodings: locale=UTF-8 FS=utf-8
== resources: all test resources are disabled, use -u option to unskip tests

Using random seed 2062641369
0:00:00 load avg: 2.06 Run 1 test sequentially
0:00:00 load avg: 2.06 [1/1] test_descr
test_pickle_slots (test.test_descr.PicklingTests.test_pickle_slots) ... Assertion failed: (STACK_LEVEL() <= STACK_SIZE()), function _PyUopExecute, file executor_cases.c.h, line 1217.
Fatal Python error: Aborted

Current thread 0x00007ff85b41c700 (most recent call first):
  File "/Users/guido/cpython/Lib/pickle.py", line 1206 in load
  File "/Users/guido/cpython/Lib/pickle.py", line 1767 in _loads
  File "/Users/guido/cpython/Lib/test/test_descr.py", line 5352 in copy
  File "/Users/guido/cpython/Lib/test/test_descr.py", line 5418 in test_pickle_slots
  File "/Users/guido/cpython/Lib/unittest/case.py", line 589 in _callTestMethod
  File "/Users/guido/cpython/Lib/unittest/case.py", line 636 in run
  File "/Users/guido/cpython/Lib/unittest/case.py", line 692 in __call__
  File "/Users/guido/cpython/Lib/unittest/suite.py", line 122 in run
  File "/Users/guido/cpython/Lib/unittest/suite.py", line 84 in __call__
  File "/Users/guido/cpython/Lib/unittest/suite.py", line 122 in run
  File "/Users/guido/cpython/Lib/unittest/suite.py", line 84 in __call__
  File "/Users/guido/cpython/Lib/unittest/suite.py", line 122 in run
  File "/Users/guido/cpython/Lib/unittest/suite.py", line 84 in __call__
  File "/Users/guido/cpython/Lib/unittest/runner.py", line 240 in run
  File "/Users/guido/cpython/Lib/test/support/__init__.py", line 1163 in _run_suite
  File "/Users/guido/cpython/Lib/test/support/__init__.py", line 1290 in run_unittest
  File "/Users/guido/cpython/Lib/test/libregrtest/single.py", line 36 in run_unittest
  File "/Users/guido/cpython/Lib/test/libregrtest/single.py", line 92 in test_func
  File "/Users/guido/cpython/Lib/test/libregrtest/single.py", line 48 in regrtest_runner
  File "/Users/guido/cpython/Lib/test/libregrtest/single.py", line 95 in _load_run_test
  File "/Users/guido/cpython/Lib/test/libregrtest/single.py", line 138 in _runtest_env_changed_exc
  File "/Users/guido/cpython/Lib/test/libregrtest/single.py", line 238 in _runtest
  File "/Users/guido/cpython/Lib/test/libregrtest/single.py", line 266 in run_single_test
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 298 in run_test
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 333 in run_tests_sequentially
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 466 in _run_tests
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 497 in run_tests
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 661 in main
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 669 in main
  File "/Users/guido/cpython/Lib/test/__main__.py", line 2 in <module>
  File "/Users/guido/cpython/Lib/runpy.py", line 88 in _run_code
  File "/Users/guido/cpython/Lib/runpy.py", line 198 in _run_module_as_main

Extension modules: _testcapi, xxsubtype (total: 2)
Abort trap: 6
~/cpython$ 

@gvanrossum
Copy link
Member Author

I think I have a winner. The last fix was setting frame->next_instr_offset = 0 in Tier 2's deoptimize block (just like in _EXIT_TRACE). I don't believe we need it in the error block, since errors don't care about the next instruction.

I'll run the tests with uops-forever, and if that passes, once @iritkatriel approves, I'll push this (without the uops-forever change, of course) to her instr_ptr branch.

But I have to repeat that I'm unhappy with the complexity of the machinery here. It feels very subtle when to set next_instr_offset to zero and when to a non-zero value. And e.g. while we're in a Tier 2 trace (executor), instr_ptr is occasionally set to the correct value (by _SET_IP) but next_instr_offset is only set to the correct value in a few places:

  • When exiting via _EXIT_TRACE or DEOPT_IF() (which set it to zero)
  • In _SAVE_CURRENT_IP (which sets it to point to the next instruction), just before a call

Is it okay for next_instr_offset to be out of sync at other times? Maybe it is -- after all it's okay for instr_ptr to be out of sync while certain Tier 2 instructions execute. But those are instructions that can't fail -- do we care that next_instr_offset is essentially a random value when an error occurs? It may be okay, since when an error occurs we only care about the current instruction.

Okay, I've at least talked myself into trying to also set it to zero in the error block. But I'm still queasy about the subtle complexity here.

@gvanrossum
Copy link
Member Author

See iritkatriel#55 instead.

@gvanrossum gvanrossum closed this Oct 21, 2023
@gvanrossum gvanrossum deleted the instr_ptr branch November 1, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants