Skip to content

Add JIT guards for INIT_METHOD_CALL when the method may be modified #8600

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 1 commit into from
May 27, 2022
Merged

Add JIT guards for INIT_METHOD_CALL when the method may be modified #8600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions ext/opcache/jit/zend_jit_arm64.dasc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8998,15 +8998,15 @@ static int zend_jit_init_method_call(dasm_State **Dst,
|2:
}

if (!func
if ((!func || zend_jit_may_be_modified(func, op_array))
Copy link
Member Author

@arnaud-lb arnaud-lb May 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

func here comes from static analysis / zend optimizer. In my understanding a non-null func here is an indication that the call is non polymorphic, since the function could be resolved statically. A guard was not added in this case, with the assumption that func could not change. This assumption doesn't hold true across multiple requests.

&& trace
&& trace->op == ZEND_JIT_TRACE_INIT_CALL
&& trace->func
) {
int32_t exit_point;
const void *exit_addr;

exit_point = zend_jit_trace_get_exit_point(opline, ZEND_JIT_EXIT_METHOD_CALL);
exit_point = zend_jit_trace_get_exit_point(opline, func ? ZEND_JIT_EXIT_INVALIDATE : ZEND_JIT_EXIT_METHOD_CALL);
Copy link
Member Author

Choose a reason for hiding this comment

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

In my understanding, ZEND_JIT_EXIT_METHOD_CALL is designed for polymorphic calls. It fallbacks to the VM, but the next loop iterations will execute this trace again for a while before the jit engine decides to generate a new/side trace.

ZEND_JIT_EXIT_INVALIDATE invalidates the trace immediately.

func being non-null here indicates that this is not a polymorphic call

Copy link
Member

Choose a reason for hiding this comment

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

For non-polimorphic calls function guard might be checked once per request, so it would be better to do this right after zend_jit_find_method_helper(). I may optimize this on top of the PR.

exit_addr = zend_jit_trace_get_exit_addr(exit_point);
if (!exit_addr) {
return 0;
Expand Down
20 changes: 20 additions & 0 deletions ext/opcache/jit/zend_jit_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,26 @@ static zend_always_inline const zend_op* zend_jit_trace_get_exit_opline(zend_jit
return NULL;
}

static inline bool zend_jit_may_be_modified(const zend_function *func, const zend_op_array *called_from)
{
if (func->type == ZEND_INTERNAL_FUNCTION) {
#ifdef _WIN32
/* ASLR */
return 1;
#else
return 0;
#endif
} else if (func->type == ZEND_USER_FUNCTION) {
if (func->common.fn_flags & ZEND_ACC_PRELOADED) {
return 0;
}
if (func->op_array.filename == called_from->filename && !func->op_array.scope) {
return 0;
}
}
return 1;
}

static zend_always_inline bool zend_jit_may_be_polymorphic_call(const zend_op *opline)
{
if (opline->opcode == ZEND_INIT_FCALL
Expand Down
20 changes: 0 additions & 20 deletions ext/opcache/jit/zend_jit_trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,26 +366,6 @@ static int zend_jit_trace_may_exit(const zend_op_array *op_array, const zend_op
return 0;
}

static bool zend_jit_may_be_modified(const zend_function *func, const zend_op_array *called_from)
{
if (func->type == ZEND_INTERNAL_FUNCTION) {
#ifdef _WIN32
/* ASLR */
return 1;
#else
return 0;
#endif
} else if (func->type == ZEND_USER_FUNCTION) {
if (func->common.fn_flags & ZEND_ACC_PRELOADED) {
return 0;
}
if (func->op_array.filename == called_from->filename && !func->op_array.scope) {
return 0;
}
}
return 1;
}

static zend_always_inline uint32_t zend_jit_trace_type_to_info_ex(zend_uchar type, uint32_t info)
{
if (type == IS_UNKNOWN) {
Expand Down
4 changes: 2 additions & 2 deletions ext/opcache/jit/zend_jit_x86.dasc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9630,7 +9630,7 @@ static int zend_jit_init_method_call(dasm_State **Dst,
|2:
}

if (!func
if ((!func || zend_jit_may_be_modified(func, op_array))
&& trace
&& trace->op == ZEND_JIT_TRACE_INIT_CALL
&& trace->func
Expand All @@ -9641,7 +9641,7 @@ static int zend_jit_init_method_call(dasm_State **Dst,
int32_t exit_point;
const void *exit_addr;

exit_point = zend_jit_trace_get_exit_point(opline, ZEND_JIT_EXIT_METHOD_CALL);
exit_point = zend_jit_trace_get_exit_point(opline, func ? ZEND_JIT_EXIT_INVALIDATE : ZEND_JIT_EXIT_METHOD_CALL);
exit_addr = zend_jit_trace_get_exit_addr(exit_point);
if (!exit_addr) {
return 0;
Expand Down
7 changes: 7 additions & 0 deletions ext/opcache/tests/jit/gh8591-001.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

declare(strict_types=1);

interface ModelInterface
{
}
49 changes: 49 additions & 0 deletions ext/opcache/tests/jit/gh8591-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
--TEST--
Bug GH-8591 001 (JIT does not account for class re-compile)
--EXTENSIONS--
opcache
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit_buffer_size=1M
opcache.jit=1255
opcache.file_update_protection=0
opcache.revalidate_freq=0
opcache.protect_memory=1
--FILE--
<?php

// Checks that JITed code does not crash in --repeat 2 after the ModelInterface
// interface is recompiled and Model is re-linked.

require __DIR__ . '/gh8591-001.inc';

class Model implements ModelInterface
{
protected static int $field = 1;

public function __construct()
{
for ($i = 0; $i < 10; $i++) {
$this->cast();
}
}

private function cast()
{
global $x;
$x = static::$field;
}
}

new Model();

// mark the file as changed (important)
touch(__DIR__ . '/gh8591-001.inc');

var_dump($x);

print "OK";
--EXPECT--
int(1)
OK
7 changes: 7 additions & 0 deletions ext/opcache/tests/jit/gh8591-002.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

declare(strict_types=1);

interface ModelInterface
{
}
52 changes: 52 additions & 0 deletions ext/opcache/tests/jit/gh8591-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
--TEST--
Bug GH-8591 002 (JIT does not account for class re-compile)
--EXTENSIONS--
opcache
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit_buffer_size=1M
opcache.jit=1255
opcache.file_update_protection=0
opcache.revalidate_freq=0
opcache.protect_memory=1
--FILE--
<?php

// Checks that JITed code does not crash in --repeat 2 after the ModelInterface
// interface changes and Model is re-linked.

if (!isset(opcache_get_status()['scripts'][__DIR__ . '/gh8591-002.inc'])) {
require __DIR__ . '/gh8591-001.inc';
} else {
interface ModelInterace
{
}
}

class Model implements ModelInterface
{
protected static int $field = 1;

public function __construct()
{
for ($i = 0; $i < 10; $i++) {
$this->cast();
}
}

private function cast()
{
global $x;
$x = static::$field;
}
}

new Model();

var_dump($x);

print "OK";
--EXPECT--
int(1)
OK
45 changes: 45 additions & 0 deletions ext/opcache/tests/jit/gh8591-003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
--TEST--
Bug GH-8591 003 (JIT does not account for class re-compile)
--EXTENSIONS--
opcache
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit_buffer_size=1M
opcache.jit=1255
opcache.file_update_protection=0
opcache.revalidate_freq=0
opcache.protect_memory=1
--FILE--
<?php

interface ModelInterface
{
}

class Model implements ModelInterface
{
protected static int $field = 1;

public function __construct()
{
for ($i = 0; $i < 10; $i++) {
$this->cast();
}
}

private function cast()
{
global $x;
$x = static::$field;
}
}

new Model();

var_dump($x);

print "OK";
--EXPECT--
int(1)
OK
7 changes: 7 additions & 0 deletions ext/opcache/tests/jit/gh8591-004.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

declare(strict_types=1);

trait ModelTrait
{
}
51 changes: 51 additions & 0 deletions ext/opcache/tests/jit/gh8591-004.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
--TEST--
Bug GH-8591 004 (JIT does not account for class re-compile)
--EXTENSIONS--
opcache
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit_buffer_size=1M
opcache.jit=1255
opcache.file_update_protection=0
opcache.revalidate_freq=0
opcache.protect_memory=1
--FILE--
<?php

// Checks that JITed code does not crash in --repeat 2 after the ModelTrait
// trait is recompiled and Model is re-linked.

require __DIR__ . '/gh8591-004.inc';

class Model
{
use ModelTrait;

protected static int $field = 1;

public function __construct()
{
for ($i = 0; $i < 10; $i++) {
$this->cast();
}
}

private function cast()
{
global $x;
$x = static::$field;
}
}

new Model();

// mark the file as changed (important)
touch(__DIR__ . '/gh8591-004.inc');

var_dump($x);

print "OK";
--EXPECT--
int(1)
OK
12 changes: 12 additions & 0 deletions ext/opcache/tests/jit/gh8591-005.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

class AbstractModel
{
protected static int $field = 1;

final protected function cast()
{
global $x;
$x = static::$field;
}
}
41 changes: 41 additions & 0 deletions ext/opcache/tests/jit/gh8591-005.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
--TEST--
Bug GH-8591 001 (JIT does not account for class re-compile)
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like a typo in test name

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, thank you. I will update it.

--EXTENSIONS--
opcache
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit_buffer_size=1M
opcache.jit=1255
opcache.file_update_protection=0
opcache.revalidate_freq=0
opcache.protect_memory=1
--FILE--
<?php

// Checks that JITed code does not crash in --repeat 2 after the AbstractModel
// class is recompiled and Model is re-linked.

require __DIR__ . '/gh8591-005.inc';

class Model extends AbstractModel
{
public function __construct()
{
for ($i = 0; $i < 10; $i++) {
$this->cast();
}
}
}

new Model();

// mark the file as changed (important)
touch(__DIR__ . '/gh8591-005.inc');

var_dump($x);

print "OK";
--EXPECT--
int(1)
OK
12 changes: 12 additions & 0 deletions ext/opcache/tests/jit/gh8591-006.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

class AbstractModel
{
protected static int $field = 1;

final protected function cast()
{
global $x;
$x = static::$field;
}
}
Loading