Skip to content

Interrupt while internal frame is on the stack #14627

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.

Sign up for GitHub

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 13 commits into from
Sep 4, 2024
Merged

Interrupt while internal frame is on the stack #14627

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c5d3a9c
Check VM interrupt while internal frame is on top
morrisonlevi Jul 23, 2024
053090f
Use tab instead of spaces
morrisonlevi Jul 26, 2024
72369ce
fix frame used in interrupt and refactor
morrisonlevi Jul 26, 2024
c56ff0e
remove unused failures for zend_jit_check_timeout
morrisonlevi Jul 26, 2024
0b06725
Fix JIT support
morrisonlevi Jul 26, 2024
cf2bcd9
Fix the missing store to vm_interrupt
morrisonlevi Aug 1, 2024
ef26528
Rename new functions
morrisonlevi Aug 1, 2024
914c283
Special case zend_interrupt_function in JIT code
morrisonlevi Aug 1, 2024
c8d9d2c
Merge branch 'master' into internal_vm_interrupt
morrisonlevi Aug 2, 2024
69fff37
Merge branch 'master' into internal_vm_interrupt
morrisonlevi Aug 9, 2024
2ffca6d
Merge branch 'master' into internal_vm_interrupt
morrisonlevi Aug 14, 2024
1255c71
refactor to use ZEND_VM_SET_OPCODE_NO_INTERRUPT
morrisonlevi Aug 14, 2024
8fa433a
Split atomic exchange into load + store
morrisonlevi Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions UPGRADING.INTERNALS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,11 @@ PHP 8.4 INTERNALS UPGRADE NOTES
4. OpCode changes
========================

* DO_ICALL, DO_FCALL, and DO_FCALL_BY_NAME now call zend_interrupt_function
while the internal frame is still on the stack. This means interrupt handlers
will now see the internal call. If your interrupt handler does something like
switching EG(current_execute_data), it should not do so if an internal func
is on top.
* New FRAMELESS_ICALL_[0,3] opcodes for faster internal function calls have been
added. These opcodes don't create a stack frame, but pass arguments via opcode
operands. They only work for functions that are known at compile-time, and
Expand Down
11 changes: 6 additions & 5 deletions Zend/tests/fibers/signal-async.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pcntl_signal(SIGUSR1, function (): void {
$fiber = new Fiber(function (): void {
echo "Fiber start\n";
posix_kill(posix_getpid(), SIGUSR1);
time_nanosleep(1);
time_nanosleep(1, 0);
echo "Fiber end\n";
});

Expand All @@ -30,8 +30,9 @@ Fiber start
Fatal error: Uncaught FiberError: Cannot switch fibers in current execution context in %ssignal-async.php:%d
Stack trace:
#0 %ssignal-async.php(%d): Fiber::suspend()
#1 %ssignal-async.php(%d): {closure:%s:%d}(%d, Array)
#2 [internal function]: {closure:%s:%d}()
#3 %ssignal-async.php(%d): Fiber->start()
#4 {main}
#1 [internal function]: {closure:%s:%d}(%d, Array)
#2 %ssignal-async.php(%d): posix_kill(%d, %d)
#3 [internal function]: {closure:%s:%d}()
#4 %ssignal-async.php(%d): Fiber->start()
#5 {main}
thrown in %ssignal-async.php on line %d
15 changes: 15 additions & 0 deletions Zend/zend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,22 @@ extern ZEND_API size_t (*zend_printf)(const char *format, ...) ZEND_ATTRIBUTE_PT
extern ZEND_API zend_write_func_t zend_write;
extern ZEND_API FILE *(*zend_fopen)(zend_string *filename, zend_string **opened_path);
extern ZEND_API void (*zend_ticks_function)(int ticks);

/* Called by the VM in certain places like at the loop header, user function
* entry, and after internal function calls, if EG(vm_interrupt) has been set.
*
* If this is used to switch the EG(current_execute_data), such as implementing
* a coroutine scheduler, then it needs to check the top frame to see if it's
* an internal function. If an internal function is on top, then the frame
* shouldn't be switched away.
*
* Prior to PHP 8.0, this check was not necessary. In PHP 8.0,
* zend_call_function started calling zend_interrupt_function, and in 8.4 the
* DO_*CALL* opcodes started calling the zend_interrupt_function while the
* internal frame is still on top.
*/
extern ZEND_API void (*zend_interrupt_function)(zend_execute_data *execute_data);

extern ZEND_API void (*zend_error_cb)(int type, zend_string *error_filename, const uint32_t error_lineno, zend_string *message);
extern ZEND_API void (*zend_on_timeout)(int seconds);
extern ZEND_API zend_result (*zend_stream_open_function)(zend_file_handle *handle);
Expand Down
9 changes: 9 additions & 0 deletions Zend/zend_execute.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4056,6 +4056,15 @@ ZEND_API void ZEND_FASTCALL zend_free_compiled_variables(zend_execute_data *exec
}
/* }}} */

ZEND_API ZEND_COLD void ZEND_FASTCALL zend_interrupt_or_timeout(zend_execute_data *call)
{
if (zend_atomic_bool_load_ex(&EG(timed_out))) {
zend_timeout();
} else if (zend_interrupt_function) {
zend_interrupt_function(call);
}
}

#define ZEND_VM_INTERRUPT_CHECK() do { \
if (UNEXPECTED(zend_atomic_bool_load_ex(&EG(vm_interrupt)))) { \
ZEND_VM_INTERRUPT(); \
Expand Down
12 changes: 12 additions & 0 deletions Zend/zend_execute.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,18 @@ ZEND_COLD void zend_magic_get_property_type_inconsistency_error(const zend_prope

ZEND_COLD void zend_match_unhandled_error(const zval *value);

/* Call this to handle the timeout or the interrupt function. It will not clear
* the EG(vm_interrupt).
Copy link
Member

Choose a reason for hiding this comment

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

Do you rely on zend_interrupt_function to clear vm_interrupt in this case? Should we still reset vm_interrupt in case of timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed up a commit while you were reviewing ^_^
The store is done in the VM with atomic exchange which is better than a load + store individually. However in JIT, I don't have an IR instruction for atomic exchange, so there it does a load + store.

*/
ZEND_API ZEND_COLD void ZEND_FASTCALL zend_interrupt_or_timeout(zend_execute_data *call);

static zend_always_inline void zend_interrupt_or_timeout_check(zend_execute_data *call)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This function is similar to ZEND_VM_INTERRUPT_CHECK() and ZEND_VM_LOOP_INTERRUPT_CHECK(), but with subtle differences.

I think it would help understanding if this function was explicitly implemented as a variant of these two macros. E.g. it could be implemented as a macro named ZEND_VM_FCALL_INTERRUPT_CHECK().

{
if (UNEXPECTED(zend_atomic_bool_exchange_ex(&EG(vm_interrupt), false))) {
zend_interrupt_or_timeout(call);
}
}

static zend_always_inline void *zend_get_bad_ptr(void)
{
ZEND_UNREACHABLE();
Expand Down
12 changes: 9 additions & 3 deletions Zend/zend_vm_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4074,6 +4074,7 @@ ZEND_VM_HOT_HANDLER(129, ZEND_DO_ICALL, ANY, ANY, SPEC(RETVAL,OBSERVER))
}
#endif
ZEND_OBSERVER_FCALL_END(call, EG(exception) ? NULL : ret);
zend_interrupt_or_timeout_check(call);

EG(current_execute_data) = execute_data;
zend_vm_stack_free_args(call);
Expand All @@ -4097,7 +4098,8 @@ ZEND_VM_HOT_HANDLER(129, ZEND_DO_ICALL, ANY, ANY, SPEC(RETVAL,OBSERVER))
HANDLE_EXCEPTION();
}

ZEND_VM_SET_OPCODE(opline + 1);
CHECK_SYMBOL_TABLES()
OPLINE = opline + 1;
ZEND_VM_CONTINUE();
}

Expand Down Expand Up @@ -4195,6 +4197,7 @@ ZEND_VM_HOT_HANDLER(131, ZEND_DO_FCALL_BY_NAME, ANY, ANY, SPEC(RETVAL,OBSERVER))
}
#endif
ZEND_OBSERVER_FCALL_END(call, EG(exception) ? NULL : ret);
zend_interrupt_or_timeout_check(call);

EG(current_execute_data) = execute_data;

Expand Down Expand Up @@ -4225,7 +4228,8 @@ ZEND_VM_C_LABEL(fcall_by_name_end):
zend_rethrow_exception(execute_data);
HANDLE_EXCEPTION();
}
ZEND_VM_SET_OPCODE(opline + 1);
CHECK_SYMBOL_TABLES()
OPLINE = opline + 1;
ZEND_VM_CONTINUE();
}

Expand Down Expand Up @@ -4315,6 +4319,7 @@ ZEND_VM_HOT_HANDLER(60, ZEND_DO_FCALL, ANY, ANY, SPEC(RETVAL,OBSERVER))
}
#endif
ZEND_OBSERVER_FCALL_END(call, EG(exception) ? NULL : ret);
zend_interrupt_or_timeout_check(call);

EG(current_execute_data) = execute_data;

Expand Down Expand Up @@ -4344,7 +4349,8 @@ ZEND_VM_C_LABEL(fcall_end):
HANDLE_EXCEPTION();
}

ZEND_VM_SET_OPCODE(opline + 1);
CHECK_SYMBOL_TABLES()
OPLINE = opline + 1;
ZEND_VM_CONTINUE();
}

Expand Down
42 changes: 33 additions & 9 deletions Zend/zend_vm_execute.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 1 addition & 3 deletions ext/opcache/jit/zend_jit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1425,9 +1425,7 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
zend_jit_set_last_valid_opline(&ctx, op_array->opcodes + ssa->cfg.blocks[b].start);
}
if (ssa->cfg.blocks[b].flags & ZEND_BB_LOOP_HEADER) {
if (!zend_jit_check_timeout(&ctx, op_array->opcodes + ssa->cfg.blocks[b].start, NULL)) {
goto jit_failure;
}
zend_jit_check_timeout(&ctx, op_array->opcodes + ssa->cfg.blocks[b].start, NULL);
}
if (!ssa->cfg.blocks[b].len) {
zend_jit_bb_end(&ctx, b);
Expand Down
Loading
Loading