Skip to content

JIT/AArch64: [macos][ZTS] Support fast path for tlv_get_addr #7042

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 3 commits into from
May 31, 2021
Merged

JIT/AArch64: [macos][ZTS] Support fast path for tlv_get_addr #7042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44f1f14
JIT/AArch64: [macos][ZTS] Support fast path for tlv_get_addr
shqking May 20, 2021
73b78eb
JIT/AArch64: [macos][ZTS] Support fast path for tlv_get_addr
shqking May 27, 2021
bfcc537
Add TODO comment for the updates in zend_jit.c
shqking May 31, 2021
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
7 changes: 7 additions & 0 deletions TSRM/TSRM.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,10 +744,17 @@ TSRM_API size_t tsrm_get_ls_cache_tcb_offset(void)
#elif defined(__aarch64__)
size_t ret;

# ifdef __APPLE__
// Points to struct TLVDecriptor for _tsrm_ls_cache in macOS.
asm("adrp %0, #__tsrm_ls_cache@TLVPPAGE\n\t"
"ldr %0, [%0, #__tsrm_ls_cache@TLVPPAGEOFF]"
: "=r" (ret));
# else
asm("mov %0, xzr\n\t"
"add %0, %0, #:tprel_hi12:_tsrm_ls_cache, lsl #12\n\t"
"add %0, %0, #:tprel_lo12_nc:_tsrm_ls_cache"
: "=r" (ret));
# endif
return ret;
#else
return 0;
Expand Down
4 changes: 4 additions & 0 deletions ext/opcache/jit/zend_jit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4055,8 +4055,12 @@ ZEND_EXT_API void zend_jit_unprotect(void)
if (!(JIT_G(debug) & (ZEND_JIT_DEBUG_GDB|ZEND_JIT_DEBUG_PERF_DUMP))) {
int opts = PROT_READ | PROT_WRITE;
#ifdef ZTS
/* TODO: EXEC+WRITE is not supported in macOS. Removing EXEC is still buggy as
* other threads, which are executing the JITed code, would crash anyway. */
# ifndef __APPLE__
/* Another thread may be executing JITed code. */
opts |= PROT_EXEC;
# endif
#endif
if (mprotect(dasm_buf, dasm_size, opts) != 0) {
fprintf(stderr, "mprotect() failed [%d] %s\n", errno, strerror(errno));
Expand Down
25 changes: 25 additions & 0 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 @@ -184,6 +184,14 @@ const char* zend_reg_name[] = {

#if ZTS
static size_t tsrm_ls_cache_tcb_offset = 0;
# ifdef __APPLE__
struct TLVDescriptor {
void* (*thunk)(struct TLVDescriptor*);
uint64_t key;
uint64_t offset;
};
typedef struct TLVDescriptor TLVDescriptor;
# endif
#endif

/* By default avoid JITing inline handlers if it does not seem profitable due to lack of
Expand Down Expand Up @@ -483,10 +491,27 @@ static int logical_immediate_p (uint64_t value, uint32_t reg_size)
|| }
|.endmacro

// Safe memory load/store with an unsigned 64-bit offset.
|.macro SAFE_MEM_ACC_WITH_64_UOFFSET, ldr_str_ins, op, base_reg, offset, tmp_reg
|| if (((uintptr_t)(offset)) > LDR_STR_PIMM64) {
| LOAD_64BIT_VAL tmp_reg, offset
| ldr_str_ins op, [base_reg, tmp_reg]
|| } else {
| ldr_str_ins op, [base_reg, #(offset)]
|| }
|.endmacro

|.macro LOAD_TSRM_CACHE, reg
||#ifdef __APPLE__
| .long 0xd53bd071 // TODO: hard-coded: mrs TMP3, tpidrro_el0
| and TMP3, TMP3, #0xfffffffffffffff8
| SAFE_MEM_ACC_WITH_64_UOFFSET ldr, TMP3, TMP3, (((TLVDescriptor*)tsrm_ls_cache_tcb_offset)->key << 3), TMP1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slow path is avoided with the premise that tlv _tsrm_ls_cache is guaranteed to be accessed before the JIT-ed.

We have one concern that it would be a bit difficult for debugging if the premise is not satisfied with the software evolution in the future.

@dstogov Do you think we should add one check here?
How to do:

  1. add cbz TMP3, ->trap_label here
  2. define one global label named trap_label in one new defined stub function only for macOS case. In this label, brk #0 is the only instruction.

Impact:

  1. Performance: one cbz instruction would be executed each time, but it would not impose much overhead with the help of branch prediction.
  2. Code size: one cbz instruction would be produced for each invocation of macro LOAD_TSRM_CACHE.

Pros: In the future, if the premise is missing, it helps to debug.

Copy link
Member

Choose a reason for hiding this comment

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

I think, everything fine now. There is no a big difference between debugging brk #0 trap or crash right after mrs TMP3, tpidrro_el0. I prefer not to add cbz.

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 think the cbz is to check the value in TMP3 after the first load at line 508.
It's possible that the crash site is far away mrs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recall that the value in TMP3 after line 508 corresponds to the x17 after line 5 in https://gist.github.com/shqking/329d7712c26bad49786ab0a544a4af43#file-tlv_get_addr-s-L6

| SAFE_MEM_ACC_WITH_64_UOFFSET ldr, reg, TMP3, (((TLVDescriptor*)tsrm_ls_cache_tcb_offset)->offset), TMP1
||#else
| .long 0xd53bd051 // TODO: hard-coded: mrs TMP3, tpidr_el0
|| ZEND_ASSERT(tsrm_ls_cache_tcb_offset <= LDR_STR_PIMM64);
| ldr reg, [TMP3, #tsrm_ls_cache_tcb_offset]
||#endif
|.endmacro

|.macro LOAD_ADDR_ZTS, reg, struct, field
Expand Down