gh-107265: Fix initialize/remove_tools for ENTER_EXECUTOR case by corona10 · Pull Request #108482 · python/cpython

corona10 · 2023-08-25T14:00:37Z

Issue: Decoding instructions should handle ENTER_EXECUTOR #107265

corona10 · 2023-08-25T14:01:12Z

@gvanrossum
See why this fix is needed: #107265 (comment)
But I am not sure how I can write test code for this case :(

gvanrossum

I don't know this code very well; let's see if @markshannon wants to review it.

gvanrossum

LG except formatting/naming nits.

gvanrossum

Green light! Thanks so much for powering through these.

bedevere-bot · 2023-08-27T04:34:06Z

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

Hi! The buildbot aarch64 Fedora Stable LTO 3.x has failed when building commit 6cb48f0.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/336/builds/3843) and take a look at the build logs.
Check if the failure is related to this commit (6cb48f0) or if it is a false positive.
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/all/#builders/336/builds/3843

Failed tests:

test.test_concurrent_futures.test_shutdown

Failed subtests:

test_interpreter_shutdown - test.test_concurrent_futures.test_shutdown.ProcessPoolForkserverProcessPoolShutdownTest.test_interpreter_shutdown

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

== Tests result: FAILURE then FAILURE ==

448 tests OK.

10 slowest tests:

test_gdb: 4 min 51 sec
test.test_multiprocessing_spawn.test_processes: 1 min 14 sec
test.test_concurrent_futures.test_wait: 1 min 11 sec
test_socket: 1 min 3 sec
test_signal: 1 min 2 sec
test.test_multiprocessing_forkserver.test_processes: 52.5 sec
test_subprocess: 50.8 sec
test_unparse: 48.2 sec
test_io: 43.5 sec
test.test_multiprocessing_spawn.test_misc: 42.6 sec

1 test failed:
test.test_concurrent_futures.test_shutdown

14 tests skipped:
test.test_asyncio.test_windows_events
test.test_asyncio.test_windows_utils test_devpoll test_ioctl
test_kqueue test_launcher test_startfile test_tkinter test_ttk
test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test.test_concurrent_futures.test_shutdown

Total duration: 4 min 57 sec

Click to see traceback logs

Traceback (most recent call last):
File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/test/test_concurrent_futures/test_shutdown.py", line 49, in test_interpreter_shutdownself.assertFalse(err)
AssertionError: b'Exception in thread Thread-1:\nTraceback (most recent call last):\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 1059, in _bootstrap_inner\n self.run()\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/concurrent/futures/process.py", line 339, in run\n self.add_call_item_to_queue()\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/concurrent/futures/process.py", line 394, in add_call_item_to_queue\n self.call_queue.put(_CallItem(work_id,\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/queues.py", line 94, in put\n self._start_thread()\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/queues.py", line 177, in _start_thread\n self._thread.start()\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 978, in start\n _start_new_thread(self._bootstrap, ())\nRuntimeError: can\'t create new thread at interpreter shutdown\nTraceback (most recent call last):\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/forkserver.py", line 274, in main\n code = _serve_one(child_r, fds,\n ^^^^^^^^^^^^^^^^^^^^^^^^\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/forkserver.py", line 313, in _serve_one\n code = spawn._main(child_r, parent_sentinel)\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/spawn.py", line 132, in _main\n self = reduction.pickle.load(from_parent)\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/synchronize.py", line 115, in __setstate__\n self._semlock = _multiprocessing.SemLock._rebuild(*state)\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\nFileNotFoundError: [Errno 2] No such file or directory\n' is not false

markshannon

All the removed opcode != ENTER_EXECUTOR assertions were correct.
There should never be instrumented ENTER_EXECUTOR instructions.

I think the correct fix is to remove all ENTER_EXECUTOR instructions in _Py_Instrument() prior to calling update_instrumentation_data(), some thing like:

if (code->co_executors->size > 0) {
// Walk code removing ENTER_EXECUTOR
// Clear all executors
}

markshannon · 2023-08-27T09:33:37Z

 _Py_CODEUNIT *instr = &_PyCode_CODE(code)[i];
 uint8_t *opcode_ptr = &instr->op.code;
 int opcode = *opcode_ptr;
- assert(opcode != ENTER_EXECUTOR);


This assertion is correct. Please don't remove assertions, unless you are really sure that they are incorrect.
ENTER_EXECUTOR should never have associated instrumentation.

It was moved to L574, it will be the same effect no?

markshannon · 2023-08-27T09:37:32Z

 assert(event != PY_MONITORING_EVENT_LINE);
 assert(event != PY_MONITORING_EVENT_INSTRUCTION);
 assert(PY_MONITORING_IS_INSTRUMENTED_EVENT(event));
- assert(opcode_has_event(_Py_GetBaseOpcode(code, offset)));


This is also correct, provided the assertion assert(PY_MONITORING_IS_INSTRUMENTED_EVENT(event)) is true.

Hmm, it will guarantee that the opcode is not ENTER_EXECUTOR?

markshannon · 2023-08-27T09:39:46Z

+ else if (opcode == INSTRUMENTED_LINE) {
 opcode = code->_co_monitoring->lines[i].original_opcode;
 }
+ assert(opcode != ENTER_EXECUTOR);


This assert should be moved before the original if (opcode == INSTRUMENTED_LINE) {, we shouldn't get here with and ENTER_EXECUTOR present.

Move it to L1310 will be enough?

markshannon · 2023-08-27T09:50:02Z

Note that it is OK to have instrumented instructions and ENTER_EXECUTOR in the same code objects, but only if ENTER_EXECUTOR is inserted after the instrumentation happened, and ENTER_EXECUTOR never replaces an instrumented instruction.

corona10 · 2023-08-27T10:08:16Z

I think the correct fix is to remove all ENTER_EXECUTOR instructions in _Py_Instrument() prior to calling update_instrumentation_data(), some thing like:

I will try to apply this approach :)

…OR case (pythongh-108482)" This reverts commit 6cb48f0.

pythongh-107265: Fix initialize_tools for ENTER_EXECUTOR case
4d48313

corona10 added the skip news label Aug 25, 2023

corona10 requested a review from gvanrossum August 25, 2023 14:00

bedevere-bot mentioned this pull request Aug 25, 2023
Decoding instructions should handle ENTER_EXECUTOR #107265
Closed
10 tasks

bedevere-bot added the awaiting core review label Aug 25, 2023

corona10 changed the title ~~gh-107265: Fix initialize_tools for ENTER_EXECUTOR case~~[WIP] gh-107265: Fix initialize_tools for ENTER_EXECUTOR caseAug 25, 2023

Update
e7ebb62

corona10 changed the title ~~[WIP] gh-107265: Fix initialize_tools for ENTER_EXECUTOR case~~gh-107265: Fix initialize_tools for ENTER_EXECUTOR caseAug 25, 2023

corona10 changed the title ~~gh-107265: Fix initialize_tools for ENTER_EXECUTOR case~~gh-107265: Fix initialize/remove_tools for ENTER_EXECUTOR caseAug 25, 2023

nit
61aadfd

gvanrossum reviewed Aug 25, 2023
View reviewed changes

Comment threadPython/instrumentation.c Outdated
Comment threadPython/instrumentation.c Outdated
Comment threadPython/instrumentation.c Outdated

gvanrossum requested a review from markshannon August 25, 2023 16:36

corona10 added 2 commits August 26, 2023 10:39

Address code review
93f43c1

fix
4b17bb9

gvanrossum approved these changes Aug 26, 2023
View reviewed changes

Comment threadPython/instrumentation.c
Comment threadPython/instrumentation.c Outdated

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 26, 2023

Address code review
46d879d

corona10 requested a review from gvanrossum August 26, 2023 07:07

gvanrossum approved these changes Aug 26, 2023
View reviewed changes

corona10 merged commit 6cb48f0 into python:mainAug 27, 2023

bedevere-bot removed the awaiting merge label Aug 27, 2023

corona10 deleted the gh-107265-initialize_tools branch August 27, 2023 03:31

markshannon reviewed Aug 27, 2023
View reviewed changes

corona10 added a commit to corona10/cpython that referenced this pull request Aug 27, 2023
Revert "pythongh-107265: Fix initialize/remove_tools for ENTER_EXECUT…
23e1f48
…OR case (pythongh-108482)" This reverts commit 6cb48f0.

corona10 mentioned this pull request Aug 27, 2023
gh-107265: Remove all ENTER_EXECUTOR when execute _Py_Instrument #108539
Merged

Uh oh!

Uh oh!

Conversation

corona10 commented Aug 25, 2023• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Aug 25, 2023

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 27, 2023

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

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

markshannonAug 27, 2023

Choose a reason for hiding this comment

Uh oh!

corona10Aug 27, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannonAug 27, 2023

Choose a reason for hiding this comment

Uh oh!

corona10Aug 27, 2023

Choose a reason for hiding this comment

Uh oh!

markshannonAug 27, 2023

Choose a reason for hiding this comment

Uh oh!

corona10Aug 27, 2023

Choose a reason for hiding this comment

Uh oh!

markshannon commented Aug 27, 2023

Uh oh!

corona10 commented Aug 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

corona10 commented Aug 25, 2023•
edited by bedevere-bot
Loading

corona10Aug 27, 2023•
edited
Loading