JIT: add missing endbr #10605
JIT: add missing endbr #10605
Conversation
PR php#8774 added IBT support for jitted code and passed all built-in tests in "ext/opcache/tests/jit". However, we found several "missing ENDBR" issues recently when running some real world workloads. Such workloads introduce new code path and thus more "indirect branch target" are in jited code. This fix adds missing endbr correspondingly. Signed-off-by: PeterYang12 <yuhan.yang@intel.com> Reviewed-by: chen-hu-97 <hu1.chen@intel.com> Reviewed-by: bjzhjing <cathy.zhang@intel.com>
can it be covered with simple unit tests? |
|
NO, I only encounter the issue in real world workloads. |
| } | ||
| | ENDBR |
There was a problem hiding this comment.
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_NThere was a problem hiding this comment.
I mean, this should be fixed in zend_jit_switch()
| @@ -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 | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
That makes sense. I will add a judgment.
| @@ -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 | |||
There was a problem hiding this comment.
ENDBR here is definitely wrong. Please identify, the code that causes the problem here.
There was a problem hiding this comment.
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.
| @@ -15026,13 +15032,15 @@ static int zend_jit_switch(dasm_State **Dst, const zend_op *opline, const zend_o | |||
| } while (count); | |||
| |.code | |||
| |3: | |||
| | ENDBR | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you. Let me figure out a proper way to do this.
|
Thank you, Dmitry, for your assistance. Our team has estimated that we will submit another patch when we have more availability in the coming quarters. Therefore, I will close this PR for now. Thanks again! |
PR #8774 added IBT support for jitted code and passed all built-in tests in "ext/opcache/tests/jit". However, we found several "missing ENDBR" issues recently when running some real world workloads.
Such workloads introduce new code path and thus more "indirect branch target" are in jited code. This fix adds missing endbr correspondingly.
Reviewed-by: chen-hu-97 hu1.chen@intel.com
Reviewed-by: bjzhjing cathy.zhang@intel.com