Skip to content

JIT: add missing endbr #10605

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

Closed
wants to merge 1 commit into from
Closed

JIT: add missing endbr #10605

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
19 changes: 14 additions & 5 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 @@ -1640,8 +1640,14 @@ static size_t tsrm_tls_offset;

#if defined (__CET__) && (__CET__ & 1) != 0
# define ENDBR_PADDING 4
/* Keep 16 exit points in a single code block */
# define ZEND_JIT_EXIT_POINTS_SPACING 8 // endbr + push byte + short jmp = bytes
# define ZEND_JIT_EXIT_POINTS_PER_GROUP 16 // number of continuous exit points
#else
# define ENDBR_PADDING 0
/* Keep 32 exit points in a single code block */
# define ZEND_JIT_EXIT_POINTS_SPACING 4 // push byte + short jmp = bytes
# define ZEND_JIT_EXIT_POINTS_PER_GROUP 32 // number of continuous exit points
#endif

static bool reuse_ip = 0;
Expand Down Expand Up @@ -2605,18 +2611,16 @@ static int zend_jit_trace_escape_stub(dasm_State **Dst)
return 1;
}

/* Keep 32 exit points in a single code block */
#define ZEND_JIT_EXIT_POINTS_SPACING 4 // push byte + short jmp = bytes
#define ZEND_JIT_EXIT_POINTS_PER_GROUP 32 // number of continuous exit points

static int zend_jit_trace_exit_group_stub(dasm_State **Dst, uint32_t n)
{
uint32_t i;

for (i = 0; i < ZEND_JIT_EXIT_POINTS_PER_GROUP - 1; i++) {
| ENDBR
| push byte i
| .byte 0xeb, (4*(ZEND_JIT_EXIT_POINTS_PER_GROUP-i)-6) // jmp >1
| .byte 0xeb, (ZEND_JIT_EXIT_POINTS_SPACING*(ZEND_JIT_EXIT_POINTS_PER_GROUP-i)-ZEND_JIT_EXIT_POINTS_SPACING -2) // jmp >1
}
| ENDBR
Copy link
Member

Choose a reason for hiding this comment

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

I suppose, this ENDBR is used here only to support indirect branches generated by zend_jit_switch(). This should be fixed in different way. In case we have to generate ENDBR we will have to generate additional gates...

.LABEL
    endbr
    jmp trace_exit_N

Copy link
Member

Choose a reason for hiding this comment

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

I mean, this should be fixed in zend_jit_switch()

| push byte i
|// 1:
| add aword [r4], n
Expand Down Expand Up @@ -3126,6 +3130,7 @@ static int zend_jit_set_ip(dasm_State **Dst, const zend_op *opline)

static int zend_jit_set_ip_ex(dasm_State **Dst, const zend_op *opline, bool set_ip_reg)
{
| ENDBR
Copy link
Member

Choose a reason for hiding this comment

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

ENDBR here is definitely wrong. Please identify, the code that causes the problem here.

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 traced jitted code and found ENDBR missing here. Let me see if I can provide a case to produce the problem. Thank you for your kind advice.

if (last_valid_opline == opline) {
zend_jit_use_last_valid_opline();
} else if (GCC_GLOBAL_REGS && last_valid_opline) {
Expand Down Expand Up @@ -3214,6 +3219,7 @@ static int zend_jit_check_exception_undef_result(dasm_State **Dst, const zend_op

static int zend_jit_trace_begin(dasm_State **Dst, uint32_t trace_num, zend_jit_trace_info *parent, uint32_t exit_num)
{
| ENDBR
Copy link
Member

Choose a reason for hiding this comment

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

This is also wrong. ENDBR for "root" traces is generated in zend_jit_prologue(). This ENDBR for "side" traces should be generated only if (parent != NULL)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I will add a judgment.

zend_regset regset = ZEND_REGSET_SCRATCH;

#if ZTS
Expand Down Expand Up @@ -15026,13 +15032,15 @@ static int zend_jit_switch(dasm_State **Dst, const zend_op *opline, const zend_o
} while (count);
|.code
|3:
| ENDBR
Copy link
Member

Choose a reason for hiding this comment

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

Actually, all the jump table targets should have ENDBR at start. This is going to be not a trivial fix...
We may use gates for side exits, but it's going to be an overhead for jumps to other basic blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Let me figure out a proper way to do this.

} else {
| LOAD_ADDR FCARG1a, jumptable
| EXT_CALL zend_hash_index_find, r0
if (!zend_jit_hash_jmp(Dst, opline, op_array, ssa, jumptable, default_b, default_label, next_opline, trace_info)) {
return 0;
}
|3:
| ENDBR
}
}
} else if (opline->opcode == ZEND_SWITCH_STRING) {
Expand Down Expand Up @@ -15074,6 +15082,7 @@ static int zend_jit_switch(dasm_State **Dst, const zend_op *opline, const zend_o
return 0;
}
|3:
| ENDBR
}
} else if (opline->opcode == ZEND_MATCH) {
if (op1_info & (MAY_BE_LONG|MAY_BE_STRING)) {
Expand Down